From owner-freebsd-fs@FreeBSD.ORG Sat Dec 1 11:34:50 2007 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 4556E16A474 for ; Sat, 1 Dec 2007 11:34:50 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id E2BD113C46E for ; Sat, 1 Dec 2007 11:34:49 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c211-30-219-213.carlnfd3.nsw.optusnet.com.au [211.30.219.213]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id lB1BYjFJ030796 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 1 Dec 2007 22:34:48 +1100 Date: Sat, 1 Dec 2007 22:34:45 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Kostik Belousov In-Reply-To: <20071201080709.GO83121@deviant.kiev.zoral.com.ua> Message-ID: <20071201215706.B12006@besplex.bde.org> References: <20071130052840.GH83121@deviant.kiev.zoral.com.ua> <200712010715.lB17FlZw011929@gw.catspoiler.org> <20071201080709.GO83121@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-fs@freebsd.org, Don Lewis Subject: Re: File remove problem X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 01 Dec 2007 11:34:50 -0000 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