Sorry, wrong mail domain :)
FrankH.
---------- Forwarded message ----------
Date: Fri, 28 Sep 2007 14:21:54 +0100 (BST)
From: Frank Hofmann <frank.hofm...@sun.com>
To: Moinak Ghosh <Moinak.Ghosh at Sun.COM>
Cc: hsfs-iteam at Sun.COM, caiman-discuss at Sun.COM
Subject: Re: Final webrevs updated for HSFS readahead changes
Hi Moinak,
to me, the code looks great now. Except for one thing:
As far as the kstats go, what's the system doing on multiple kstats being
created with the same name ? You're doing the kstat_create once per mount, but
re-use the same name/ID fields. I think that might cause memory leaks; has the
hsfs kstat code been tested with 2+ concurrently mounted/accessed HSFS
filesystems ?
If you take e.g. UFS as a precedence (inode / logging / directio kstats), all
of these are created at module initialization, not at mount. Same for the ZFS
arc kstats.
There's other precedence (fssnap) for per-mountpoint kstats, but if you do that
you'd have to:
- make sure you pass unique names / IDs
- delete previously-existing kstats of the same name / ID
when you call kstat_create(). It's more complicated and there's the possibility
for errors.
The kstats are nice to have, right there; but if it gets too complicated to
make them per-mountpoint then either make them global, or simply purge the
kstats, get a few more SDT probes in and let DTrace do the sum() instead of the
kstat framework. It's probably not worth spending another week on this, it's a
side aspect of the change.
Thanks for the great work on this !
FrankH.
On Thu, 20 Sep 2007, Moinak Ghosh wrote:
> I have completed all the updates after the last review comments and completed
> all my testing. I have done manual tests and Filebench tests on SPARC and
> x86. The Filebench tests were also repeated to kmem_flags set to 0x1f and
> checked for correctness and leaks. The fsfuzzer test was run for 10days
> without
> issues. The bug is updated.
>
> The updated webrevs are available at:
> http://www.genunix.org/distributions/belenix_site//binfiles/hsfswebrev/index.htm
>
> Updated binaries available from:
> http://www.genunix.org/distributions/belenix_site//binfiles/hsfs.amd64
> http://www.genunix.org/distributions/belenix_site//binfiles/hsfs.sparcv9
>
> This update adds the following changes:
>
> - Incorporate all the review comments.
> - Add kstats and SDT probes for debugging purposes. You can copy the
> updated module, mount a DVD and see:
> kstat -m hsfs_fs
> dtrace -l -P sdt | grep hsfs
> - SPARC testing was done on a T2000 and it threw up a surprise perf
> issue for the single-stream sequential read and random read tests. It was
> found using standard kmem_alloc for thousands of small allocations of
> size 96 bytes was inefficient. So a kmem_cache was used and it made
> a dramatic difference.
> - Removed one unnecessary page_lookup in the readahead path.
>
> As it stands now there is an across the board improvement on all Filebench
> tests including the random read tests on both x86 and SPARC.
>
> Please send your replies to caiman-discuss where this email was posted as
> well.
>
> Regards,
> Moinak.
>
