[PATCHES] Issue fsync for SMB2 FLUSH asynchronously

Michael Adam obnox at samba.org
Fri Nov 13 14:55:26 UTC 2015


On 2015-11-13 at 15:38 +0100, Volker Lendecke wrote:
> 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?

Sounds like a good idea.

Like this?

s/threads Samba will create/threads one smbd process will create/

Great cleanup work!
Reviewed-by me as well.

Cheers - Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20151113/a3df891d/signature.sig>


More information about the samba-technical mailing list