Quantcast

[PATCH] SMB2_FIND improvement for clustered environments

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
Hi!

Attached is a patchset that improves SMB2_FIND performance in a ctdb cluster by
making records fetching from locking.tdb asynchronous.

This has three parts:

- add a seperate IPC channel to ctdb for asynchrous processing

- new async dbwrap API: dbwrap_parse_record_send|recv()

- making use of it in smbd_smb2_query_directory_send()

Some numbers follow.

The time it takes to enumerate a directory with 50k files, current code:

$ time bin/smbclient -U FAST\\slow%x -m smb3 //localhost/share -c "ls dir\50kfiles\*" > /dev/null | grep real

3 runs:
real    0m10.693s
real    0m10.608s
real    0m10.710s

Using the new async API this becomes:

4 runs:
real    0m3.903s
real    0m2.809s
real    0m3.869s
real    0m3.894s

Some more implementation details shamelessly copied from the commit messages:

---8<---
dbwrap: add dbwrap_parse_record_send/recv

The req_state parameter tells the caller whether the async request is
blocked in a full send queue:

req_state >= DBWRAP_REQ_DISPATCHED := request is dispatched
req_state < DBWRAP_REQ_DISPATCHED := send queue is full

This is useful in a clustered Samba environment where the async dbwrap
request is sent over a socket to the local ctdbd.

If the send queue is full and the caller was issuing multiple async
dbwrap requests in a loop, the caller knows it's probably time to stop
sending requests for now and try again later.

This will be used in subsequent commits in
smbd_smb2_query_directory_send() when implementing async write time
updates. Directories may contain umpteen files so we send many requests
to ctdb without going through tevent and reading the responses which has
the potential to deadlock.
---8<---

---8<---
s3/smbd: make write time fetching async

Finally use the new async dbwrap_parse_record_send/recv() functions
respectively the fetch_share_mode_send/recv wrappers for fetching the
write time from locking.tdb.

Previously for a directory with n files we would sit idle in the
directory enumeration loop fo n * m seconds waiting for responses from
ctdb, where m is the response time in seconds for a dbwrap request via
ctbd.

This is known to kill performance and we even have a parameter
"smbd:search ask sharemode" that can be used to disable fetching the
write time from locking.tdb.

Using fetch_write_time_send() works this way: in the directory
enumeration loop that calls smbd_dirptr_lanman2_entry() to marshall the
directory entries we

1. call fetch_write_time_send() after calling smbd_dirptr_lanman2_entry
   passing a pointer to the current position in the marshall buffer.

2. If fetch_write_time_send() has set the out parameter "stop", we exit
   the enumeration loop. This is necessary because we only send dbwrap
   requests but don't consume the results. This has the potential to
   deadlock so we must stop sending requests as soon as our ctdb send
   queue is full.

3. In the fetch_write_time_done() callback, if the recv function got a
   locking.tdb record, we push the write time into the marshall buffer
   at the offet saved in the request state.

This new feature is still off by default as it doesn't
give any improvement in the non-clustered usecase.
"smbd:async search ask sharemode" can be used to activate it,
which makes only sense with "clustering = yes" (execept for testing).
---8<---

The patchset is already reviewed by metze, additional reviewers welcome!
Otherwise if noone comments or objects I'll push next week. Thanks!

Cheerio!
-slow

async-write-time.patch (92K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
On Thu, Mar 23, 2017 at 10:54:09AM +0100, Ralph Böhme via samba-technical wrote:
> This new feature is still off by default as it doesn't
> give any improvement in the non-clustered usecase.
> "smbd:async search ask sharemode" can be used to activate it,
> which makes only sense with "clustering = yes" (execept for testing).

Can we always enable this in clustered mode, i.e. not introduce the parameter?

Volker

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
On Thu, Mar 23, 2017 at 11:09:16AM +0100, Volker Lendecke wrote:
> On Thu, Mar 23, 2017 at 10:54:09AM +0100, Ralph Böhme via samba-technical wrote:
> > This new feature is still off by default as it doesn't
> > give any improvement in the non-clustered usecase.
> > "smbd:async search ask sharemode" can be used to activate it,
> > which makes only sense with "clustering = yes" (execept for testing).
>
> Can we always enable this in clustered mode, i.e. not introduce the parameter?

sure. I was a bit hesitant, given the size of the change, to enable this by
default. But if you and others think we should do this, I'm won't object. :)

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
On Thu, Mar 23, 2017 at 11:32:23AM +0100, Ralph Böhme wrote:

> On Thu, Mar 23, 2017 at 11:09:16AM +0100, Volker Lendecke wrote:
> > On Thu, Mar 23, 2017 at 10:54:09AM +0100, Ralph Böhme via samba-technical wrote:
> > > This new feature is still off by default as it doesn't
> > > give any improvement in the non-clustered usecase.
> > > "smbd:async search ask sharemode" can be used to activate it,
> > > which makes only sense with "clustering = yes" (execept for testing).
> >
> > Can we always enable this in clustered mode, i.e. not introduce the parameter?
>
> sure. I was a bit hesitant, given the size of the change, to enable this by
> default. But if you and others think we should do this, I'm won't object. :)

It is a big change, but we won't find the bugs if people don't use it.
And they won't use it if it's hidden behind a parametric option. I
know this a difficult decision, but because this does not affect
functionality at all, only performance, I'm not sure a parameter is
the right thing to do.

Just my 2ct.

Volker

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
Am 23.03.2017 um 12:43 schrieb Volker Lendecke via samba-technical:

> On Thu, Mar 23, 2017 at 11:32:23AM +0100, Ralph Böhme wrote:
>> On Thu, Mar 23, 2017 at 11:09:16AM +0100, Volker Lendecke wrote:
>>> On Thu, Mar 23, 2017 at 10:54:09AM +0100, Ralph Böhme via samba-technical wrote:
>>>> This new feature is still off by default as it doesn't
>>>> give any improvement in the non-clustered usecase.
>>>> "smbd:async search ask sharemode" can be used to activate it,
>>>> which makes only sense with "clustering = yes" (execept for testing).
>>>
>>> Can we always enable this in clustered mode, i.e. not introduce the parameter?
>>
>> sure. I was a bit hesitant, given the size of the change, to enable this by
>> default. But if you and others think we should do this, I'm won't object. :)
>
> It is a big change, but we won't find the bugs if people don't use it.
> And they won't use it if it's hidden behind a parametric option. I
> know this a difficult decision, but because this does not affect
> functionality at all, only performance, I'm not sure a parameter is
> the right thing to do.
Either that or we default it to yes (and may remove it later).

metze


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
On Thu, Mar 23, 2017 at 01:02:02PM +0100, Stefan Metzmacher via samba-technical wrote:

> Am 23.03.2017 um 12:43 schrieb Volker Lendecke via samba-technical:
> > On Thu, Mar 23, 2017 at 11:32:23AM +0100, Ralph Böhme wrote:
> >> On Thu, Mar 23, 2017 at 11:09:16AM +0100, Volker Lendecke wrote:
> >>> On Thu, Mar 23, 2017 at 10:54:09AM +0100, Ralph Böhme via samba-technical wrote:
> >>>> This new feature is still off by default as it doesn't
> >>>> give any improvement in the non-clustered usecase.
> >>>> "smbd:async search ask sharemode" can be used to activate it,
> >>>> which makes only sense with "clustering = yes" (execept for testing).
> >>>
> >>> Can we always enable this in clustered mode, i.e. not introduce the parameter?
> >>
> >> sure. I was a bit hesitant, given the size of the change, to enable this by
> >> default. But if you and others think we should do this, I'm won't object. :)
> >
> > It is a big change, but we won't find the bugs if people don't use it.
> > And they won't use it if it's hidden behind a parametric option. I
> > know this a difficult decision, but because this does not affect
> > functionality at all, only performance, I'm not sure a parameter is
> > the right thing to do.
>
> Either that or we default it to yes (and may remove it later).

I just went through this code. It's *great*. Very
impressive change !

Please turn this on by default. Volker is correct
that we will not find all the bugs in this until it is
turned on by default.

+1 for 4.7.0 !!!!

Great work Ralph and Metze.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
On Thu, Mar 23, 2017 at 06:36:58AM -0700, Jeremy Allison wrote:

> On Thu, Mar 23, 2017 at 01:02:02PM +0100, Stefan Metzmacher via samba-technical wrote:
> > Am 23.03.2017 um 12:43 schrieb Volker Lendecke via samba-technical:
> > > On Thu, Mar 23, 2017 at 11:32:23AM +0100, Ralph Böhme wrote:
> > >> On Thu, Mar 23, 2017 at 11:09:16AM +0100, Volker Lendecke wrote:
> > >>> On Thu, Mar 23, 2017 at 10:54:09AM +0100, Ralph Böhme via samba-technical wrote:
> > >>>> This new feature is still off by default as it doesn't
> > >>>> give any improvement in the non-clustered usecase.
> > >>>> "smbd:async search ask sharemode" can be used to activate it,
> > >>>> which makes only sense with "clustering = yes" (execept for testing).
> > >>>
> > >>> Can we always enable this in clustered mode, i.e. not introduce the parameter?
> > >>
> > >> sure. I was a bit hesitant, given the size of the change, to enable this by
> > >> default. But if you and others think we should do this, I'm won't object. :)
> > >
> > > It is a big change, but we won't find the bugs if people don't use it.
> > > And they won't use it if it's hidden behind a parametric option. I
> > > know this a difficult decision, but because this does not affect
> > > functionality at all, only performance, I'm not sure a parameter is
> > > the right thing to do.
> >
> > Either that or we default it to yes (and may remove it later).
>
> I just went through this code. It's *great*. Very
> impressive change !

Thanks!

> Please turn this on by default. Volker is correct
> that we will not find all the bugs in this until it is
> turned on by default.

What about folding the existing and the new parametric options

  smbd:search ask sharemode (existing)
  smbd:async search ask sharemode (new)

into a full fledged option with extended syntax:

  smbd:search ask sharemode = yes | sync | async | no (default async)

yes | sync = same behaviour as current setting of "yes"
async      = fetch write time, async version
no         = same behaviour as current setting of "no"

? Maybe the "sync" alias is not needed.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
On Thu, Mar 23, 2017 at 06:53:09PM +0100, Ralph Böhme wrote:

> On Thu, Mar 23, 2017 at 06:36:58AM -0700, Jeremy Allison wrote:
> > On Thu, Mar 23, 2017 at 01:02:02PM +0100, Stefan Metzmacher via samba-technical wrote:
> > > Am 23.03.2017 um 12:43 schrieb Volker Lendecke via samba-technical:
> > > > On Thu, Mar 23, 2017 at 11:32:23AM +0100, Ralph Böhme wrote:
> > > >> On Thu, Mar 23, 2017 at 11:09:16AM +0100, Volker Lendecke wrote:
> > > >>> On Thu, Mar 23, 2017 at 10:54:09AM +0100, Ralph Böhme via samba-technical wrote:
> > > >>>> This new feature is still off by default as it doesn't
> > > >>>> give any improvement in the non-clustered usecase.
> > > >>>> "smbd:async search ask sharemode" can be used to activate it,
> > > >>>> which makes only sense with "clustering = yes" (execept for testing).
> > > >>>
> > > >>> Can we always enable this in clustered mode, i.e. not introduce the parameter?
> > > >>
> > > >> sure. I was a bit hesitant, given the size of the change, to enable this by
> > > >> default. But if you and others think we should do this, I'm won't object. :)
> > > >
> > > > It is a big change, but we won't find the bugs if people don't use it.
> > > > And they won't use it if it's hidden behind a parametric option. I
> > > > know this a difficult decision, but because this does not affect
> > > > functionality at all, only performance, I'm not sure a parameter is
> > > > the right thing to do.
> > >
> > > Either that or we default it to yes (and may remove it later).
> >
> > I just went through this code. It's *great*. Very
> > impressive change !
>
> Thanks!
>
> > Please turn this on by default. Volker is correct
> > that we will not find all the bugs in this until it is
> > turned on by default.
>
> What about folding the existing and the new parametric options
>
>   smbd:search ask sharemode (existing)
>   smbd:async search ask sharemode (new)
>
> into a full fledged option with extended syntax:
>
>   smbd:search ask sharemode = yes | sync | async | no (default async)
>
> yes | sync = same behaviour as current setting of "yes"
> async      = fetch write time, async version
> no         = same behaviour as current setting of "no"
>
> ? Maybe the "sync" alias is not needed.

I think that's horribly complicated :-). Can we just turn
it on by default, and have an (undocumented) parametric
option to turn if off that we can remove once we're
confident it works correctly ?

Don't want to drop this on the floor :-).

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi,

On Thu, Mar 23, 2017 at 8:54 PM, Ralph Böhme via samba-technical <
[hidden email]> wrote:

> Hi!
>
> Attached is a patchset that improves SMB2_FIND performance in a ctdb
> cluster by
> making records fetching from locking.tdb asynchronous.
>
> This has three parts:
>
> - add a seperate IPC channel to ctdb for asynchrous processing
>
> - new async dbwrap API: dbwrap_parse_record_send|recv()
>
> - making use of it in smbd_smb2_query_directory_send()
>
> Some numbers follow.
>
> The time it takes to enumerate a directory with 50k files, current code:
>
> $ time bin/smbclient -U FAST\\slow%x -m smb3 //localhost/share -c "ls
> dir\50kfiles\*" > /dev/null | grep real
>
> 3 runs:
> real    0m10.693s
> real    0m10.608s
> real    0m10.710s
>
> Using the new async API this becomes:
>
> 4 runs:
> real    0m3.903s
> real    0m2.809s
> real    0m3.869s
> real    0m3.894s
>
> Some more implementation details shamelessly copied from the commit
> messages:
>
> ---8<---
> dbwrap: add dbwrap_parse_record_send/recv
>
> The req_state parameter tells the caller whether the async request is
> blocked in a full send queue:
>
> req_state >= DBWRAP_REQ_DISPATCHED := request is dispatched
> req_state < DBWRAP_REQ_DISPATCHED := send queue is full
>
> This is useful in a clustered Samba environment where the async dbwrap
> request is sent over a socket to the local ctdbd.
>
> If the send queue is full and the caller was issuing multiple async
> dbwrap requests in a loop, the caller knows it's probably time to stop
> sending requests for now and try again later.
>
> This will be used in subsequent commits in
> smbd_smb2_query_directory_send() when implementing async write time
> updates. Directories may contain umpteen files so we send many requests
> to ctdb without going through tevent and reading the responses which has
> the potential to deadlock.
> ---8<---
>
>
Looking at the code, it seems to duplicate a lot of functionality in the
CTDB client (new) code.  May be it's time to start using CTDB client code
to avoid duplication of effort and keep separate implementation of CTDB
protocol.  All of the CTDB client code has async API already.

May be something worth discussing at SambaXP?

Amitay.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Mar 29, 2017 at 01:54:27PM -0700, Jeremy Allison via samba-technical wrote:
> I think that's horribly complicated :-). Can we just turn
> it on by default, and have an (undocumented) parametric
> option to turn if off that we can remove once we're
> confident it works correctly ?
>
> Don't want to drop this on the floor :-).

Always turn it on please. The existing search ask sharemode option is
a functionality change that should stay as yes/no option, but if we're
in clustered mode we should not do the rabbit pellet mode.

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:[hidden email]

Samba eXPerience 2017, Hotel Freizeit In
2nd-4th of May 2017, http://sambaXP.org

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
On Thu, Mar 30, 2017 at 08:14:41AM +0200, Volker Lendecke via samba-technical wrote:

> On Wed, Mar 29, 2017 at 01:54:27PM -0700, Jeremy Allison via samba-technical wrote:
> > I think that's horribly complicated :-). Can we just turn
> > it on by default, and have an (undocumented) parametric
> > option to turn if off that we can remove once we're
> > confident it works correctly ?
> >
> > Don't want to drop this on the floor :-).
>
> Always turn it on please. The existing search ask sharemode option is
> a functionality change that should stay as yes/no option, but if we're
> in clustered mode we should not do the rabbit pellet mode.

Yeah, fair enough. I'm happy for it to be "always on".
We need to be confident enough in the changes to do this
anyway.


> 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:[hidden email]
>
> Samba eXPerience 2017, Hotel Freizeit In
> 2nd-4th of May 2017, http://sambaXP.org
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
On Fri, Mar 31, 2017 at 10:52:27AM -0700, Jeremy Allison wrote:

> On Thu, Mar 30, 2017 at 08:14:41AM +0200, Volker Lendecke via samba-technical wrote:
> > On Wed, Mar 29, 2017 at 01:54:27PM -0700, Jeremy Allison via samba-technical wrote:
> > > I think that's horribly complicated :-). Can we just turn
> > > it on by default, and have an (undocumented) parametric
> > > option to turn if off that we can remove once we're
> > > confident it works correctly ?
> > >
> > > Don't want to drop this on the floor :-).
> >
> > Always turn it on please. The existing search ask sharemode option is
> > a functionality change that should stay as yes/no option, but if we're
> > in clustered mode we should not do the rabbit pellet mode.
>
> Yeah, fair enough. I'm happy for it to be "always on".
> We need to be confident enough in the changes to do this
> anyway.

ok. Will post an updated patchset. Thanks!

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Thu, Mar 30, 2017 at 05:13:21PM +1100, Amitay Isaacs wrote:

> Hi,
>
> On Thu, Mar 23, 2017 at 8:54 PM, Ralph Böhme via samba-technical <
> [hidden email]> wrote:
>
> > Hi!
> >
> > Attached is a patchset that improves SMB2_FIND performance in a ctdb
> > cluster by
> > making records fetching from locking.tdb asynchronous.
> >
> > This has three parts:
> >
> > - add a seperate IPC channel to ctdb for asynchrous processing
> >
> > - new async dbwrap API: dbwrap_parse_record_send|recv()
> >
> > - making use of it in smbd_smb2_query_directory_send()
> >
> > Some numbers follow.
> >
> > The time it takes to enumerate a directory with 50k files, current code:
> >
> > $ time bin/smbclient -U FAST\\slow%x -m smb3 //localhost/share -c "ls
> > dir\50kfiles\*" > /dev/null | grep real
> >
> > 3 runs:
> > real    0m10.693s
> > real    0m10.608s
> > real    0m10.710s
> >
> > Using the new async API this becomes:
> >
> > 4 runs:
> > real    0m3.903s
> > real    0m2.809s
> > real    0m3.869s
> > real    0m3.894s
> >
> > Some more implementation details shamelessly copied from the commit
> > messages:
> >
> > ---8<---
> > dbwrap: add dbwrap_parse_record_send/recv
> >
> > The req_state parameter tells the caller whether the async request is
> > blocked in a full send queue:
> >
> > req_state >= DBWRAP_REQ_DISPATCHED := request is dispatched
> > req_state < DBWRAP_REQ_DISPATCHED := send queue is full
> >
> > This is useful in a clustered Samba environment where the async dbwrap
> > request is sent over a socket to the local ctdbd.
> >
> > If the send queue is full and the caller was issuing multiple async
> > dbwrap requests in a loop, the caller knows it's probably time to stop
> > sending requests for now and try again later.
> >
> > This will be used in subsequent commits in
> > smbd_smb2_query_directory_send() when implementing async write time
> > updates. Directories may contain umpteen files so we send many requests
> > to ctdb without going through tevent and reading the responses which has
> > the potential to deadlock.
> > ---8<---
> >
> >
> Looking at the code, it seems to duplicate a lot of functionality in the
> CTDB client (new) code.  May be it's time to start using CTDB client code
> to avoid duplication of effort and keep separate implementation of CTDB
> protocol.  All of the CTDB client code has async API already.
>
> May be something worth discussing at SambaXP?

yup. :) Afaict all I need is ctdb_client_call_send()/recv(), but we'll see.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] SMB2_FIND improvement for clustered environments

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, Mar 31, 2017 at 08:05:18PM +0200, Ralph Böhme wrote:

> On Fri, Mar 31, 2017 at 10:52:27AM -0700, Jeremy Allison wrote:
> > On Thu, Mar 30, 2017 at 08:14:41AM +0200, Volker Lendecke via samba-technical wrote:
> > > On Wed, Mar 29, 2017 at 01:54:27PM -0700, Jeremy Allison via samba-technical wrote:
> > > > I think that's horribly complicated :-). Can we just turn
> > > > it on by default, and have an (undocumented) parametric
> > > > option to turn if off that we can remove once we're
> > > > confident it works correctly ?
> > > >
> > > > Don't want to drop this on the floor :-).
> > >
> > > Always turn it on please. The existing search ask sharemode option is
> > > a functionality change that should stay as yes/no option, but if we're
> > > in clustered mode we should not do the rabbit pellet mode.
> >
> > Yeah, fair enough. I'm happy for it to be "always on".
> > We need to be confident enough in the changes to do this
> > anyway.
>
> ok. Will post an updated patchset. Thanks!
attached. No more "async search ask sharemode" option, it's now always on and
there's no way back. The old option "search ask sharemode" is still there of
course.

Thanks!
-slow

async_write_time-2017-04-18.patch (92K) Download Attachment
Loading...