Increasing response times for byte range unlock requests.

[Hemanth Thummala]

Jeremy,

Yes you are correct. In that case, I think we can just use
brl_lock_cancel() instead of remove_pending_lock() to avoid any chances of
removing existing locks.

I have also filed bug(https://bugzilla.samba.org/show_bug.cgi?id=10673) for
the same issue. Will look for more updates as part of this.

Thanks alot for looking at this issue.

Thanks,
Hemanth.




On Wed, Jun 25, 2014 at 10:40 AM, Jeremy Allison <jra at samba.org> wrote:

> On Tue, Jun 24, 2014 at 10:05:58PM -0700, Jeremy Allison wrote:
> > On Wed, Jun 25, 2014 at 10:20:37AM +0530, Hemanth Thummala wrote:
> > > Hi Jeremy,
> > >
> > > Clients are running SMB2. We haven't tested with SMB1 clients.
> > >
> > > I think I  have identified the problem. When the blocking lock
> requests are
> > > reprocessed successfully, we are not removing them from pending queue
> in
> > > function reprocess_blocked_smb2_lock().
> > >
> > > With the following changes, we did not observe any further degradation
> in
> > > unlock responses.
> > >
> > > bash-4.0$ diff -up smbd/smb2_lock.c.orig smbd/smb2_lock.c
> > > --- smbd/smb2_lock.c.orig       2014-06-24 21:43:25.300420642 -0700
> > > +++ smbd/smb2_lock.c    2014-06-24 21:43:33.290457365 -0700
> > > @@ -745,6 +745,10 @@ static void reprocess_blocked_smb2_lock(
> > >                 if (NT_STATUS_IS_ERR(status)) {
> > >                         break;
> > >                 }
> > > +               else {
> > > +                       /* Removed it from pending queue.*/
> > > +                       remove_pending_lock(state, blr);
> > > +               }
> > >         }
> > >
> > >         if(blr->lock_num == state->lock_count) {
> > >
> > > Please let me know if this change looks fine.
> > >
> > > I will also raise a bug and update all the details.
> >
> > Yes, in looking over the code I was coming to
> > the same conclusion (we're not removing the pending
> > lock records correctly :-).
> >
> > At first glance your fix looks right, but only
> > because SMB2 wait lock requests can only ever
> > have one element :-).
> >
> > I'll check this code path in SMB1 also to make
> > sure this is also being done correctly there.
> >
> > Please raise a bug and I'll get patches prepared
> > for master, 4.1.x and 4.0.x (all active branches
> > we'll be doing releases on - 3.6.x is security
> > maintanence only right now but your fix should
> > do the jobs there).
> >
> > Thanks a *LOT* for your help on this !
>
> Actually I think remove_pending_lock() inside smb2_lock.c
> is incorrect, due to the code it tries to execute
> after the comment:
>
> /* Remove the locks we already got. */
>
> Having said that, again we get away with it
> because SMB2 blocking locks can only ever
> have one waitable element (Windows behavior
> note:
>
> 3.3.5.14.2
> Processing Locks
> If the Locks array has more than one entry and the Flags field in any of
> these entries does not
> have SMB2_LOCKFLAG_FAIL_IMMEDIATELY set, the server SHOULD<296> fail the
> request with
> STATUS_INVALID_PARAMETER.
>
> :-).
>
> But that code really shouldn't be there :-).
>
> Jeremy.
>