[caiman-discuss] Final webrevs updated for HSFS readahead changes

[Frank Hofmann]

Sorry, wrong mail domain :)
FrankH.


---------- Forwarded message ----------
Date: Fri, 28 Sep 2007 14:21:54 +0100 (BST)
From: Frank Hofmann <Frank.Hofmann at 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.
>