File remove problem

[Bruce Evans]

On Sat, 1 Dec 2007, Kostik Belousov wrote:

> On Fri, Nov 30, 2007 at 11:15:47PM -0800, Don Lewis wrote:
>> On 30 Nov, Kostik Belousov wrote:

>>> As a speculation, it might be that ufs_inactive() should conditionalize on
>>> fs_ronly instead of MNT_RDONLY. Then, VOP_INACTIVE() would set up the
>>> IN_CHANGE|IN_UPDATE and finally call the ffs_update() ?
>>
>> That sounds reasonable to me.  I see that ffs_update(), which is called
>> by ufs_inactive(), looks at fs_ronly.
> The patch (compile-tested only) is below.
>
> diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c
> index cbccc62..687011d 100644
> --- a/sys/ufs/ffs/ffs_vfsops.c
> +++ b/sys/ufs/ffs/ffs_vfsops.c
> @@ -79,6 +79,7 @@ static void	ffs_oldfscompat_read(struct fs *, struct ufsmount *,
> 		    ufs2_daddr_t);
> static void	ffs_oldfscompat_write(struct fs *, struct ufsmount *);
> static void	ffs_ifree(struct ufsmount *ump, struct inode *ip);
> +static int	ffs_isronly(struct ufsmount *ump);
> static vfs_init_t ffs_init;
> static vfs_uninit_t ffs_uninit;
> static vfs_extattrctl_t ffs_extattrctl;
> @@ -748,6 +749,7 @@ ffs_mountfs(devvp, mp, td)
> 	ump->um_valloc = ffs_valloc;
> 	ump->um_vfree = ffs_vfree;
> 	ump->um_ifree = ffs_ifree;
> +	ump->um_isronly = ffs_isronly;
> 	mtx_init(UFS_MTX(ump), "FFS", "FFS Lock", MTX_DEF);
> 	bcopy(bp->b_data, ump->um_fs, (u_int)fs->fs_sbsize);
> 	if (fs->fs_sbsize < SBLOCKSIZE)

I don't like these indirections.  The layering provided by them is pseudo.

> @@ -1862,3 +1864,12 @@ ffs_geom_strategy(struct bufobj *bo, struct buf *bp)
> 	}
> 	g_vfs_strategy(bo, bp);
> }
> +
> +static int
> +ffs_isronly(struct ufsmount *ump)
> +{
> +	struct fs *fs = ump->um_fs;
> +
> +	return (fs->fs_ronly);
> +}
> +

Could be ump->um_fs->fs_ronly.

> diff --git a/sys/ufs/ufs/ufs_inode.c b/sys/ufs/ufs/ufs_inode.c
> index 448f436..a9abbeb 100644
> --- a/sys/ufs/ufs/ufs_inode.c
> +++ b/sys/ufs/ufs/ufs_inode.c
> @@ -90,8 +90,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 && !UFS_ISRONLY(ip->i_ump))) {
> 	loop:
> 		if (vn_start_secondary_write(vp, &mp, V_NOWAIT) != 0) {
> 			/* Cannot delete file while file system is suspended */

This might help for non-soft updates, but with soft updates I also see
the problem in ~5.2 which doesn't have any r/o check here (5.2 doesn't
have this loop at all).  I haven't seen the problem in ~5.2 without
soft updates.

> @@ -121,7 +120,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 && !UFS_ISRONLY(ip->i_ump)) {
> #ifdef QUOTA
> 		if (!getinoquota(ip))
> 			(void)chkiq(ip, -1, NOCRED, FORCE);

5.2 only has this i_nlink check, before a v_write_suspend_wait() call, and
without any r/o check.

With the MNT_RDONLY check here, the truncation isn't done, which is
clearly wrong.  However, I think this isn't the main problem here.
The truncation always does get done in simple test cases like
"rm /f/a; mount -u -o ro /f" which show the problem.  Then there
is no race between the rm and the mount -- ufs_inactive() completes
normally in the rm process before the mount process sets MNT_RDONLY.
I think soft updates (or just sync?) calls ufs_inactive again later
when MNT_RDONLY is set, but it should find nothing more to do then.

Anyway, doesn't the vn_start_write() in ffs_mount() wait for all writers
like the rm process to complete?  Mount sets MNT_RDONLY before that,
so there may be a problem before that.  There is only a single
vn_start_write() call in vfs_mount.c.  It is interesting that this
wraps ffs_unmount() which seems to work.  Mount-update may need to wait
for writers to go away before it does things even more than unmount,
since it changes active mount flags like MNT_RDONLY.

Bruce