Date: Tue, 25 Jan 2011 19:23:40 +0200 From: Jaakko Heinonen <jh@FreeBSD.org> To: Craig Rodrigues <rodrigc@crodrigues.org> Cc: freebsd-hackers@FreeBSD.org Subject: Re: [patch] nmount ro, rw and negated option handling Message-ID: <20110125172340.GA1535@a91-153-123-205.elisa-laajakaista.fi> In-Reply-To: <20110119151210.GA2182@a91-153-123-205.elisa-laajakaista.fi> References: <20110114122454.GA4805@jh> <20110119083407.GA51372@crodrigues.org> <20110119151210.GA2182@a91-153-123-205.elisa-laajakaista.fi>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2011-01-19, Jaakko Heinonen wrote:
> On 2011-01-19, Craig Rodrigues wrote:
> > I disagree with your patch and do not approve it.
>
> > > 1. Have mountd(8) running.
> > > 2. # mdconfig -a -t vnode -f ufsimg
> > > 3. # mount -o ro,rw /dev/md0 /mnt
>
> With your patch[1] after the third step the mount point has both "ro"
> and and "noro" active but the MNT_RDONLY flag is not set. Again, you
> will eventually get the "ffs_sync: rofs mod" (or similar) panic because
> the "ro" option is active during remount.
Could you please elaborate on why my patch isn't acceptable and/or
suggest an alternative approach to fix the bugs. As I said earlier the
patch you posted doesn't solve the issues. I really want to get these
bugs fixed.
One option would be to revert the file system code to use the MNT_RDONLY
flag instead of checking for the "ro" string option until nmount gets
fixed but I don't think it's feasible because too many file systems are
already testing for "ro".
A quick fix for the particular problem above would be to commit the
vfs_equalopts() part of my patch. I believe that the change is correct
as the code stands now. It is not enough to fix PR kern/150206.
Another related bug is in vfs_domount_update(): if VFS_MOUNT() succeeds
but vfs_export() fails, old mount flags and options are restored. I
think this shouldn't happen when VFS_MOUNT() has been already
successfully completed and this is the final reason why FFS fs_ronly and
the MNT_RDONLY flag get out of sync. Does the patch below look sane?
%%%
Index: sys/kern/vfs_mount.c
===================================================================
--- sys/kern/vfs_mount.c (revision 217792)
+++ sys/kern/vfs_mount.c (working copy)
@@ -683,8 +683,6 @@ bail:
}
}
- if (error != 0)
- vfs_freeopts(optlist);
return (error);
}
@@ -800,6 +798,7 @@ vfs_domount_first(
}
if (error != 0) {
vput(vp);
+ vfs_freeopts(fsdata);
return (error);
}
VOP_UNLOCK(vp, 0);
@@ -824,6 +823,7 @@ vfs_domount_first(
vp->v_iflag &= ~VI_MOUNT;
VI_UNLOCK(vp);
vrele(vp);
+ vfs_freeopts(fsdata);
return (error);
}
@@ -881,15 +881,15 @@ vfs_domount_update(
struct oexport_args oexport;
struct export_args export;
struct mount *mp;
- int error, flag;
+ int error, export_error, flag;
mtx_assert(&Giant, MA_OWNED);
ASSERT_VOP_ELOCKED(vp, __func__);
KASSERT((fsflags & MNT_UPDATE) != 0, ("MNT_UPDATE should be here"));
if ((vp->v_vflag & VV_ROOT) == 0) {
- vput(vp);
- return (EINVAL);
+ error = EINVAL;
+ goto fail;
}
mp = vp->v_mount;
/*
@@ -898,28 +898,26 @@ vfs_domount_update(
*/
flag = mp->mnt_flag;
if ((fsflags & MNT_RELOAD) != 0 && (flag & MNT_RDONLY) == 0) {
- vput(vp);
- return (EOPNOTSUPP); /* Needs translation */
+ error = EOPNOTSUPP; /* Needs translation */
+ goto fail;
}
/*
* Only privileged root, or (if MNT_USER is set) the user that
* did the original mount is permitted to update it.
*/
error = vfs_suser(mp, td);
- if (error != 0) {
- vput(vp);
- return (error);
- }
+ if (error != 0)
+ goto fail;
if (vfs_busy(mp, MBF_NOWAIT)) {
- vput(vp);
- return (EBUSY);
+ error = EBUSY;
+ goto fail;
}
VI_LOCK(vp);
if ((vp->v_iflag & VI_MOUNT) != 0 || vp->v_mountedhere != NULL) {
VI_UNLOCK(vp);
vfs_unbusy(mp);
- vput(vp);
- return (EBUSY);
+ error = EBUSY;
+ goto fail;
}
vp->v_iflag |= VI_MOUNT;
VI_UNLOCK(vp);
@@ -942,11 +940,12 @@ vfs_domount_update(
*/
error = VFS_MOUNT(mp);
+ export_error = 0;
if (error == 0) {
/* Process the export option. */
if (vfs_copyopt(mp->mnt_optnew, "export", &export,
sizeof(export)) == 0) {
- error = vfs_export(mp, &export);
+ export_error = vfs_export(mp, &export);
} else if (vfs_copyopt(mp->mnt_optnew, "export", &oexport,
sizeof(oexport)) == 0) {
export.ex_flags = oexport.ex_flags;
@@ -958,7 +957,7 @@ vfs_domount_update(
export.ex_masklen = oexport.ex_masklen;
export.ex_indexfile = oexport.ex_indexfile;
export.ex_numsecflavors = 0;
- error = vfs_export(mp, &export);
+ export_error = vfs_export(mp, &export);
}
}
@@ -1005,6 +1004,11 @@ end:
vp->v_iflag &= ~VI_MOUNT;
VI_UNLOCK(vp);
vrele(vp);
+ return (error != 0 ? error : export_error);
+
+fail:
+ vput(vp);
+ vfs_freeopts(fsdata);
return (error);
}
%%%
--
Jaakko
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110125172340.GA1535>