Quantcast

[PATCH] Update two optimisations that check for oplocks to the lease area

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

[PATCH] Update two optimisations that check for oplocks to the lease area

Samba - samba-technical mailing list
Hi!

This is the one I talked about in my talk.

I took another look and found another plase where we need this.

Both combined eat 12%CPU in perf, not including syscalls.

Please review carefully and push if ok. Thanks!

-slow

bug12766-master.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Update two optimisations that check for oplocks to the lease area

Samba - samba-technical mailing list
On Thu, May 04, 2017 at 12:26:48PM +0200, Ralph Böhme wrote:
> Hi!
>
> This is the one I talked about in my talk.
>
> I took another look and found another plase where we need this.
>
> Both combined eat 12%CPU in perf, not including syscalls.
>
> Please review carefully and push if ok. Thanks!

Reviewed carefully :-) and pushed ! Thanks Ralph,
nice work.

> From 56f1da66edd62ffb7192a462664227fe9c0ae3f2 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Thu, 4 May 2017 11:50:01 +0200
> Subject: [PATCH 1/4] s3/locking: add const to fsp_lease_type
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12766
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/locking/leases_util.c | 2 +-
>  source3/locking/proto.h       | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/source3/locking/leases_util.c b/source3/locking/leases_util.c
> index cb307c8..5d07ff9 100644
> --- a/source3/locking/leases_util.c
> +++ b/source3/locking/leases_util.c
> @@ -46,7 +46,7 @@ uint32_t map_oplock_to_lease_type(uint16_t op_type)
>   return ret;
>  }
>  
> -uint32_t fsp_lease_type(struct files_struct *fsp)
> +uint32_t fsp_lease_type(const struct files_struct *fsp)
>  {
>   if (fsp->oplock_type == LEASE_OPLOCK) {
>   return fsp->lease->lease.lease_state;
> diff --git a/source3/locking/proto.h b/source3/locking/proto.h
> index 967af02..44ab315 100644
> --- a/source3/locking/proto.h
> +++ b/source3/locking/proto.h
> @@ -257,6 +257,6 @@ bool release_posix_lock_posix_flavour(files_struct *fsp,
>  
>  /* The following definitions come from locking/leases_util.c */
>  uint32_t map_oplock_to_lease_type(uint16_t op_type);
> -uint32_t fsp_lease_type(struct files_struct *fsp);
> +uint32_t fsp_lease_type(const struct files_struct *fsp);
>  
>  #endif /* _LOCKING_PROTO_H_ */
> --
> 2.9.3
>
>
> From 4c224ecd9f91fb89298ee91161e866643d494902 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Thu, 4 May 2017 11:50:56 +0200
> Subject: [PATCH 2/4] s3/locking: helper functions for lease types
>
> Add some helper functions that will be used to update a bunch of checks
> for exclusive oplocks to the lease area.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12766
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/locking/leases_util.c | 17 +++++++++++++++++
>  source3/locking/proto.h       |  2 ++
>  2 files changed, 19 insertions(+)
>
> diff --git a/source3/locking/leases_util.c b/source3/locking/leases_util.c
> index 5d07ff9..af1e837 100644
> --- a/source3/locking/leases_util.c
> +++ b/source3/locking/leases_util.c
> @@ -53,3 +53,20 @@ uint32_t fsp_lease_type(const struct files_struct *fsp)
>   }
>   return map_oplock_to_lease_type(fsp->oplock_type);
>  }
> +
> +uint32_t lease_type_is_exclusive(uint32_t lease_type)
> +{
> + if ((lease_type & (SMB2_LEASE_READ | SMB2_LEASE_WRITE)) ==
> +    (SMB2_LEASE_READ | SMB2_LEASE_WRITE)) {
> + return true;
> + }
> +
> + return false;
> +}
> +
> +bool fsp_lease_type_is_exclusive(const struct files_struct *fsp)
> +{
> + uint32_t lease_type = fsp_lease_type(fsp);
> +
> + return lease_type_is_exclusive(lease_type);
> +}
> diff --git a/source3/locking/proto.h b/source3/locking/proto.h
> index 44ab315..5d2326a 100644
> --- a/source3/locking/proto.h
> +++ b/source3/locking/proto.h
> @@ -258,5 +258,7 @@ bool release_posix_lock_posix_flavour(files_struct *fsp,
>  /* The following definitions come from locking/leases_util.c */
>  uint32_t map_oplock_to_lease_type(uint16_t op_type);
>  uint32_t fsp_lease_type(const struct files_struct *fsp);
> +uint32_t lease_type_is_exclusive(uint32_t lease_type);
> +bool fsp_lease_type_is_exclusive(const struct files_struct *fsp);
>  
>  #endif /* _LOCKING_PROTO_H_ */
> --
> 2.9.3
>
>
> From b704c881885498cf308cd867106a4a33b530fd28 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Thu, 20 Apr 2017 21:37:37 +0200
> Subject: [PATCH 3/4] s3/smbd: update exclusive oplock optimisation to the
>  lease area
>
> This is similar to 9533a55ee5ffe430589dcea845851b84876ef656 but this
> time in the contend_level2_oplocks_begin_default() function.
>
> The idea of the optimisation is to avoid expensive db queries in
> locking.tdb if we *know* we're the only open.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12766
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/smbd/oplock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/smbd/oplock.c b/source3/smbd/oplock.c
> index 417eda5..cd92c83 100644
> --- a/source3/smbd/oplock.c
> +++ b/source3/smbd/oplock.c
> @@ -1044,7 +1044,7 @@ static void contend_level2_oplocks_begin_default(files_struct *fsp,
>   * the shared memory area whilst doing this.
>   */
>  
> - if (EXCLUSIVE_OPLOCK_TYPE(fsp->oplock_type)) {
> + if (fsp_lease_type_is_exclusive(fsp)) {
>   /*
>   * There can't be any level2 oplocks, we're alone.
>   */
> --
> 2.9.3
>
>
> From 2b47711a7a93a7a92f04d5083de6939b8b4685d6 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Thu, 4 May 2017 11:52:16 +0200
> Subject: [PATCH 4/4] s3/smbd: update exclusive oplock optimisation to the
>  lease area
>
> Update an optimisation in update_num_read_oplocks() that checks for
> exclusive oplocks to the lease area.
>
> The idea of the optimisation is to avoid expensive db queries in
> brlock.tdb if we *know* we're the only open.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12766
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/smbd/oplock.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/source3/smbd/oplock.c b/source3/smbd/oplock.c
> index cd92c83..87ca53c 100644
> --- a/source3/smbd/oplock.c
> +++ b/source3/smbd/oplock.c
> @@ -166,13 +166,17 @@ bool update_num_read_oplocks(files_struct *fsp, struct share_mode_lock *lck)
>   uint32_t num_read_oplocks = 0;
>   uint32_t i;
>  
> - if (EXCLUSIVE_OPLOCK_TYPE(fsp->oplock_type)) {
> + if (fsp_lease_type_is_exclusive(fsp)) {
> + uint32_t lease_type;
> +
>   /*
>   * If we're the only one, we don't need a brlock entry
>   */
>   remove_stale_share_mode_entries(d);
>   SMB_ASSERT(d->num_share_modes == 1);
> - SMB_ASSERT(EXCLUSIVE_OPLOCK_TYPE(d->share_modes[0].op_type));
> +
> + lease_type = get_lease_type(d, 0);
> + SMB_ASSERT(lease_type_is_exclusive(lease_type));
>   return true;
>   }
>  
> --
> 2.9.3
>


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

Re: [PATCH] Update two optimisations that check for oplocks to the lease area

Samba - samba-technical mailing list
On Thu, May 04, 2017 at 04:25:04AM -0700, Jeremy Allison wrote:

> On Thu, May 04, 2017 at 12:26:48PM +0200, Ralph Böhme wrote:
> > Hi!
> >
> > This is the one I talked about in my talk.
> >
> > I took another look and found another plase where we need this.
> >
> > Both combined eat 12%CPU in perf, not including syscalls.
> >
> > Please review carefully and push if ok. Thanks!
>
> Reviewed carefully :-) and pushed ! Thanks Ralph,
> nice work.

woohoo! Thanks!

-slow

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

Re: [PATCH] Update two optimisations that check for oplocks to the lease area

Samba - samba-technical mailing list
On Thu, May 04, 2017 at 01:35:36PM +0200, Ralph Böhme via samba-technical wrote:

> On Thu, May 04, 2017 at 04:25:04AM -0700, Jeremy Allison wrote:
> > On Thu, May 04, 2017 at 12:26:48PM +0200, Ralph Böhme wrote:
> > > Hi!
> > >
> > > This is the one I talked about in my talk.
> > >
> > > I took another look and found another plase where we need this.
> > >
> > > Both combined eat 12%CPU in perf, not including syscalls.
> > >
> > > Please review carefully and push if ok. Thanks!
> >
> > Reviewed carefully :-) and pushed ! Thanks Ralph,
> > nice work.
>
> woohoo! Thanks!

Oh not so fast, sorry :-(. I think there's a logic
bug in the num_read_oplocks() case. Still investigating..

Jeremy.

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

Re: [PATCH] Update two optimisations that check for oplocks to the lease area

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Thu, May 04, 2017 at 12:26:48PM +0200, Ralph Böhme wrote:
> Hi!
>
> This is the one I talked about in my talk.
>
> I took another look and found another plase where we need this.
>
> Both combined eat 12%CPU in perf, not including syscalls.
>
> Please review carefully and push if ok. Thanks!

OK, there were some flaws in the last patch in this set
(found by the smb2.lease tests).

Can you try this version of your patch instead ? It changes
the assertion in the last patch (num_oplocks one) to ensure
that all existing leases in the array are exclusive (remember
with leases the client can open multiple times with compatible
leases so long as the client guid is the same - it's the multi-handle
advantage leases have over oplocks).

The logic in the assertion now looks like:

        if (fsp_lease_type_is_exclusive(fsp)) {
                /*
                 * If we're fully exclusive, we don't need a brlock entry
                 */
                remove_stale_share_mode_entries(d);

                for (i=0; i<d->num_share_modes; i++) {
                        struct share_mode_entry *e = &d->share_modes[i];
                        uint32_t e_lease_type = get_lease_type(d, e);

                        SMB_ASSERT(lease_type_is_exclusive(e_lease_type));
                }
                return true;
        }

instead of checking for only one entry.

Please push if happy !

Jeremy.

leases (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Update two optimisations that check for oplocks to the lease area

Samba - samba-technical mailing list
On Thu, May 04, 2017 at 11:29:21PM -0700, Jeremy Allison wrote:

> On Thu, May 04, 2017 at 12:26:48PM +0200, Ralph Böhme wrote:
> > Hi!
> >
> > This is the one I talked about in my talk.
> >
> > I took another look and found another plase where we need this.
> >
> > Both combined eat 12%CPU in perf, not including syscalls.
> >
> > Please review carefully and push if ok. Thanks!
>
> OK, there were some flaws in the last patch in this set
> (found by the smb2.lease tests).
>
> Can you try this version of your patch instead ? It changes
> the assertion in the last patch (num_oplocks one) to ensure
> that all existing leases in the array are exclusive (remember
> with leases the client can open multiple times with compatible
> leases so long as the client guid is the same - it's the multi-handle
> advantage leases have over oplocks).
>
> The logic in the assertion now looks like:
>
>         if (fsp_lease_type_is_exclusive(fsp)) {
>                 /*
>                  * If we're fully exclusive, we don't need a brlock entry
>                  */
>                 remove_stale_share_mode_entries(d);
>
>                 for (i=0; i<d->num_share_modes; i++) {
>                         struct share_mode_entry *e = &d->share_modes[i];
>                         uint32_t e_lease_type = get_lease_type(d, e);
>
>                         SMB_ASSERT(lease_type_is_exclusive(e_lease_type));
>                 }
>                 return true;
>         }
>
> instead of checking for only one entry.
>
> Please push if happy !

Pushed!

I'm really sorry for dumping half baked and not properly tested code on you. And
thanks for helping to get it right!

-slow

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

Re: [PATCH] Update two optimisations that check for oplocks to the lease area

Samba - samba-technical mailing list
On Sat, May 06, 2017 at 06:55:54PM +0200, Ralph Böhme wrote:
> Pushed!
>
> I'm really sorry for dumping half baked and not properly tested code on you. And
> thanks for helping to get it right!

No problem. Do you think we should raise a bug for this
so we can back-port to existing versions (it gives 12%+
improvements, which is quite a lot !) ?

Jeremy.

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

Re: [PATCH] Update two optimisations that check for oplocks to the lease area

Samba - samba-technical mailing list
On Sat, May 06, 2017 at 02:08:32PM -0700, Jeremy Allison wrote:
> On Sat, May 06, 2017 at 06:55:54PM +0200, Ralph Böhme wrote:
> > Pushed!
> >
> > I'm really sorry for dumping half baked and not properly tested code on you. And
> > thanks for helping to get it right!
>
> No problem. Do you think we should raise a bug for this
> so we can back-port to existing versions (it gives 12%+
> improvements, which is quite a lot !) ?

take a look at the commit messages:
<https://git.samba.org/?p=samba.git;a=commit;h=a50343779a8a92d6f53095b36506b1d47ef68513>

:)

I will do the backports later on.

-slow

Loading...