From owner-freebsd-hackers@FreeBSD.ORG Tue Jan 25 17:23:45 2011 Return-Path: Delivered-To: freebsd-hackers@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C7D751065672 for ; Tue, 25 Jan 2011 17:23:45 +0000 (UTC) (envelope-from jh@FreeBSD.org) Received: from gw02.mail.saunalahti.fi (gw02.mail.saunalahti.fi [195.197.172.116]) by mx1.freebsd.org (Postfix) with ESMTP id 4E8358FC08 for ; Tue, 25 Jan 2011 17:23:45 +0000 (UTC) Received: from a91-153-123-205.elisa-laajakaista.fi (a91-153-123-205.elisa-laajakaista.fi [91.153.123.205]) by gw02.mail.saunalahti.fi (Postfix) with SMTP id 6863D13995B; Tue, 25 Jan 2011 19:23:40 +0200 (EET) Date: Tue, 25 Jan 2011 19:23:40 +0200 From: Jaakko Heinonen To: Craig Rodrigues Message-ID: <20110125172340.GA1535@a91-153-123-205.elisa-laajakaista.fi> References: <20110114122454.GA4805@jh> <20110119083407.GA51372@crodrigues.org> <20110119151210.GA2182@a91-153-123-205.elisa-laajakaista.fi> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20110119151210.GA2182@a91-153-123-205.elisa-laajakaista.fi> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-hackers@FreeBSD.org Subject: Re: [patch] nmount ro, rw and negated option handling X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Jan 2011 17:23:45 -0000 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