File remove problem

[Bruce Evans]

On Sun, 2 Dec 2007, Kostik Belousov wrote:

> On Sat, Dec 01, 2007 at 02:07:50PM -0800, Don Lewis wrote:
>> On  1 Dec, Bruce Evans wrote:
>>> On Sat, 1 Dec 2007, Kostik Belousov wrote:
>>
>>>> +static int
>>>> +ffs_isronly(struct ufsmount *ump)
>>>> +{
>>>> +	struct fs *fs = ump->um_fs;
>>>> +
>>>> +	return (fs->fs_ronly);
>>>> +}
>>>> +
>>>
>>> Could be ump->um_fs->fs_ronly.
>>
>> That's the change that I would have made.  A #include for <ufs/ffs/fs.h>
>> would have to be added, which some might argue would be a layering
>> violation.  I'd prefer to avoid the extra indirection.
>
> I would argue that the ufs already knows too much about the ffs. But,
> this seems to be the first explicit reference to the ffs from the ufs
> code. With your approval, see below.

It's more like the fourth:
- ufs_itimes() is a layering violation.  However, with both ffs and ufs
   needing to set timestamps (for ffs, only in ffs_update()), and with
   both ffs and ufs both needing to set IN_* all over the place, it isn't
   clear which layer timstamps belong in.
- ufs_vnops.c now includes ffs_extern.h for some reason (5.2 didn't).
- ufs_gjournal.c includes both ffs_extern.h and fs.h.  It uses ip->i_fs
   a lot to access the superblock in ufs_gjournal_modref().

> diff --git a/sys/ufs/ufs/ufs_inode.c b/sys/ufs/ufs/ufs_inode.c
> index 448f436..22e29e9 100644
> --- a/sys/ufs/ufs/ufs_inode.c
> +++ b/sys/ufs/ufs/ufs_inode.c
> @@ -60,6 +60,7 @@ __FBSDID("$FreeBSD: src/sys/ufs/ufs/ufs_inode.c,v 1.69 2007/06/22 13:22:37 kib E
> #ifdef UFS_GJOURNAL
> #include <ufs/ufs/gjournal.h>
> #endif
> +#include <ufs/ffs/fs.h>
>
> /*
>  * Last reference to an inode.  If necessary, write or delete it.

ufs/ffs includes are conventionally separated from ufs/ufs includes by a
blank line.  About 2/3 of the files in ufs/ffs follow this convention.

> @@ -90,8 +91,7 @@ ufs_inactive(ap)
> 	ufs_gjournal_close(vp);
> #endif
> 	if ((ip->i_effnlink == 0 && DOINGSOFTDEP(vp)) ||
> -	    (ip->i_nlink <= 0 &&
> -	     (vp->v_mount->mnt_flag & MNT_RDONLY) == 0)) {
> +	    (ip->i_nlink <= 0 && !VFSTOUFS(mp)->um_fs->fs_ronly)) {
> 	loop:
> 		if (vn_start_secondary_write(vp, &mp, V_NOWAIT) != 0) {
> 			/* Cannot delete file while file system is suspended */
> @@ -121,7 +121,7 @@ ufs_inactive(ap)
> 	}
> 	if (ip->i_effnlink == 0 && DOINGSOFTDEP(vp))
> 		softdep_releasefile(ip);
> -	if (ip->i_nlink <= 0 && (vp->v_mount->mnt_flag & MNT_RDONLY) == 0) {
> +	if (ip->i_nlink <= 0 && !VFSTOUFS(mp)->um_fs->fs_ronly) {
> #ifdef QUOTA
> 		if (!getinoquota(ip))
> 			(void)chkiq(ip, -1, NOCRED, FORCE);
>

Should be ip->i_fs->fs_ronly.

The locking for fs_ronly is unclear.  It seems to be locked mainly by
vn_start_write(), and that enough for everything except probably access
time changes.

I've now tested the following similar change in ufs_itimes() after
removing all other related fixes.

% Index: ufs_vnops.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_vnops.c,v
% retrieving revision 1.293
% diff -u -2 -r1.293 ufs_vnops.c
% --- ufs_vnops.c	8 Nov 2007 17:21:51 -0000	1.293
% +++ ufs_vnops.c	2 Dec 2007 04:56:58 -0000
% @@ -89,4 +89,5 @@
%  #endif
% 
% +#include <ufs/ffs/fs.h>
%  #include <ufs/ffs/ffs_extern.h>
% 
% @@ -137,8 +138,38 @@
% 
%  	ip = VTOI(vp);
% +	/*
% +	 * MNT_RDONLY can barely be trusted here.  Full r/o mode is indicated
% +	 * by fs_ronly, and the MNT_RDONLY setting [should] differ from the
% +	 * fs_ronly setting only during transition from r/w mode to r/o mode.
% +	 * We set IN_ACCESS even in full r/o mode, so we must discard it
% +	 * unconditionally here.  During the transition, we must convert
% +	 * IN_CHANGE | IN_UPDATE to IN_MODIFIED, since some callers neglect
% +	 * to set IN_MODIFIED.  We also set the timestamps indicated by
% +	 * IN_CHANGE | IN_UPDATE normally during the transition, since the
% +	 * update marks may have been set correctly before the transition and
% +	 * not yet converted into timestamps.  Callers that set IN_CHANGE |
% +	 * IN_UPDATE during the transition are buggy, since userland writes
% +	 * are supposed to be denied (by MNT_RDONLY checks) during the
% +	 * transition, while kernel writes should should only be for syncs
% +	 * and syncs should not touch timestamps except to convert old
% +	 * update marks to timestamps.  Callers that set any update mark or
% +	 * modification flag except IN_ACCESS while in full r/o mode are
% +	 * broken; we will panic for them later.
% +	 */
%  	if ((vp->v_mount->mnt_flag & MNT_RDONLY) != 0)
% -		goto out;
% +		ip->i_flag &= ~IN_ACCESS;
%  	if ((ip->i_flag & (IN_ACCESS | IN_CHANGE | IN_UPDATE)) == 0)
%  		return;
% +	if (ip->i_fs->fs_ronly) {	/* XXX locking? */
% +		vprint("ufs_itimes_locked: r/o mod", vp);
% +		/*
% +		 * Should panic here, or return and let ffs_update() panic.
% +		 * The fs_ronly check in ffs_update() is now almost redundant
% +		 * and should not succeed, so it should be replaced by a
% +		 * panic.  It detects more invariants failures than we detect
% +		 * here.
% +		 */
% +		goto out;
% +	}
% 
%  	if ((vp->v_type == VBLK || vp->v_type == VCHR) && !DOINGSOFTDEP(vp))

The comments in this are too verbose.

This seems to work for "rm a; mount -u -o ro /f" and "mv a b; mount ...",
but since this is without your change to ufs_inactive(), I'm now surprised
that it works.  I think it works for the simple test cases as follows:

- there are no unlinked open files, so ufs_inactive() should have already
   set up all the needed i/o, and the sync for the mount-update should
   finish that i/o.

- however, in ufs_inactive() seems to be called as part of the sync, and
   in ~5.2 where it doesn't do any read-only checks, it seems to call
   ffs_truncate().  This truncate should be null (in the simple test
   cases), but it seems to have the side effect of generating more i/o
   (I hope just to convert bogus settings of IN_CHANGE | IN_UPDATE into
   dinode changes).  Then other bugs cause an inconsistent fs if MNT_RDONLY
   is set.

   BTW, the other bugs don't affect plain 5.2, since it still has
   rev.1.182 of ufs_vnops.c to convert the bogus settings of IN_CHANGE |
   IN_UPDATE into IN_MODIFIED.  I was "lucky" to see almost the same
   bugs in ~5.2 as in -current because I have debugging code in
   ffs_update() instead of rev.1.182 in ufs_vnops.c, but the debugging
   code showed too many apparently-harmless problems so it was turned
   off.

In cases involving unlinked open files, the truncation has to be delayed
until the sync.  Things seem to work reasonably:  If a file on the fs
is open for read, then mount-update from rw to ro is allowed unless the
file is unlinked; if the file is unlinked then there is an EBUSY error
unless MNT_FORCE is used, but if MNT_FORCE is used, then the mount-update
must be allowed to complete and this involves truncating and otherwise
completing the removal of unlinked open files.  In -current, your patch
should make this work again, and with only my patch above the
"update error: blocks 32: files 1" is back because ufs_inactive() doesn't
do the truncation.

I don't understand how WRITECLOSE inter-operates with this -- mount-update
always sets it but there is still an EBUSY error unless MNT_FORCE is
used, while MNT_FORCE should kill all opens.

Bruce