avoid deadlock by calling begin_trans() before locking any inodes

5053dd6a · Robert Morris · c95ce31c · 5053dd6a · 5053dd6a · 5053dd6a
--- a/file.c
+++ b/file.c
@@ -118,7 +118,6 @@ filewrite(struct file *f, char *addr, int n)
  if(f->type == FD_PIPE)
    return pipewrite(f->pipe, addr, n);
  if(f->type == FD_INODE){
-    ilock(f->ip);
    // write a few blocks at a time to avoid exceeding
    // the maximum log transaction size, including
    // i-node, indirect block, allocation blocks,
@@ -131,9 +130,13 @@ filewrite(struct file *f, char *addr, int n)
      int n1 = n - i;
      if(n1 > max)
        n1 = max;
+
      begin_trans();
+      ilock(f->ip);
      r = writei(f->ip, addr + i, f->off, n1);
+      iunlock(f->ip);
      commit_trans();
+
      if(r < 0)
        break;
      if(r != n1)
@@ -141,7 +144,6 @@ filewrite(struct file *f, char *addr, int n)
      f->off += r;
      i += r;
    }
-    iunlock(f->ip);
    return i == n ? n : -1;
  }
  panic("filewrite");

--- a/fs.c
+++ b/fs.c
@@ -43,13 +43,13 @@ bzero(int dev, int bno)
  
  bp = bread(dev, bno);
  memset(bp->data, 0, BSIZE);
-  bwrite(bp);
+  log_write(bp);
  brelse(bp);
 }

 // Blocks. 

-// Allocate a disk block.
+// Allocate a zeroed disk block.
 static uint
 balloc(uint dev)
 {
@@ -67,6 +67,7 @@ balloc(uint dev)
        bp->data[bi/8] |= m;  // Mark block in use on disk.
        log_write(bp);
        brelse(bp);
+        bzero(dev, b + bi);
        return b + bi;
      }
    }
@@ -83,8 +84,6 @@ bfree(int dev, uint b)
  struct superblock sb;
  int bi, m;

-  bzero(dev, b);
-
  readsb(dev, &sb);
  bp = bread(dev, BBLOCK(b, sb.ninodes));
  bi = b % BPB;

--- a/log.c
+++ b/log.c
@@ -124,9 +124,9 @@ static void
 recover_from_log(void)
 {
  read_head();      
-  install_trans();  // Install all transactions till head
+  install_trans(); // if committed, copy from log to disk
  log.lh.n = 0;
-  write_head();     //  Reclaim log
+  write_head(); // clear the log
 }

 void

--- a/sysfile.c
+++ b/sysfile.c
@@ -116,14 +116,16 @@ sys_link(void)
    return -1;
  if((ip = namei(old)) == 0)
    return -1;
+
+  begin_trans();
+
  ilock(ip);
  if(ip->type == T_DIR){
    iunlockput(ip);
+    commit_trans();
    return -1;
  }

-  begin_trans();
-
  ip->nlink++;
  iupdate(ip);
  iunlock(ip);
@@ -180,16 +182,21 @@ sys_unlink(void)
    return -1;
  if((dp = nameiparent(path, name)) == 0)
    return -1;
+
+  begin_trans();
+
  ilock(dp);

  // Cannot unlink "." or "..".
  if(namecmp(name, ".") == 0 || namecmp(name, "..") == 0){
    iunlockput(dp);
+    commit_trans();
    return -1;
  }

  if((ip = dirlookup(dp, name, &off)) == 0){
    iunlockput(dp);
+    commit_trans();
    return -1;
  }
  ilock(ip);
@@ -199,11 +206,10 @@ sys_unlink(void)
  if(ip->type == T_DIR && !isdirempty(ip)){
    iunlockput(ip);
    iunlockput(dp);
+    commit_trans();
    return -1;
  }

-  begin_trans();
-
  memset(&de, 0, sizeof(de));
  if(writei(dp, (char*)&de, off, sizeof(de)) != sizeof(de))
    panic("unlink: writei");

--- a/usertests.c
+++ b/usertests.c
@@ -364,6 +364,8 @@ sharedfd(void)
  int fd, pid, i, n, nc, np;
  char buf[10];

+  printf(1, "sharedfd test\n");
+
  unlink("sharedfd");
  fd = open("sharedfd", O_CREATE|O_RDWR);
  if(fd < 0){
@@ -655,7 +657,7 @@ linktest(void)
  printf(1, "linktest ok\n");
 }

-// test concurrent create and unlink of the same file
+// test concurrent create/link/unlink of the same file
 void
 concreate(void)
 {
@@ -728,10 +730,15 @@ concreate(void)
    }
    if(((i % 3) == 0 && pid == 0) ||
       ((i % 3) == 1 && pid != 0)){
-      fd = open(file, 0);
-      close(fd);
+      close(open(file, 0));
+      close(open(file, 0));
+      close(open(file, 0));
+      close(open(file, 0));
    } else {
      unlink(file);
+      unlink(file);
+      unlink(file);
+      unlink(file);
    }
    if(pid == 0)
      exit();
@@ -742,6 +749,42 @@ concreate(void)
  printf(1, "concreate ok\n");
 }

+// another concurrent link/unlink/create test,
+// to look for deadlocks.
+void
+linkunlink()
+{
+  int pid, i;
+
+  printf(1, "linkunlink test\n");
+
+  unlink("x");
+  pid = fork();
+  if(pid < 0){
+    printf(1, "fork failed\n");
+    exit();
+  }
+
+  unsigned int x = (pid ? 1 : 97);
+  for(i = 0; i < 100; i++){
+    x = x * 1103515245 + 12345;
+    if((x % 3) == 0){
+      close(open("x", O_RDWR | O_CREATE));
+    } else if((x % 3) == 1){
+      link("cat", "x");
+    } else {
+      unlink("x");
+    }
+  }
+
+  if(pid)
+    wait();
+  else 
+    exit();
+
+  printf(1, "linkunlink ok\n");
+}
+
 // directory that uses indirect blocks
 void
 bigdir(void)
@@ -1518,6 +1561,7 @@ main(int argc, char *argv[])
  bigfile();
  subdir();
  concreate();
+  linkunlink();
  linktest();
  unlinkread();
  createdelete();