[vbox-dev] [PATCH] fixes and improvements to solaris (guest) shared folders

[Life is hard, and then you die]

sorry for the slow reply.

On Fri, Jun 25, 2010 at 03:37:52PM +0200, Ramshankar wrote:
> I've integrated upto the readdir coalesing patch (07) and I've been
> doing some testing with each of the patches.
> 
> With patch 7, with readdir coalesing, I don't seem to achieve as much of
> a performance gain seems to marginal. How exactly did you test this
> since you claimed a much higher boost in performance?

I presume you mean the 10-20 times speedup? That's for the
sfprov_readdir() call alone. If you measure an 'ls' you will see only
a factor 2 or so overall improvement because there are other functions
being called that take quite some time.

To measure these things I find dtrace the most handy tool. E.g. the
following will run an ls and print the total for the sfprov_readdir()
calls into the 'dtrace.out' file (dtrace appends to the file, so you
can just run this several times):

  pfexec dtrace -q \
    -i 'BEGIN { tot = 0; }' \
    -n 'fbt::sfprov_readdir:entry  { self->start = timestamp; }' \
    -n 'fbt::sfprov_readdir:return { tot += (timestamp - self->start) / 1000; }' \
    -i 'END { printf("total: %d us", tot); }' \
    -o dtrace.out -c 'ls mnt/foobar' > /dev/null

Here are some sample totals from a directory with 5000 files
(OpenSolaris 2009.06 guest, Linux 2.6.31/ext4 host):

  total: 373372 us
  total: 307280 us
  total: 330815 us
  total: 332105 us
  
  total: 19609 us
  total: 33160 us
  total: 22638 us
  total: 37024 us

the first batch is up to patch 06; the second batch is with patch 07;
in both cases I first reloaded (umount+modunload+mount) the vboxfs
kernel module (to clear out caches etc on the guest) and then ran the
above dtrace 4 times. YMMV of course. If you take the fastest times
(which I think makes the most sense, given that we're trying to look
at the performance the various function calls and not of the disks and
system as a whole) then the above number indicate a factor of over 15
speedup.

If you replace the sfprov_readdir with sffs_readdir in the above
dtrace then you already see a slightly smaller speedup; here is a
modified dtrace command that in addition to the total time spent
in the sffs_readdir() calls will also dump a distribution of the
times of the individual calls:

  pfexec dtrace -q \
    -i 'BEGIN { tot = 0; }' \
    -n 'fbt::sffs_readdir:entry  { self->start = timestamp; }' \
    -n 'fbt::sffs_readdir:return { d = (timestamp - self->start) / 1000; @times[probefunc] = quantize(d); tot += d; }' \
    -i 'END { printa("%s: %@d\n", @times); printf("total: %d us", tot); }' \
    -o dtrace.out -c 'ls mnt/foobar' > /dev/null

A similar sample run like above gave me:

  total: 1251834 us
  total: 318061 us
  total: 332139 us
  total: 258201 us

  total: 350480 us
  total: 45477 us
  total: 25959 us
  total: 24327 us

To get an overview of the time spent in all the various functions in
the vboxfs module for an 'ls', try this (the printed out times are
again in microseconds):

  pfexec dtrace -q \
    -n 'fbt:vboxfs::entry  { self->start[probefunc] = timestamp; }' \
    -n 'fbt:vboxfs::return { d = (timestamp - self->start[probefunc]) / 1000; @times[probefunc] = quantize(d); @totals[probefunc] = sum(d); }' \
    -o dtrace.out -c 'ls mnt/foobar' > /dev/null

And to understand a bit why I optimized the getattr calls, run the
above using 'ls -F' or 'ls -l'.

Hope this helps.


  Cheers,

  Ronald


> On Wed, 2010-06-23 at 17:50 -0700, Life is hard, and then you die wrote:
> > On Wed, Jun 23, 2010 at 02:23:57PM +0200, Ramshankar wrote:
> > > On Wed, 2010-06-23 at 04:51 -0700, Life is hard, and then you die wrote:
> > > > Ok, makes sense. I took a closer look at the Solaris coding standards
> > > > guide and realized I got a couple things wrong (mainly
> > > > line-continuations and switch statements); so I reworked the patches
> > > > to follows those better.
> > > > 
> > > > Also, while looking at the latest linux shared-folders changes today I
> > > > realized that I had fallen into the same trap with the readdir fix
> > > > (patch 06) as the linux code (http://www.virtualbox.org/ticket/5251),
> > > > i.e. it wasn't properly resetting the directory listing for apps doing
> > > > a seekdir. I've fixed that now and tested it, both using the 'svnadmin
> > > > load' mentioned in the above ticket as well as with a small test-suite
> > > > I wrote for this.
> > > > 
> > > > Attached therefore you'll find a new set of patches which include both
> > > > the style fixes and the readdir fix, but are otherwise identical to
> > > > the previous ones.
> > > > 
> > > > > I'll take a look at the patches you provided and get back to you
> > > > > regarding them. Thanks a lot for the effort and time on this.
> > > > 
> > > > Thanks.
> > > 
> > > Great! Thanks for the quick fixes. I've begun reviewing & committing
> > > your patches. Since these patches are significant, I'll mentioned your
> > > name as "Ronald" (no mail address included) as a contributor in the
> > > source files involved, I hope this is agreeable. If you don't want to be
> > > mentioned for some reason let me know.
> > 
> > Sounds good. Either way is fine with me.
> > 
> > > I will mail you here once I complete reviewing, committing and testing
> > > these patches.
> > 
> > Thanks - if you'd like me to rework/change anything, just let me know.
> > 
> > A few things I should mention:
> > 
> >  - with the 08_stat_coalesce.patch the sfprov_get_{a,m,c}time()
> >    and sfprov_get_mode() functions are not used anymore; I left them
> >    in as I wasn't sure what the full reasoning behind the original
> >    approach was, but they could be deleted now.
> > 
> >  - the sfprov_get_size() function could also easily be removed by
> >    replacing the last remaining call to it with an sfnode_get_stat()
> >    call. Again, I didn't do so because I wasn't completely sure if you
> >    were thinking of other providers and whether you wanted vboxfs_prov
> >    interface to be as fine-grained as possible or not.
> > 
> >  - lastly, I realize the stat-caching may be controversial (patch 09).
> >    Maybe defaulting it to off (DEF_STAT_TTL_MS=0) may make people more
> >    comfortable.
> > 
> > 
> >   Cheers,
> > 
> >   Ronald
> >