Subject: FFS reliability problems
To: None <tech-kern@netbsd.org>
From: Charles M. Hannum <abuse@spamalicious.com>
List: tech-kern
Date: 05/14/2002 14:38:35
[Note: This problem was discovered in another *BSD variant, but the
code in NetBSD appears to be the same, so...]

I've been running some file system reliability tests, and have
discovered a couple of interesting things about mkdir(2) and rmdir(2)
on FFS:

1) In addition to the other mkdir(2) ordering problem introduced with
   the softdep code (which I already fixed), there is another.  The
   code currently does not guarantee that the data block is written
   before the block pointer, so a crash in the middle of mkdir(2)
   often results in a `DIRECTORY CORRUPTED' error from fsck(8).
   (Note: This depends on what was in the block before, so it doesn't
   happen every time, and is less likely to happen with each run of
   the test.)

   I include a somewhat ugly patch for this below.  I didn't want to
   change the behavior in the softdep case, but it is probably wrong.

2) Since it uses VOP_TRUNCATE() to do the dirty work, ufs_rmdir() will
   zero the inode block pointers long before it updates i_blocks or
   zeroes the mode.  While not technically a bug, this results in lots
   of `INCORRECT BLOCK COUNT' and `ZERO LENGTH DIR' errors from
   fsck(8), which is undesirable.

   My current thought on this is to add an `I'm about to delete this'
   flag to VOP_TRUNCATE() so that it can fully bash the inode.
   However, there are issues about how this interacts with
   ufs_inactive().

   An alternative, specifically for rmdir(2), is to rearrange
   ufs_inactive() slightly and *not* call VOP_TRUNCATE() from
   ufs_rmdir(), instead allowing ufs_inactive() do it.  This might
   have the undesirable property of leaving the directory accessible
   as long as there is a cwd there, but I'm not sure.


Index: ufs_vnops.c
===================================================================
RCS file: /cvsroot/syssrc/sys/ufs/ufs/ufs_vnops.c,v
retrieving revision 1.85
diff -u -r1.85 ufs_vnops.c
--- ufs_vnops.c	2001/12/23 16:16:59	1.85
+++ ufs_vnops.c	2002/05/14 14:37:15
@@ -1282,10 +1282,6 @@
 			blkoff += DIRBLKSIZ;
 		}
 	}
-	if ((error = VOP_UPDATE(tvp, NULL, NULL, UPDATE_DIROP)) != 0) {
-		(void)VOP_BWRITE(bp);
-		goto bad;
-	}
 	/*
 	 * Directory set up, now install it's entry in the parent directory.
 	 *
@@ -1297,8 +1293,15 @@
 	 * an appropriate ordering dependency to the buffer which ensures that
 	 * the buffer is written before the new name is written in the parent.
 	 */
-	if (!DOINGSOFTDEP(dvp) && ((error = VOP_BWRITE(bp)) != 0))
+	if (!DOINGSOFTDEP(dvp) && ((error = VOP_BWRITE(bp)) != 0)) {
+		(void)VOP_UPDATE(tvp, NULL, NULL, UPDATE_DIROP);
 		goto bad;
+	}
+	if ((error = VOP_UPDATE(tvp, NULL, NULL, UPDATE_DIROP)) != 0) {
+		if (DOINGSOFTDEP(tvp))
+			(void)VOP_BWRITE(bp);
+		goto bad;
+	}
 	ufs_makedirentry(ip, cnp, &newdir);
 	error = ufs_direnter(dvp, tvp, &newdir, cnp, bp);
  bad: