Date: Wed, 5 Mar 2008 15:20:29 +0300 From: Yar Tikhiy <yar@comp.chem.msu.su> To: Craig Rodrigues <rodrigc@FreeBSD.org> Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sbin/fsck_ffs main.c Message-ID: <20080305122029.GA7027@dg.local> In-Reply-To: <200803050825.m258Ppv2016738@repoman.freebsd.org> References: <200803050825.m258Ppv2016738@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Mar 05, 2008 at 08:25:49AM +0000, Craig Rodrigues wrote: > rodrigc 2008-03-05 08:25:49 UTC > > FreeBSD src repository > > Modified files: > sbin/fsck_ffs main.c > Log: > For a mounted file system which is read-only, when > doing the MNT_RELOAD, pass in "ro" and "update" > string mount options to nmount() instead of MNT_RDONLY and MNT_UPDATE flags. > > Due to the complexity of the mount parsing code especially > with respect to the root file system, passing in MNT_RDONLY and MNT_UPDATE > flags would do weird things and would cause fsck to convert the root > file system from a read-only mount to read-write. Your analysis of the problem sounds not quite correct to me. In fact, the weird things happen because the "ro" string option isn't set on the root mount from the outset. After your patch, fsck just adds the missing "ro" to the root mount options `when it becomes safe to do so' -- the "ro" string option is dangerous to set on the root FS from the outset due to a bunch of bugs in the kernel code, as my experience with this issue showed to the public. The bugs are all the same: one piece of code passes bit flags and old-style mount() arguments while its counterpart wants string options or vice versa. Our current position in the middle of the way appears totally unacceptable as it leads to problems like this one. Either we should arrive at using string options exlusively, or abandon them at all. Also note that, were the string options implemented in full, there would be no need for fsck to pass "ro" along with "update" to preserve read-only status: existing string options go away from the mount point only if explicitly turned off through a "no" prefix, as in "noro". This is an evident design point of string options, and it's implemented already (modulo bugs in vfs_mergeopts().) Finally, it can be argued that "update" actually belongs to nmount() flags, not to string options, because essentially it is not a persistent mount option, it's a modifier for the current operation requested by this nmount() call. But this is a topic for a separate discussion on the desired nmount(2) semantics. I won't restart our back-out war, but please clearly mark your change as a temporary workaround so that it doesn't mislead people, including us when we revisit the code. Adding "ro" to the root mount options lately is an obvious hack, and it isn't a job for fsck at all. It'll be safe to set the "ro" string option on the root mount from the beginning as soon as all the root-capable filesystems and their mount_foo tools are converted to using string options properly. A crucial part of the task is your work on nfs_mount(), which I fully support. Thanks! > To test: > - boot into single user mode > - show mounted file systems with: mount > - root file system should be mounted read-only > - fsck / > - show mounted file systems with: mount > - root file system should still be mounted read-only > > PR: 120319 > MFC after: 1 month > Reported by: yar > > Revision Changes Path > 1.49 +3 -1 src/sbin/fsck_ffs/main.c -- Yar
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080305122029.GA7027>