[PATCHES] Issue fsync for SMB2 FLUSH asynchronously

[Volker Lendecke]

On Thu, Nov 12, 2015 at 02:59:23PM -0800, Jeremy Allison wrote:
> On Thu, Nov 12, 2015 at 02:41:36PM -0700, Christof Schmitt wrote:
> > On Thu, Nov 12, 2015 at 11:56:47AM -0800, Jeremy Allison wrote:
> > > On Wed, Nov 11, 2015 at 06:05:57PM -0800, Jeremy Allison wrote:
> > > > > TEST SMB2-BASIC FAILED!
> > > > > 
> > > > > Let me look at this some more..
> > > > 
> > > > Found the bug. It's my fault of old..
> > > > 
> > > > There is a *HIDEOUS* global variable that counts
> > > > the number of outstanding aio calls:
> > > > 
> > > > extern int outstanding_aio_calls;
> > > > 
> > > > and when source3/modules/vfs_default.c
> > > > wants to read the results of an async
> > > > call in vfswrap_asys_finished() it
> > > > uses a variable length array defined
> > > > as:
> > > > 
> > > > struct asys_result results[outstanding_aio_calls];
> > > > 
> > > > ret = asys_results(asys_ctx, results, outstanding_aio_calls);
> > > > 
> > > > to work out how many to read. The vfswrap_fsync_send()
> > > > doesn't increment outstanding_aio_calls so when
> > > > the fflush finishes it calls asys_results(asys_ctx, results, 0);
> > > > which doesn't work too well :-).
> > > > 
> > > > I need to ensure outstanding_aio_calls is kept up
> > > > to date everywhere.
> > > > 
> > > > Going to clock off now - I'll post a fixed
> > > > patch (that passes make test) tomorrow.
> > > > 
> > > > Sorry for the problem !
> > > 
> > > OK, here is the revised and working patch.
> > > 
> > > I removed the global access to the problematic
> > > variables, created wrapper functions for them, and
> > > ensure that the number of outstanding
> > > aio calls is incremented and decremented
> > > correctly on the async fsync calls.
> > > 
> > > Passes local make test ! Please review and
> > > push if happy.
> > 
> > Looks good, pushed to autobuild.
> > 
> > Thanks for the patches, it would have taken me some time to figure out
> > the details.
> 
> No problem, thanks for the review !
> 
> After some private conversations with Volker,
> here is a follow-up patch that will apply
> on top of the previous one and removes one
> of the global varables completely.
> 
> Volker pointed out that we need to rethink
> the aio_pending_size limit on outstanding
> aio requests.
> 
> What happens when we reach this limit is
> that we start processing SMB requests
> synchronously, which slows down the system
> further and we never get to see the replies
> from the asys_results write to the internal
> pipe as we're too busy reading and processing
> incoming SMB requests.
> 
> So here is a patchset that does a few
> things on top of the one you pushed !
> 
> a). Removes --with-aio-support. We no longer
> need to run on systems that don't have
> pthread support but use POSIX-RT aio_read()
> aio_write() and friends. These days we
> require pthreads.
> 
> This has the advantage that it removes
> vfs_aio_posix as it's no longer used
> (and indeed it was never even documented !).
> 
> Plus is removes a boatload of code and
> options processing (as we no longer have
> to check for POSIX-RT io calls).
> 
> b). Removes the fallback to processing SMB requests
> synchronously which as Volker pointed out makes
> the problem worse. Instead always put them onto
> the internal pthreadpool queue, which will
> dispatch them as a thread becomes free.
> 
> c). Change the internal aio_pending_size static variable
> which is hard-coded at compile time to 100 to a
> proper smb.conf global variable "aio max threads"
> which is set to 100 by default but is now a
> system-tunable.
> 
> It passes make test.

Reviewed-by: Me.

I have only one minor question: Should we explain in the
manpage entry that the parameter is per smbd, not global?

Thanks!

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de