Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 1 Dec 2007 10:07:09 +0200
From:      Kostik Belousov <kostikbel@gmail.com>
To:        Don Lewis <truckman@freebsd.org>
Cc:        freebsd-fs@freebsd.org
Subject:   Re: File remove problem
Message-ID:  <20071201080709.GO83121@deviant.kiev.zoral.com.ua>
In-Reply-To: <200712010715.lB17FlZw011929@gw.catspoiler.org>
References:  <20071130052840.GH83121@deviant.kiev.zoral.com.ua> <200712010715.lB17FlZw011929@gw.catspoiler.org>

next in thread | previous in thread | raw e-mail | index | archive | help

--f4HxWLVbzokH9yio
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Nov 30, 2007 at 11:15:47PM -0800, Don Lewis wrote:
> On 30 Nov, Kostik Belousov wrote:
> > On Fri, Nov 30, 2007 at 03:58:55PM +1100, Bruce Evans wrote:
> >> On Fri, 30 Nov 2007, David Cecil wrote:
> >>=20
> >> >Thanks Bruce.
> >> >
> >> >Actually, I had found the same problem, and I came up with the first =
line=20
> >> >of your patch (adding IN_MODIFIED) myself, but I still saw the proble=
m.  I
> >>=20
> >> Yes, it's not that.  Testing reminded me that there is normally a
> >> VOP_INACTIVE() after unlink so the IN_CHANGE mark doesn't live very lo=
ng
> >> for unlink (it can only live long for open files).
> >>=20
> >> Testing shows that the problem is easy to reproduce and often partially
> >> detected before it becomes fatal.  I saw something like the following:
> >>=20
> >>     after touch a; ln a b; rm a; unmount -- no problem with 1 link rem=
aining
> >>     after touch a;         rm a; unmount -- no problem with unmount
> >>     after touch a; ln a b; rm a; mount -u o ro -- no problem with 1 li=
nk...
> >>     after touch a;       ; rm a; mount -u o ro -- worked once without =
soft
> >>        updates but seemed to be responsible for a soft update panic la=
ter
> >>     after touch a;       ; rm a; mount -u o ro -- usually fails with s=
oft
> >>        updates; the error is detected in various ways:
> >>           under ~5.2, mount -u prints "/f: update error: blocks 0 file=
s 1"
> >>              but succeeds
> >>           under -current, mount -u fails and a subroutine prints
> >> 	     "softdep_waitidle: Failed to flush worklist for 0xc3e1a29c"
> >> 	     However, mount -u apparently cannot afford to fail at this
> >> 	     poing since it has committed to succeeding -- further
> >> 	     mount -u's and unmounts fail and it takes a reboot to reach
> >> 	     an fsck that can fix the problem.
> >>=20
> >> 	  mount -u seems to do some things right: at least under -current:
> >> 	  - it calls ffs_sync() and thus ffs_update() with waitfor !=3D 0.
> >> 	  - IN_MODIFIED is usually already set in ffs_update().
> >> 	  - softdep_update_inode_inodeblock() in ffs_update() seems to
> >> 	    make null changes.  That doesn't seem right -- shouldn't it
> >> 	    update the link count and finish removing the file?...  I
> >> 	    just noticed that ufs_inactive() handles some of this.
> >> 	  - it calls softdep_flushfiles() after doing the sync.  This
> >> 	    doesn't seem to touch the inode.
> >> 	  - apparently, softdep_flushfiles() fails in -current, while in
> >> 	    ~5.2 it bogusly succeeds and then code just after it is called
> >> 	    detects a problem but doesn't handle it.
> >>=20
> >> >didn't pick up on the need for the second line (else if (DOINGASYNC(d=
vp))=20
> >> >{) though.  It's a default mount, so I don't understand how that will=
=20
> >> >help, i.e. it won't be an async mount, right?
> >>=20
> >> Ignore that.  It is for async mounts, to make them unconditionally asy=
nc.
> >>=20
> >> >One more point to address Julian's question, the partition is not mou=
nted=20
> >> >with soft updates.
> >>=20
> >> Interesting.  I saw no sign of the problem without soft updates except=
 a
> >> panic later after enabling soft updates.  I was running fsck a lot but
> >> may have forgotten one since no error was detected.  The problem should
> >> be easier to understand if it affects non-soft-updates.
> >>=20
> >> [Context lost to top posting]
> >>=20
> >=20
> > 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() ?
>=20
> 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 ufsm=
ount *,
 		    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 =3D ffs_valloc;
 	ump->um_vfree =3D ffs_vfree;
 	ump->um_ifree =3D ffs_ifree;
+	ump->um_isronly =3D 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)
@@ -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 =3D ump->um_fs;
+
+	return (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 =3D=3D 0 && DOINGSOFTDEP(vp)) ||
-	    (ip->i_nlink <=3D 0 &&
-	     (vp->v_mount->mnt_flag & MNT_RDONLY) =3D=3D 0)) {
+	    (ip->i_nlink <=3D 0 && !UFS_ISRONLY(ip->i_ump))) {
 	loop:
 		if (vn_start_secondary_write(vp, &mp, V_NOWAIT) !=3D 0) {
 			/* Cannot delete file while file system is suspended */
@@ -121,7 +120,7 @@ ufs_inactive(ap)
 	}
 	if (ip->i_effnlink =3D=3D 0 && DOINGSOFTDEP(vp))
 		softdep_releasefile(ip);
-	if (ip->i_nlink <=3D 0 && (vp->v_mount->mnt_flag & MNT_RDONLY) =3D=3D 0) {
+	if (ip->i_nlink <=3D 0 && !UFS_ISRONLY(ip->i_ump)) {
 #ifdef QUOTA
 		if (!getinoquota(ip))
 			(void)chkiq(ip, -1, NOCRED, FORCE);
diff --git a/sys/ufs/ufs/ufsmount.h b/sys/ufs/ufs/ufsmount.h
index 80fe3fa..57e7dd0 100644
--- a/sys/ufs/ufs/ufsmount.h
+++ b/sys/ufs/ufs/ufsmount.h
@@ -93,6 +93,7 @@ struct ufsmount {
 	int	(*um_valloc)(struct vnode *, int, struct ucred *, struct vnode **);
 	int	(*um_vfree)(struct vnode *, ino_t, int);
 	void	(*um_ifree)(struct ufsmount *, struct inode *);
+	int	(*um_isronly)(struct ufsmount *);
 };
=20
 #define UFS_BALLOC(aa, bb, cc, dd, ee, ff) VFSTOUFS((aa)->v_mount)->um_bal=
loc(aa, bb, cc, dd, ee, ff)
@@ -102,6 +103,7 @@ struct ufsmount {
 #define UFS_VALLOC(aa, bb, cc, dd) VFSTOUFS((aa)->v_mount)->um_valloc(aa, =
bb, cc, dd)
 #define UFS_VFREE(aa, bb, cc) VFSTOUFS((aa)->v_mount)->um_vfree(aa, bb, cc)
 #define UFS_IFREE(aa, bb) ((aa)->um_ifree(aa, bb))
+#define	UFS_ISRONLY(aa) ((aa)->um_isronly(aa))
=20
 #define	UFS_LOCK(aa)	mtx_lock(&(aa)->um_lock)
 #define	UFS_UNLOCK(aa)	mtx_unlock(&(aa)->um_lock)

--f4HxWLVbzokH9yio
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (FreeBSD)

iD8DBQFHURYtC3+MBN1Mb4gRAuz7AJ402T07oC29X4x7cl3OPoh/EylrqQCcDvoK
82kvkH2TYZEMHmlkfIoZ8Gc=
=r8eH
-----END PGP SIGNATURE-----

--f4HxWLVbzokH9yio--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20071201080709.GO83121>