From owner-cvs-src@FreeBSD.ORG Mon Mar 10 13:43:59 2008 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2C3A41065676 for ; Mon, 10 Mar 2008 13:43:59 +0000 (UTC) (envelope-from yar.tikhiy@gmail.com) Received: from fg-out-1718.google.com (fg-out-1718.google.com [72.14.220.156]) by mx1.freebsd.org (Postfix) with ESMTP id 636F78FC1B for ; Mon, 10 Mar 2008 13:43:58 +0000 (UTC) (envelope-from yar.tikhiy@gmail.com) Received: by fg-out-1718.google.com with SMTP id 16so1638169fgg.35 for ; Mon, 10 Mar 2008 06:43:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:date:from:to:cc:subject:message-id:references:mime-version:content-type:content-disposition:in-reply-to:user-agent:sender; bh=BN6mBuGQ/Sn3sCKEHuui5KdSBg7dN+vOJV9Xw+BBDSI=; b=vS+8RJJB3CtLyINZi9CWmZbODLIV6uV2MFu0WG9X3NxdY0fmx9tEbwW6S3WRtNEeIzNSLu5GeOLPH48jzJiz/SaPdSiy0MtFkr3ettQosJs8Vbs+P9bkaDQtfCohy887SIIDP9odvNlCCa8JzyFrhU+GHnOJUS8QY5izN8w2kOM= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version:content-type:content-disposition:in-reply-to:user-agent:sender; b=pWd3/SHsayjVV+4RMs8TCB+WYgHWoVQbws+M49zzYEIoGkNGhFAiFiggF++vkfjNM5o5yDGEEmaAXvid7PQ8TXrtsVWZ9ONwXF5/dr246akkF9e0k7Jkod8+A6D+AJREQzgRcLQXvUo41r6uO8K3z+i2GeYKsn83on5s8Joj+QI= Received: by 10.86.98.18 with SMTP id v18mr1832070fgb.52.1205156636074; Mon, 10 Mar 2008 06:43:56 -0700 (PDT) Received: from dg.local ( [83.237.56.23]) by mx.google.com with ESMTPS id 4sm8126144fgg.4.2008.03.10.06.43.53 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 10 Mar 2008 06:43:54 -0700 (PDT) Date: Mon, 10 Mar 2008 16:44:10 +0300 From: Yar Tikhiy To: Craig Rodrigues Message-ID: <20080310134410.GA47195@dg.local> References: <200803050825.m258Ppv2016738@repoman.freebsd.org> <20080305122029.GA7027@dg.local> <20080305222402.GA80407@crodrigues.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080305222402.GA80407@crodrigues.org> User-Agent: Mutt/1.5.17 (2007-11-01) Sender: Yar Tikhiy Cc: Craig Rodrigues , cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sbin/fsck_ffs main.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Mar 2008 13:43:59 -0000 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); } /*