Date: Mon, 10 Mar 2008 16:44:10 +0300 From: Yar Tikhiy <yar@comp.chem.msu.su> To: Craig Rodrigues <rodrigc@crodrigues.org> Cc: Craig Rodrigues <rodrigc@FreeBSD.org>, cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sbin/fsck_ffs main.c Message-ID: <20080310134410.GA47195@dg.local> In-Reply-To: <20080305222402.GA80407@crodrigues.org> References: <200803050825.m258Ppv2016738@repoman.freebsd.org> <20080305122029.GA7027@dg.local> <20080305222402.GA80407@crodrigues.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Mar 05, 2008 at 10:24:02PM +0000, Craig Rodrigues wrote:
> On Wed, Mar 05, 2008 at 03:20:29PM +0300, Yar Tikhiy wrote:
> > Your analysis of the problem sounds not quite correct to me.
>
> You make some interesting points in your e-mail.
> I suggest that you summarize the points and
> post them to arch@ for further review and discussion.
My points are directly related to your commit. To illustrate them
clearlier, I instrumented vfs_mount.c with the patch shown in the
attachment and investigated how fsck_ffs now affects the root mount
options. My findings agree with my prognosis; they are as follows.
When fsck_ffs just invokes nmount(2), the initial mount options on
the root are:
fstype, fspath, from.
Just before nmount() returns, the mount options on the root become:
fstype, from, fspath, errmsg, update, ro.
Prior to your commit the final options were:
fstype, from, fspath, errmsg.
Thus, due to your commit, fsck_ffs now sets two additional string
options on the root mount, "ro" and "update". Both are permanent,
i.e., they remain there after fsck_ffs exited. I maintain that:
1) Setting "ro" from fsck_ffs is but a hack needed because the
option cannot be set on the root mount at boot time due
to bugs. The hack is tolerable, but you failed to mark it
clearly as a temporary workaround, e.g., using a reasonable
XXX-style comment providing enough details.
2) Setting "update" as a _permanent_ option on the mount point is
just wrong because "update" isn't one. The nmount(2) code cannot
cope with "update" as a string option yet; it still needs to be
spelled MNT_UPDATE. And, to the best of my knowledge, there is
no problem with still using that bit flag, although you hint at
some `weird things'. This part of the change should be reverted.
(Another approach could be to fix vfs_donmount(), but that would
require the architectural decision be made on whether nmount(2)
modifiers are spelled as string options or bit flags. Currently
no FS code tries to check for "update", and "reload" isn't there
yet, so the issue is still pending. Feel free to conduct a thread
on -arch.)
Now I expect you either prove my points wrong using technical facts,
or react to them by the respective commit(s) to fsck_ffs. Otherwise
I'll be forced to request fsck_ffs/main.c rev. 1.49 be backed out
and PR 120319 be reopened.
Thanks!
--
Yar
P.S. Here's how I instrumented vfs_mount.c:
--- vfs_mount.c.orig 2008-02-19 20:04:36.000000000 +0300
+++ vfs_mount.c 2008-03-10 12:29:30.000000000 +0300
@@ -324,6 +324,19 @@
return (error);
}
+static void
+vfs_printopts(struct vfsoptlist *opts)
+{
+ struct vfsopt *opt;
+ int first = 1;
+
+ TAILQ_FOREACH(opt, opts, link) {
+ printf("%s%s", first ? "" : " ", opt->name);
+ if (first)
+ first = 0;
+ }
+}
+
/*
* Merge the old mount options with the new ones passed
* in the MNT_UPDATE case.
@@ -948,7 +961,16 @@
MNT_IUNLOCK(mp);
VOP_UNLOCK(vp, 0);
mp->mnt_optnew = fsdata;
+ printf(">>> opts before: ");
+ vfs_printopts(mp->mnt_opt);
+ printf("\n");
+ printf(">>> opts new: ");
+ vfs_printopts(mp->mnt_optnew);
+ printf("\n");
vfs_mergeopts(mp->mnt_optnew, mp->mnt_opt);
+ printf(">>> opts merged: ");
+ vfs_printopts(mp->mnt_optnew);
+ printf("\n");
} else {
/*
* If the user is not root, ensure that they own the directory
@@ -1027,6 +1049,9 @@
if (mp->mnt_opt != NULL)
vfs_freeopts(mp->mnt_opt);
mp->mnt_opt = mp->mnt_optnew;
+ printf(">>> opts final: ");
+ vfs_printopts(mp->mnt_opt);
+ printf("\n");
(void)VFS_STATFS(mp, &mp->mnt_stat, td);
}
/*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080310134410.GA47195>