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

[Life is hard, and then you die]

On Thu, Jul 08, 2010 at 02:56:30PM +0200, Ramshankar wrote:
> I've committed your patches after reviewing them and will be part of the
> next 3.2.x release. Thank you for the effort and time dedicated to
> improving the sharedfolders.

Thanks for applying them so quickly!

With these you can close tickets 4154 and 6512.


  Cheers,

  Ronald


> On Tue, 2010-06-29 at 10:16 +0200, Ramshankar wrote:
> > Thanks for the info on how you benchmarked these. I'll continue applying
> > the patches.
> > 
> > Regards,
> > Ram.
> > 
> > On Sat, 2010-06-26 at 23:59 -0700, Life is hard, and then you die wrote:
> > > 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
> > > > >