Date: Sat, 1 Dec 2007 22:34:45 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Kostik Belousov <kostikbel@gmail.com> Cc: freebsd-fs@freebsd.org, Don Lewis <truckman@freebsd.org> Subject: Re: File remove problem Message-ID: <20071201215706.B12006@besplex.bde.org> In-Reply-To: <20071201080709.GO83121@deviant.kiev.zoral.com.ua> References: <20071130052840.GH83121@deviant.kiev.zoral.com.ua> <200712010715.lB17FlZw011929@gw.catspoiler.org> <20071201080709.GO83121@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20071201215706.B12006>