From owner-freebsd-fs@freebsd.org Tue May 17 20:39:56 2016 Return-Path: Delivered-To: freebsd-fs@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7D24CB3FAB6 for ; Tue, 17 May 2016 20:39:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id 6B4E214EE for ; Tue, 17 May 2016 20:39:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: by mailman.ysv.freebsd.org (Postfix) id 6A8DDB3FAB5; Tue, 17 May 2016 20:39:56 +0000 (UTC) Delivered-To: fs@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 6A310B3FAB3 for ; Tue, 17 May 2016 20:39:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id 37A8314ED for ; Tue, 17 May 2016 20:39:55 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id AE2BA428F79; Wed, 18 May 2016 06:39:52 +1000 (AEST) Date: Wed, 18 May 2016 06:39:49 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov cc: fs@freebsd.org Subject: Re: fix for per-mount i/o counting in ffs In-Reply-To: <20160517084241.GY89104@kib.kiev.ua> Message-ID: <20160518061040.D5948@besplex.bde.org> References: <20160517072104.I2137@besplex.bde.org> <20160517084241.GY89104@kib.kiev.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=EfU1O6SC c=1 sm=1 tr=0 a=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=QJha7pIZtpZdJjQaBnUA:9 a=CjuIK1q_8ugA:10 X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 May 2016 20:39:56 -0000 On Tue, 17 May 2016, Konstantin Belousov wrote: > On Tue, May 17, 2016 at 07:26:08AM +1000, Bruce Evans wrote: >> Counting of i/o's in g_vfs_strategy() requires the fs to initialize >> devvp->v_rdev->si_mountpt to non-null. This seems to be done correctly >> in ext2fs and msdosfs, but in ffs it is not done for ro mounts, or for >> rw mounts that started as ro. The bug is most obvious for the root >> file system since it always starts as ro. > > I committed the comments updates. > > For the accounting patch, don't we want to account for all io, including > the mount-time metadata reads and initial superblock update ? > > diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c > index 9776554..712fc21 100644 > --- a/sys/ufs/ffs/ffs_vfsops.c > +++ b/sys/ufs/ffs/ffs_vfsops.c > @@ -780,6 +780,8 @@ ffs_mountfs(devvp, mp, td) > mp->mnt_iosize_max = MAXPHYS; > > devvp->v_bufobj.bo_ops = &ffs_ops; > + if (devvp->v_type == VCHR) > + devvp->v_rdev->si_mountpt = mp; > > fs = NULL; > sblockloc = 0; > @@ -1049,8 +1051,6 @@ ffs_mountfs(devvp, mp, td) > ffs_flushfiles(mp, FORCECLOSE, td); > goto out; > } > - if (devvp->v_type == VCHR && devvp->v_rdev != NULL) > - devvp->v_rdev->si_mountpt = mp; > if (fs->fs_snapinum[0] != 0) > ffs_snapshot_mount(mp); > fs->fs_fmod = 1; > @@ -1083,6 +1083,8 @@ ffs_mountfs(devvp, mp, td) > out: > if (bp) > brelse(bp); > + if (devvp->v_type == VCHR && devvp->v_rdev != NULL) > + devvp->v_rdev->si_mountpt = NULL; > if (cp != NULL) { > DROP_GIANT(); > g_topology_lock(); Yes, that looks better. The other file systems that support the counters (ext2fs and msdosfs) need a similar change. Grepping for si_mountpoint shows no other file systems that support this. The recently axed reiserfs sets si_mountpt, but only if si_mountpt is #defined. This only works in old versions: - in old versions, si_mountpt is #defined. GEOM broke this, and the #define was removed. The ifdef kept reiserfs compiling. History for reiserfs was broken by repo-copying after the ifdefed was added. - mckusick fixed the counting for ffs and restored si_mountpt, but it is now not #define'd. The following file systems are something like ffs so they should set si_mountpt, but don't: cd9660, fuse, nandfs (?), udf, zfs. I only understand cd9660 and udf. The following file systems used to set si_mountpt but now don't: hpfs (axed), ntfs (axed), udf (but not cd9660). Counters for the ro file systems are only moderately useful. They tell you that the block the block size is too small and/or if the clustering is bad. No version seems to be as careful as the above -- they don't set si_mountpt until near the end of a sucessful mount. This takes just 1 statement in mount() and 1 in umount(). I'd like vfs to do this setting so that leaf file systems can't forget to do it, but never figured out the plumbing to tell upper layers of vfs about devvp. g_vfs_open() can't do it since it knows devvp but not mp. Bruce