[PATCH] lock reference count improvements

[Anoop C S]

On Fri, 2018-09-21 at 16:40 +0200, Andreas Schneider via samba-technical wrote:
> On Friday, 21 September 2018 13:23:29 CEST Michael Adam via samba-technical 
> wrote:
> > On 2018-09-18 at 23:09 +0530, Anoop C S via samba-technical wrote:
> > > Hi,
> > > 
> > > Attached are two patches which improves an assertion check and DEBUG
> > > statements around lock reference count code inside source3/locking.
> > 
> > Good catch, is there a bug for this? Should we have one?
> > 
> > > Reviews are appreciated.
> > 
> > See below:
> > > Thanks,
> > > Anoop C S
> > > 
> > > From 3e4645f14f3963d5df6b85e5b25514555641d460 Mon Sep 17 00:00:00 2001
> > > From: Anoop C S <anoopcs at redhat.com>
> > > Date: Tue, 18 Sep 2018 21:53:40 +0530
> > > Subject: [PATCH 1/2] s3/locking: Fix assertion check on lock reference
> > > count
> > > 
> > > lock_ref_count will always hold the old value prior to change. By the
> > > time current assertion check succeeds lock_ref_count would have already
> > > reached INT32_MAX. Th

> > > erefore modifying this assertion to check for a
> > > value less than i.e, (INT32_MAX - 1).
> > 
> > Is INT32_MAX not supported as a value?

I think we are good to have INT32_MAX. Thanks for pointing it out.
The only problem is that we will store (INT32_MAX + 1) in db and assert just afterwards.

I am revoking this patch. 

> > Is INT32_MAX - 1 the largest allowed value?
> > Note we are already asserting '<', not '<='.
> > 
> > > Signed-off-by: Anoop C S <anoopcs at redhat.com>
> > > ---
> > > 
> > >  source3/locking/posix.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/source3/locking/posix.c b/source3/locking/posix.c
> > > index 79c33cfb0fa..1ee82b76f28 100644
> > > --- a/source3/locking/posix.c
> > > +++ b/source3/locking/posix.c
> > > @@ -429,7 +429,7 @@ static void increment_lock_ref_count(const
> > > files_struct *fsp)> 
> > >  		&lock_ref_count, 1);
> > >  	
> > >  	SMB_ASSERT(NT_STATUS_IS_OK(status));
> > > 
> > > -	SMB_ASSERT(lock_ref_count < INT32_MAX);
> > > +	SMB_ASSERT(lock_ref_count < (INT32_MAX - 1));
> > > 
> > >  	DEBUG(10,("lock_ref_count for file %s = %d\n",
> > >  	
> > >  		  fsp_str_dbg(fsp), (int)lock_ref_count));
> > > 
> > > From 6113aeef666b10f0679d3bbb6a86e916bbdf5bc0 Mon Sep 17 00:00:00 2001
> > > From: Anoop C S <anoopcs at redhat.com>
> > > Date: Tue, 18 Sep 2018 21:53:54 +0530
> > > Subject: [PATCH 2/2] s3/locking: Fix logging of lock reference count
> > > 
> > > lock refernce count is always increased and reduced by a value of 1.
> 
>          ^^^^^^^^
>          typo :-)

Fixed in attached version.

> > > But lock_ref_count variable holds the old value prior to change and
> > > was being logged wrongly under debug level 10. DEBUG statement must
> > > log lock_ref_count+1 and lock_ref_count-1 respectively when value
> > > gets increased and decreased.
> > 
> > This second patch LGTM.
> > 
> > Cheers - Michael
> > 
> > > Signed-off-by: Anoop C S <anoopcs at redhat.com>
> > > ---
> > > 
> > >  source3/locking/posix.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/source3/locking/posix.c b/source3/locking/posix.c
> > > index 1ee82b76f28..787577408e5 100644
> > > --- a/source3/locking/posix.c
> > > +++ b/source3/locking/posix.c
> > > @@ -432,7 +432,7 @@ static void increment_lock_ref_count(const
> > > files_struct *fsp)> 
> > >  	SMB_ASSERT(lock_ref_count < (INT32_MAX - 1));
> > >  	
> > >  	DEBUG(10,("lock_ref_count for file %s = %d\n",
> > > 
> > > -		  fsp_str_dbg(fsp), (int)lock_ref_count));
> > > +		  fsp_str_dbg(fsp), (int)(lock_ref_count + 1)));
> > > 
> > >  }
> > >  
> > >  /************************************************************************
> > >  ****> 
> > > @@ -453,7 +453,7 @@ static void decrement_lock_ref_count(const
> > > files_struct *fsp)> 
> > >  	SMB_ASSERT(lock_ref_count > 0);
> > >  	
> > >  	DEBUG(10,("lock_ref_count for file %s = %d\n",
> > > 
> > > -		  fsp_str_dbg(fsp), (int)lock_ref_count));
> > > +		  fsp_str_dbg(fsp), (int)(lock_ref_count - 1)));
> > > 
> > >  }
> > >  
> > >  /************************************************************************
> > >  ****
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-s3-locking-Fix-logging-of-lock-reference-count.patch
Type: text/x-patch
Size: 1531 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180922/e13ea88c/0001-s3-locking-Fix-logging-of-lock-reference-count.bin>