Fix: Avoid closing the file after loading segments to be able to lazily load from file without opening, w/ G

src/userprog/exception.c

@@ -13,8 +13,7 @@ static long long page_fault_cnt;
 
 static void kill (struct intr_frame *);
 static void page_fault (struct intr_frame *);
-static bool try_fetch_page (void *upage, bool not_present, bool write,
-                            bool user);
+static bool try_fetch_page (void *upage, bool write);
 
 /* Registers handlers for interrupts that can be caused by user
    programs.
@@ -150,9 +149,15 @@ page_fault (struct intr_frame *f)
   write = (f->error_code & PF_W) != 0;
   user = (f->error_code & PF_U) != 0;
 
+  /* If the fault address is in a user page that is not present, then it might
+     just need to be lazily loaded. So, we check our SPT to see if the page
+     is expected to have data loaded in memory. */
   void *upage = pg_round_down (fault_addr);
-  if (try_fetch_page (upage, not_present, write, user))
-    return;
+  if (not_present && is_user_vaddr (upage) && upage != NULL)
+    {
+      if (try_fetch_page (upage, write))
+        return;
+    }
 
   /* To implement virtual memory, delete the rest of the function
      body, and replace it with code that brings in the page to
@@ -166,11 +171,10 @@ page_fault (struct intr_frame *f)
 }
 
 static bool
-try_fetch_page (void *upage, bool not_present, bool write, bool user)
+try_fetch_page (void *upage, bool write)
 {
-  if (!not_present || !is_user_vaddr (upage) || upage == NULL)
-    return false;
-
+  /* Check if the page is in the supplemental page table. That is, it is a page
+     that is expected to be in memory. */
   struct page_entry *page = page_get (upage);
   if (page == NULL)
     return false;
@@ -182,6 +186,7 @@ try_fetch_page (void *upage, bool not_present, bool write, bool user)
       return true;
     }
 
+  /* Load the page into memory based on the type of data it is expecting. */
   switch (page->type) {
     case PAGE_EXECUTABLE:
       return page_load (page, write);

src/userprog/process.c

@@ -623,7 +623,6 @@ load (const char *file_name, void (**eip) (void), void **esp)
 
  done:
   /* We arrive here whether the load is successful or not. */
-  file_close (file);
   lock_release (&filesys_lock);
   return success;
 }
@@ -691,14 +690,13 @@ validate_segment (const struct Elf32_Phdr *phdr, struct file *file)
    or disk read error occurs. */
 static bool
 load_segment (struct file *file, off_t ofs, uint8_t *upage,
-              uint32_t read_bytes, uint32_t zero_bytes, bool writable) 
+              uint32_t read_bytes, uint32_t zero_bytes, bool writable)
 {
   ASSERT ((read_bytes + zero_bytes) % PGSIZE == 0);
   ASSERT (pg_ofs (upage) == 0);
   ASSERT (ofs % PGSIZE == 0);
 
-  file_seek (file, ofs);
-  while (read_bytes > 0 || zero_bytes > 0) 
+  while (read_bytes > 0 || zero_bytes > 0)
     {
       /* Calculate how to fill this page.
          We will read PAGE_READ_BYTES bytes from FILE
@@ -706,15 +704,15 @@ load_segment (struct file *file, off_t ofs, uint8_t *upage,
       size_t page_read_bytes = read_bytes < PGSIZE ? read_bytes : PGSIZE;
       size_t page_zero_bytes = PGSIZE - page_read_bytes;
 
-      /* Add the page to the SPT */
+      /* Add the page metadata to the SPT to be lazy loaded later on */
       if (page_insert (file, ofs, upage, page_read_bytes, page_zero_bytes,
                        writable, PAGE_EXECUTABLE) == NULL)
         return false;
 
       /* Advance. */
-      ofs += page_read_bytes;
       read_bytes -= page_read_bytes;
       zero_bytes -= page_zero_bytes;
+      ofs += PGSIZE;
       upage += PGSIZE;
     }
   return true;

src/vm/page.c

@@ -69,30 +69,34 @@ page_get (void *upage)
 bool
 page_load (struct page_entry *page, bool writable)
 {
-  void *frame = frame_alloc (PAL_USER, page->upage, thread_current());
+  /* Allocate a frame for the page. If a frame allocation fails, then
+     frame_alloc should try to evict a page. If it is still NULL, the OS
+     panics as this should not happen if eviction is working correctly. */
+  void *frame = frame_alloc (PAL_USER, page->upage, thread_current ());
   if (frame == NULL)
-    return false; // TODO: Try to evict a page instead.
+    PANIC ("Could not allocate a frame to load page into memory.");
 
+  /* Map the page to the frame. */
   if (!install_page (page->upage, frame, writable))
-  {
-    frame_free (frame);
-    return false;
-  }
-
-  printf ("Hi! 3\n");
+    {
+      frame_free (frame);
+      return false;
+    }
 
+  /* Move the file pointer to the correct location in the file. Then, read the
+     data from the file into the frame. Checks that we were able to read the
+     expected number of bytes. */
   file_seek (page->file, page->offset);
-  int read = -1;
-  if ((read = file_read (page->file, frame, page->read_bytes)) != (int) page->read_bytes)
+  if (file_read (page->file, frame, page->read_bytes) != (int) page->read_bytes)
   {
-    printf ("%p, %p, %d %d\n", page->file, frame, read, page->offset);
     frame_free (frame);
     return false;
   }
 
-  printf ("Hi! 4\n");
-
+  /* Zero out the remaining bytes in the frame. */
   memset (frame + page->read_bytes, 0, page->zero_bytes);
+
+  /* Mark the page as loaded successfully. */
   return true;
 }