Date: Wed, 18 May 2016 06:39:49 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: fs@freebsd.org Subject: Re: fix for per-mount i/o counting in ffs Message-ID: <20160518061040.D5948@besplex.bde.org> In-Reply-To: <20160517084241.GY89104@kib.kiev.ua> References: <20160517072104.I2137@besplex.bde.org> <20160517084241.GY89104@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160518061040.D5948>