every iput() and namei() must be inside a transaction

2c565472 · Robert Morris · 020c8e23 · 2c565472 · 2c565472 · 2c565472
--- a/exec.c
+++ b/exec.c
@@ -18,8 +18,11 @@ exec(char *path, char **argv)
  struct proghdr ph;
  pde_t *pgdir, *oldpgdir;

-  if((ip = namei(path)) == 0)
+  begin_trans();
+  if((ip = namei(path)) == 0){
+    commit_trans();
    return -1;
+  }
  ilock(ip);
  pgdir = 0;

@@ -47,6 +50,7 @@ exec(char *path, char **argv)
      goto bad;
  }
  iunlockput(ip);
+  commit_trans();
  ip = 0;

  // Allocate two pages at the next page boundary.
@@ -95,7 +99,9 @@ exec(char *path, char **argv)
 bad:
  if(pgdir)
    freevm(pgdir);
-  if(ip)
+  if(ip){
    iunlockput(ip);
+    commit_trans();
+  }
  return -1;
 }
--- a/fs.c
+++ b/fs.c
@@ -314,6 +314,8 @@ iunlock(struct inode *ip)
 // be recycled.
 // If that was the last reference and the inode has no links
 // to it, free the inode (and its content) on disk.
+// All calls to iput() must be inside a transaction in
+// case it has to free the inode.
 void
 iput(struct inode *ip)
 {
@@ -601,6 +603,7 @@ skipelem(char *path, char *name)
 // Look up and return the inode for a path name.
 // If parent != 0, return the inode for the parent and copy the final
 // path element into name, which must have room for DIRSIZ bytes.
+// Must be called inside a transaction since it calls iput().
 static struct inode*
 namex(char *path, int nameiparent, char *name)
 {

--- a/log.c
+++ b/log.c
@@ -5,7 +5,7 @@
 #include "fs.h"
 #include "buf.h"

-// Simple logging. Each system call that might write the file system
+// Simple logging. Each file system system call
 // should be surrounded with begin_trans() and commit_trans() calls.
 //
 // The log holds at most one transaction at a time. Commit forces
@@ -18,10 +18,6 @@
 // one transaction reading a block that another one has modified,
 // for example an i-node block.
 //
-// Read-only system calls don't need to use transactions, though
-// this means that they may observe uncommitted data. I-node and
-// buffer locks prevent read-only calls from seeing inconsistent data.
-//
 // The log is a physical re-do log containing disk blocks.
 // The on-disk log format:
 //   header block, containing sector #s for block A, B, C, ...

--- a/proc.c
+++ b/proc.c
@@ -186,7 +186,9 @@ exit(void)
    }
  }

+  begin_trans();
  iput(proc->cwd);
+  commit_trans();
  proc->cwd = 0;

  acquire(&ptable.lock);

--- a/sysfile.c
+++ b/sysfile.c
@@ -120,10 +120,12 @@ sys_link(void)

  if(argstr(0, &old) < 0 || argstr(1, &new) < 0)
    return -1;
-  if((ip = namei(old)) == 0)
-    return -1;

  begin_trans();
+  if((ip = namei(old)) == 0){
+    commit_trans();
+    return -1;
+  }

  ilock(ip);
  if(ip->type == T_DIR){
@@ -186,10 +188,12 @@ sys_unlink(void)

  if(argstr(0, &path) < 0)
    return -1;
-  if((dp = nameiparent(path, name)) == 0)
-    return -1;

  begin_trans();
+  if((dp = nameiparent(path, name)) == 0){
+    commit_trans();
+    return -1;
+  }

  ilock(dp);

@@ -286,18 +290,24 @@ sys_open(void)

  if(argstr(0, &path) < 0 || argint(1, &omode) < 0)
    return -1;
+
+  begin_trans();
+
  if(omode & O_CREATE){
-    begin_trans();
    ip = create(path, T_FILE, 0, 0);
-    commit_trans();
-    if(ip == 0)
+    if(ip == 0){
+      commit_trans();
      return -1;
+    }
  } else {
-    if((ip = namei(path)) == 0)
+    if((ip = namei(path)) == 0){
+      commit_trans();
      return -1;
+    }
    ilock(ip);
    if(ip->type == T_DIR && omode != O_RDONLY){
      iunlockput(ip);
+      commit_trans();
      return -1;
    }
  }
@@ -306,9 +316,11 @@ sys_open(void)
    if(f)
      fileclose(f);
    iunlockput(ip);
+    commit_trans();
    return -1;
  }
  iunlock(ip);
+  commit_trans();

  f->type = FD_INODE;
  f->ip = ip;
@@ -361,15 +373,20 @@ sys_chdir(void)
  char *path;
  struct inode *ip;

-  if(argstr(0, &path) < 0 || (ip = namei(path)) == 0)
+  begin_trans();
+  if(argstr(0, &path) < 0 || (ip = namei(path)) == 0){
+    commit_trans();
    return -1;
+  }
  ilock(ip);
  if(ip->type != T_DIR){
    iunlockput(ip);
+    commit_trans();
    return -1;
  }
  iunlock(ip);
  iput(proc->cwd);
+  commit_trans();
  proc->cwd = ip;
  return 0;
 }

--- a/usertests.c
+++ b/usertests.c
@@ -13,6 +13,106 @@ char name[3];
 char *echoargv[] = { "echo", "ALL", "TESTS", "PASSED", 0 };
 int stdout = 1;

+// does chdir() call iput(p->cwd) in a transaction?
+void
+iputtest(void)
+{
+  printf(stdout, "iput test\n");
+
+  if(mkdir("iputdir") < 0){
+    printf(stdout, "mkdir failed\n");
+    exit();
+  }
+  if(chdir("iputdir") < 0){
+    printf(stdout, "chdir iputdir failed\n");
+    exit();
+  }
+  if(unlink("../iputdir") < 0){
+    printf(stdout, "unlink ../iputdir failed\n");
+    exit();
+  }
+  if(chdir("/") < 0){
+    printf(stdout, "chdir / failed\n");
+    exit();
+  }
+  printf(stdout, "iput test ok\n");
+}
+
+// does exit() call iput(p->cwd) in a transaction?
+void
+exitiputtest(void)
+{
+  int pid;
+
+  printf(stdout, "exitiput test\n");
+
+  pid = fork();
+  if(pid < 0){
+    printf(stdout, "fork failed\n");
+    exit();
+  }
+  if(pid == 0){
+    if(mkdir("iputdir") < 0){
+      printf(stdout, "mkdir failed\n");
+      exit();
+    }
+    if(chdir("iputdir") < 0){
+      printf(stdout, "child chdir failed\n");
+      exit();
+    }
+    if(unlink("../iputdir") < 0){
+      printf(stdout, "unlink ../iputdir failed\n");
+      exit();
+    }
+    exit();
+  }
+  wait();
+  printf(stdout, "exitiput test ok\n");
+}
+
+// does the error path in open() for attempt to write a
+// directory call iput() in a transaction?
+// needs a hacked kernel that pauses just after the namei()
+// call in sys_open():
+//    if((ip = namei(path)) == 0)
+//      return -1;
+//    {
+//      int i;
+//      for(i = 0; i < 10000; i++)
+//        yield();
+//    }
+void
+openiputtest(void)
+{
+  int pid;
+
+  printf(stdout, "openiput test\n");
+  if(mkdir("oidir") < 0){
+    printf(stdout, "mkdir oidir failed\n");
+    exit();
+  }
+  pid = fork();
+  if(pid < 0){
+    printf(stdout, "fork failed\n");
+    exit();
+  }
+  if(pid == 0){
+    int fd = open("oidir", O_RDWR);
+    if(fd >= 0){
+      printf(stdout, "open directory for write succeeded\n");
+      exit();
+    }
+    exit();
+  }
+  sleep(1);
+  if(unlink("oidir") != 0){
+    printf(stdout, "unlink failed\n");
+    exit();
+  }
+  wait();
+  printf(stdout, "openiput test ok\n");
+}
+
 // simple file system tests

 void
@@ -187,7 +287,7 @@ void dirtest(void)
    printf(stdout, "unlink dir0 failed\n");
    exit();
  }
-  printf(stdout, "mkdir test\n");
+  printf(stdout, "mkdir test ok\n");
 }

 void
@@ -1628,6 +1728,10 @@ main(int argc, char *argv[])
  writetest1();
  createtest();

+  openiputtest();
+  exitiputtest();
+  iputtest();
+
  mem();
  pipe1();
  preempt();