File remove problem

[Bruce Evans]

On Sun, 2 Dec 2007, Bruce Evans wrote:

> Here is a non-hackish patch which explains why ignoring MNT_RDONLY in
> the above or in ffs_mount() helps.  It just fixes the confusion between
> IN_MODIFIED and IN_CHANGE in critical places.
>
> % Index: ffs_softdep.c
> ...
>
> Without this change, soft updates depends on IN_CHANGE being converted
> to IN_MODIFIED by ufs_itimes(), but this conversion doesn't happen
> when MNT_RDONLY is set.  With soft updates, changes are often delayed
> until sync time, and when the sync is for mount-update it is done after
> setting MNT_RDONLY so the above doesn't work.
> ...
> Without the above patch, after "mv a b", "fsync b" is insufficient to
> make "mount -u -o ro /f" not corrupt the fs (because the mv generates
> 3 or 4 entries in the worklist, and the fsync generates 0 or 1 more
> and always ends up with 1 or 2 not handled, and then the sync for
> mount-update usually doesn't make any more progress).  I didn't check
> if a short delay after the fsync is sufficient, but expect that it is.
> With the above patch, a loop doing "mv a b; mount -u -o ro /f; fsck
> -n /f; mount -u -o rw /f; mv b a" shows no problems.  However, the
> original problem was without soft updates, so there must be more bugs
> here.

The above seems to fix "mv a b", but for "rm a" it only restores the
previous symptoms of the bug -- the "failed to flush worklist" error
is gone but the "update error" is back.

The following simiilar patch seems to fix "rm a" in 5.2:

% Index: ufs_inode.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_inode.c,v
% retrieving revision 1.53
% diff -u -2 -r1.53 ufs_inode.c
% --- ufs_inode.c	7 Apr 2004 03:47:20 -0000	1.53
% +++ ufs_inode.c	1 Dec 2007 17:26:30 -0000
% @@ -108,5 +111,5 @@
%  		ip->i_mode = 0;
%  		DIP(ip, i_mode) = 0;
% -		ip->i_flag |= IN_CHANGE | IN_UPDATE;
% +		ip->i_flag |= IN_MODIFIED;
%  		if (DOINGSOFTDEP(vp))
%  			softdep_change_linkcnt(ip);

This is in ufs_inactive().  Soft updates calls back here from the sync.
softdep_change_linkcount() just creates a dependency and depends on its
callers to set IN_* flags which will cause i/o to flush the dependency.
Here the caller sets flags that had no effect in the MNT_RDONLY case,
the same as in the previous patch.  I removed the IN_CHANGE and IN_UPDATE
flags completely since they just clobber the previous times (set by a
non-delayed ufs_inactive but probably already clobbered by a delayed
truncation).  (We are clearing out parts of the inode, so the times don't
really matter, but unless something else clears the times it is better
to leave the non-delayed times which record when the update marks for
the userland remove were converted to timestamps.)

Just before the above, the file is truncated.  ffs_truncate() has the
usual confusion between IN_CHANGE and IN_MODIFIED.  It sets
(IN_CHANGE | IN_UPDATE) in 7 different places, but never sets IN_MODIFIED.
In my test case of "rm a; mount -u -o ro /f", this isn't a problem, because
IN_MODIFIED is usually already set set ffs_truncate() doesn't forget to
start i/o.  ffs_truncate() calls ffs_update() with waitfor == 1 in many
cases, but in my test case it always calls ffs_update() with waitfor == 0.
Any call to ffs_update() in it cancels any previous setting of IN_MODIFIED
when modifications are moved to the buffer.  Thus when we forget to set
the IN_MODIFIED in the above code, in the MNT_RDONLY case we always have
inconsistencies:
- non-soft updates case: we have at least a cleared i_rdev and i_mode with
   no means to write them.  Is this enough to cause the original problem?
- soft updates case: we also have a dependency with no means to flush it.

I think this change doesn't help for -current yet, since this code is
unreachable when MNT_RDONLY is set.  The truncation code is also
unreachable.  kib's patch makes both reachable again.

I observe/predict/explain the following error behaviours for soft updates
(still some guesses):
-current the first patch: "/f: update error: blocks 32 files 1"
     (because the truncation and other things aren't done, but worklist
     items aren't created for them so the problem isn't detected in
     softdep_waitidle).
-5.2 with first patch: "/f: update error: blocks 0 files 1"
     (because the truncation works accidentally, and softdep_waitidle()
     doesn't exist to detect the unflushable worklist item created by
     the reachable softdep_change_linkcnt() call).
   Panic later (due to broken worklist after a buggy remount is permitted?).
-current with first patch and kib's patch: "softdep_waitidle: ..."
     (because the bug fixed in the second patch is exposed again and
     softdep_waitidle() detects it).

Bruce