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: