[PATCH] lockd: Remove unused b_fl member from struct nlm_block

[PATCH] lockd: Remove unused b_fl member from struct nlm_block

[Kinglong Mee]

Fix left code by Joe Perches's patch,
"locks: Remove unused conf argument from lm_grant"

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/lockd/svclock.c          | 26 +++++---------------------
 include/linux/lockd/lockd.h |  1 -
 2 files changed, 5 insertions(+), 22 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 2a61701..796e63b 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -245,7 +245,6 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
 	block->b_daemon = rqstp->rq_server;
 	block->b_host   = host;
 	block->b_file   = file;
-	block->b_fl = NULL;
 	file->f_count++;
 
 	/* Add to file's list of blocks */
@@ -295,7 +294,6 @@ static void nlmsvc_free_block(struct kref *kref)
 	nlmsvc_freegrantargs(block->b_call);
 	nlmsvc_release_call(block->b_call);
 	nlm_release_file(block->b_file);
-	kfree(block->b_fl);
 	kfree(block);
 }
 
@@ -523,20 +521,13 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 	block = nlmsvc_lookup_block(file, lock);
 
 	if (block == NULL) {
-		struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
-
-		if (conf == NULL)
-			return nlm_granted;
 		block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
-		if (block == NULL) {
-			kfree(conf);
+		if (block == NULL)
 			return nlm_granted;
-		}
-		block->b_fl = conf;
 	}
 	if (block->b_flags & B_QUEUED) {
-		dprintk("lockd: nlmsvc_testlock deferred block %p flags %d fl %p\n",
-			block, block->b_flags, block->b_fl);
+		dprintk("lockd: nlmsvc_testlock deferred block %p flags %d\n",
+			block, block->b_flags);
 		if (block->b_flags & B_TIMED_OUT) {
 			nlmsvc_unlink_block(block);
 			ret = nlm_lck_denied;
@@ -544,14 +535,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 		}
 		if (block->b_flags & B_GOT_CALLBACK) {
 			nlmsvc_unlink_block(block);
-			if (block->b_fl != NULL
-					&& block->b_fl->fl_type != F_UNLCK) {
-				lock->fl = *block->b_fl;
-				goto conf_lock;
-			} else {
-				ret = nlm_granted;
-				goto out;
-			}
+			ret = nlm_granted;
+			goto out;
 		}
 		ret = nlm_drop_reply;
 		goto out;
@@ -575,7 +560,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 		goto out;
 	}
 
-conf_lock:
 	dprintk("lockd: conflicting lock(ty=%d, %Ld-%Ld)\n",
 		lock->fl.fl_type, (long long)lock->fl.fl_start,
 		(long long)lock->fl.fl_end);
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 219d796..ff82a32 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -178,7 +178,6 @@ struct nlm_block {
 	unsigned char		b_granted;	/* VFS granted lock */
 	struct nlm_file *	b_file;		/* file in question */
 	struct cache_req *	b_cache_req;	/* deferred request handling */
-	struct file_lock *	b_fl;		/* set for GETLK */
 	struct cache_deferred_req * b_deferred_req;
 	unsigned int		b_flags;	/* block flags */
 #define B_QUEUED		1	/* lock queued */
-- 
1.9.3

Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block

[Jeff Layton]

On Wed, 20 Aug 2014 18:07:19 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:

> Fix left code by Joe Perches's patch,
> "locks: Remove unused conf argument from lm_grant"
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/lockd/svclock.c          | 26 +++++---------------------
>  include/linux/lockd/lockd.h |  1 -
>  2 files changed, 5 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 2a61701..796e63b 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -245,7 +245,6 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
>  	block->b_daemon = rqstp->rq_server;
>  	block->b_host   = host;
>  	block->b_file   = file;
> -	block->b_fl = NULL;
>  	file->f_count++;
>  
>  	/* Add to file's list of blocks */
> @@ -295,7 +294,6 @@ static void nlmsvc_free_block(struct kref *kref)
>  	nlmsvc_freegrantargs(block->b_call);
>  	nlmsvc_release_call(block->b_call);
>  	nlm_release_file(block->b_file);
> -	kfree(block->b_fl);
>  	kfree(block);
>  }
>  
> @@ -523,20 +521,13 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>  	block = nlmsvc_lookup_block(file, lock);
>  
>  	if (block == NULL) {
> -		struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> -
> -		if (conf == NULL)
> -			return nlm_granted;
>  		block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> -		if (block == NULL) {
> -			kfree(conf);
> +		if (block == NULL)
>  			return nlm_granted;
> -		}
> -		block->b_fl = conf;

NAK. The b_fl member is not unused, as is evidenced by the assignment
above.

Joe's patch removed the conflock from the lm_grant callback since the
filesystem never set that parameter in the lm_grant callback. This call
however has nothing to do with lm_grant. It's done when the client
issues a NLM_TEST operation.

>  	}
>  	if (block->b_flags & B_QUEUED) {
> -		dprintk("lockd: nlmsvc_testlock deferred block %p flags %d fl %p\n",
> -			block, block->b_flags, block->b_fl);
> +		dprintk("lockd: nlmsvc_testlock deferred block %p flags %d\n",
> +			block, block->b_flags);
>  		if (block->b_flags & B_TIMED_OUT) {
>  			nlmsvc_unlink_block(block);
>  			ret = nlm_lck_denied;
> @@ -544,14 +535,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>  		}
>  		if (block->b_flags & B_GOT_CALLBACK) {
>  			nlmsvc_unlink_block(block);
> -			if (block->b_fl != NULL
> -					&& block->b_fl->fl_type != F_UNLCK) {
> -				lock->fl = *block->b_fl;
> -				goto conf_lock;
> -			} else {
> -				ret = nlm_granted;
> -				goto out;
> -			}
> +			ret = nlm_granted;
> +			goto out;
>  		}
>  		ret = nlm_drop_reply;
>  		goto out;
> @@ -575,7 +560,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>  		goto out;
>  	}
>  
> -conf_lock:
>  	dprintk("lockd: conflicting lock(ty=%d, %Ld-%Ld)\n",
>  		lock->fl.fl_type, (long long)lock->fl.fl_start,
>  		(long long)lock->fl.fl_end);
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 219d796..ff82a32 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -178,7 +178,6 @@ struct nlm_block {
>  	unsigned char		b_granted;	/* VFS granted lock */
>  	struct nlm_file *	b_file;		/* file in question */
>  	struct cache_req *	b_cache_req;	/* deferred request handling */
> -	struct file_lock *	b_fl;		/* set for GETLK */
>  	struct cache_deferred_req * b_deferred_req;
>  	unsigned int		b_flags;	/* block flags */
>  #define B_QUEUED		1	/* lock queued */


-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block

[Jeff Layton]

On Wed, 20 Aug 2014 18:07:19 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:

> Fix left code by Joe Perches's patch,
> "locks: Remove unused conf argument from lm_grant"
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/lockd/svclock.c          | 26 +++++---------------------
>  include/linux/lockd/lockd.h |  1 -
>  2 files changed, 5 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 2a61701..796e63b 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -245,7 +245,6 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
>  	block->b_daemon = rqstp->rq_server;
>  	block->b_host   = host;
>  	block->b_file   = file;
> -	block->b_fl = NULL;
>  	file->f_count++;
>  
>  	/* Add to file's list of blocks */
> @@ -295,7 +294,6 @@ static void nlmsvc_free_block(struct kref *kref)
>  	nlmsvc_freegrantargs(block->b_call);
>  	nlmsvc_release_call(block->b_call);
>  	nlm_release_file(block->b_file);
> -	kfree(block->b_fl);
>  	kfree(block);
>  }
>  
> @@ -523,20 +521,13 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>  	block = nlmsvc_lookup_block(file, lock);
>  
>  	if (block == NULL) {
> -		struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> -

I think the problem here is that we're allocating a file_lock with
kzalloc instead of using locks_alloc_lock. I think we should change
this code to use that, and the kfree in nlmsvc_free_block to use
locks_free_lock. Maybe something like this? (untested, but it compiles).

I'll plan to resend the patch below for 3.18 once I've tested it out and
Trond, Bruce and I can work out who should merge it.

-------------------------[snip]-------------------------

[PATCH] lockd: switch allocation of conflock to standard lock allocation routines

lockd currently allocates a struct file_lock with kzalloc to use as a
conflock. Change it to use locks_alloc_lock and locks_free_lock instead.

In the event that someone were to add lm_get_owner/lm_put_owner ops for
lockd, then this would help ensure that things get cleaned up properly.
It's also less wasteful with memory since locks_alloc_lock allocates
from a dedicated slabcache.

Cc: Kinglong Mee <kinglongmee@gmail.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/lockd/svclock.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 2a6170133c1d..1eb2ae47e6b1 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -295,7 +295,8 @@ static void nlmsvc_free_block(struct kref *kref)
 	nlmsvc_freegrantargs(block->b_call);
 	nlmsvc_release_call(block->b_call);
 	nlm_release_file(block->b_file);
-	kfree(block->b_fl);
+	if (block->b_fl)
+		locks_free_lock(block->b_fl);
 	kfree(block);
 }
 
@@ -523,13 +524,13 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 	block = nlmsvc_lookup_block(file, lock);
 
 	if (block == NULL) {
-		struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
+		struct file_lock *conf = locks_alloc_lock();
 
 		if (conf == NULL)
 			return nlm_granted;
 		block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
 		if (block == NULL) {
-			kfree(conf);
+			locks_free_lock(conf);
 			return nlm_granted;
 		}
 		block->b_fl = conf;
-- 
1.9.3

Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block

[Kinglong Mee]

On 8/20/2014 18:58, Jeff Layton wrote:
> On Wed, 20 Aug 2014 18:07:19 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
> 
>> Fix left code by Joe Perches's patch,
>> "locks: Remove unused conf argument from lm_grant"
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>>  fs/lockd/svclock.c          | 26 +++++---------------------
>>  include/linux/lockd/lockd.h |  1 -
>>  2 files changed, 5 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
>> index 2a61701..796e63b 100644
>> --- a/fs/lockd/svclock.c
>> +++ b/fs/lockd/svclock.c
>> @@ -245,7 +245,6 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
>>  	block->b_daemon = rqstp->rq_server;
>>  	block->b_host   = host;
>>  	block->b_file   = file;
>> -	block->b_fl = NULL;
>>  	file->f_count++;
>>  
>>  	/* Add to file's list of blocks */
>> @@ -295,7 +294,6 @@ static void nlmsvc_free_block(struct kref *kref)
>>  	nlmsvc_freegrantargs(block->b_call);
>>  	nlmsvc_release_call(block->b_call);
>>  	nlm_release_file(block->b_file);
>> -	kfree(block->b_fl);
>>  	kfree(block);
>>  }
>>  
>> @@ -523,20 +521,13 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>>  	block = nlmsvc_lookup_block(file, lock);
>>  
>>  	if (block == NULL) {
>> -		struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
>> -
>> -		if (conf == NULL)
>> -			return nlm_granted;
>>  		block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
>> -		if (block == NULL) {
>> -			kfree(conf);
>> +		if (block == NULL)
>>  			return nlm_granted;
>> -		}
>> -		block->b_fl = conf;
> 
> NAK. The b_fl member is not unused, as is evidenced by the assignment
> above.

Sorry for my bad title, Maybe I should use a good name, sorry!

> 
> Joe's patch removed the conflock from the lm_grant callback since the
> filesystem never set that parameter in the lm_grant callback. This call
> however has nothing to do with lm_grant. It's done when the client
> issues a NLM_TEST operation.
> 
>>  	}
>>  	if (block->b_flags & B_QUEUED) {
>> -		dprintk("lockd: nlmsvc_testlock deferred block %p flags %d fl %p\n",
>> -			block, block->b_flags, block->b_fl);
>> +		dprintk("lockd: nlmsvc_testlock deferred block %p flags %d\n",
>> +			block, block->b_flags);
>>  		if (block->b_flags & B_TIMED_OUT) {
>>  			nlmsvc_unlink_block(block);
>>  			ret = nlm_lck_denied;
>> @@ -544,14 +535,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>>  		}
>>  		if (block->b_flags & B_GOT_CALLBACK) {
>>  			nlmsvc_unlink_block(block);
>> -			if (block->b_fl != NULL
>> -					&& block->b_fl->fl_type != F_UNLCK) {
>> -				lock->fl = *block->b_fl;
>> -				goto conf_lock;

block->b_fl = conf just set an all-zero filed structure to block above,
and never be updated later.
If lockd enter here, lock->fl will contains all filed with zero,
I don't know whether is it OK.

thanks,
Kinglong Mee

>> -			} else {
>> -				ret = nlm_granted;
>> -				goto out;
>> -			}
>> +			ret = nlm_granted;
>> +			goto out;
>>  		}
>>  		ret = nlm_drop_reply;
>>  		goto out;
>> @@ -575,7 +560,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>>  		goto out;
>>  	}
>>  
>> -conf_lock:
>>  	dprintk("lockd: conflicting lock(ty=%d, %Ld-%Ld)\n",
>>  		lock->fl.fl_type, (long long)lock->fl.fl_start,
>>  		(long long)lock->fl.fl_end);
>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
>> index 219d796..ff82a32 100644
>> --- a/include/linux/lockd/lockd.h
>> +++ b/include/linux/lockd/lockd.h
>> @@ -178,7 +178,6 @@ struct nlm_block {
>>  	unsigned char		b_granted;	/* VFS granted lock */
>>  	struct nlm_file *	b_file;		/* file in question */
>>  	struct cache_req *	b_cache_req;	/* deferred request handling */
>> -	struct file_lock *	b_fl;		/* set for GETLK */
>>  	struct cache_deferred_req * b_deferred_req;
>>  	unsigned int		b_flags;	/* block flags */
>>  #define B_QUEUED		1	/* lock queued */
> 
> 

Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block

[Jeff Layton]

On Wed, 20 Aug 2014 20:10:51 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:

> On 8/20/2014 18:58, Jeff Layton wrote:
> > On Wed, 20 Aug 2014 18:07:19 +0800
> > Kinglong Mee <kinglongmee@gmail.com> wrote:
> > 
> >> Fix left code by Joe Perches's patch,
> >> "locks: Remove unused conf argument from lm_grant"
> >>
> >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> >> ---
> >>  fs/lockd/svclock.c          | 26 +++++---------------------
> >>  include/linux/lockd/lockd.h |  1 -
> >>  2 files changed, 5 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> >> index 2a61701..796e63b 100644
> >> --- a/fs/lockd/svclock.c
> >> +++ b/fs/lockd/svclock.c
> >> @@ -245,7 +245,6 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
> >>  	block->b_daemon = rqstp->rq_server;
> >>  	block->b_host   = host;
> >>  	block->b_file   = file;
> >> -	block->b_fl = NULL;
> >>  	file->f_count++;
> >>  
> >>  	/* Add to file's list of blocks */
> >> @@ -295,7 +294,6 @@ static void nlmsvc_free_block(struct kref *kref)
> >>  	nlmsvc_freegrantargs(block->b_call);
> >>  	nlmsvc_release_call(block->b_call);
> >>  	nlm_release_file(block->b_file);
> >> -	kfree(block->b_fl);
> >>  	kfree(block);
> >>  }
> >>  
> >> @@ -523,20 +521,13 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> >>  	block = nlmsvc_lookup_block(file, lock);
> >>  
> >>  	if (block == NULL) {
> >> -		struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> >> -
> >> -		if (conf == NULL)
> >> -			return nlm_granted;
> >>  		block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> >> -		if (block == NULL) {
> >> -			kfree(conf);
> >> +		if (block == NULL)
> >>  			return nlm_granted;
> >> -		}
> >> -		block->b_fl = conf;
> > 
> > NAK. The b_fl member is not unused, as is evidenced by the assignment
> > above.
> 
> Sorry for my bad title, Maybe I should use a good name, sorry!
> 
> > 
> > Joe's patch removed the conflock from the lm_grant callback since the
> > filesystem never set that parameter in the lm_grant callback. This call
> > however has nothing to do with lm_grant. It's done when the client
> > issues a NLM_TEST operation.
> > 
> >>  	}
> >>  	if (block->b_flags & B_QUEUED) {
> >> -		dprintk("lockd: nlmsvc_testlock deferred block %p flags %d fl %p\n",
> >> -			block, block->b_flags, block->b_fl);
> >> +		dprintk("lockd: nlmsvc_testlock deferred block %p flags %d\n",
> >> +			block, block->b_flags);
> >>  		if (block->b_flags & B_TIMED_OUT) {
> >>  			nlmsvc_unlink_block(block);
> >>  			ret = nlm_lck_denied;
> >> @@ -544,14 +535,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> >>  		}
> >>  		if (block->b_flags & B_GOT_CALLBACK) {
> >>  			nlmsvc_unlink_block(block);
> >> -			if (block->b_fl != NULL
> >> -					&& block->b_fl->fl_type != F_UNLCK) {
> >> -				lock->fl = *block->b_fl;
> >> -				goto conf_lock;
> 
> block->b_fl = conf just set an all-zero filed structure to block above,
> and never be updated later.
> If lockd enter here, lock->fl will contains all filed with zero,
> I don't know whether is it OK.
> 
> thanks,
> Kinglong Mee
> 

Not quite....You can end up getting back FILE_LOCK_DEFERRED from an
initial vfs_test_lock request. At that point, a block will be queued
and we'll end up retrying that until the fs comes back. The result of
those retries will end up in b_fl and that's what will end up being
copied to lock->fl.

lockd is one giant Rube Goldberg machine made of baling wire and duct
tape, but it *basically* works and I don't have much inclination to
tinker with it. It's legacy code at this point.

I'd suggest that we take add the patch I proposed earlier since I think
it's basically harmless and should help future-proof changes to this
code.

-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block

[Kinglong Mee]

On 8/20/2014 21:04, Jeff Layton wrote:
> On Wed, 20 Aug 2014 20:10:51 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
> 
>> On 8/20/2014 18:58, Jeff Layton wrote:
>>> On Wed, 20 Aug 2014 18:07:19 +0800
>>> Kinglong Mee <kinglongmee@gmail.com> wrote:
>>>
>>>> Fix left code by Joe Perches's patch,
>>>> "locks: Remove unused conf argument from lm_grant"
>>>>
>>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>>> ---
>>>>  fs/lockd/svclock.c          | 26 +++++---------------------
>>>>  include/linux/lockd/lockd.h |  1 -
>>>>  2 files changed, 5 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
>>>> index 2a61701..796e63b 100644
>>>> --- a/fs/lockd/svclock.c
>>>> +++ b/fs/lockd/svclock.c
>>>> @@ -245,7 +245,6 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
>>>>  	block->b_daemon = rqstp->rq_server;
>>>>  	block->b_host   = host;
>>>>  	block->b_file   = file;
>>>> -	block->b_fl = NULL;
>>>>  	file->f_count++;
>>>>  
>>>>  	/* Add to file's list of blocks */
>>>> @@ -295,7 +294,6 @@ static void nlmsvc_free_block(struct kref *kref)
>>>>  	nlmsvc_freegrantargs(block->b_call);
>>>>  	nlmsvc_release_call(block->b_call);
>>>>  	nlm_release_file(block->b_file);
>>>> -	kfree(block->b_fl);
>>>>  	kfree(block);
>>>>  }
>>>>  
>>>> @@ -523,20 +521,13 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>>>>  	block = nlmsvc_lookup_block(file, lock);
>>>>  
>>>>  	if (block == NULL) {
>>>> -		struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
>>>> -
>>>> -		if (conf == NULL)
>>>> -			return nlm_granted;
>>>>  		block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
>>>> -		if (block == NULL) {
>>>> -			kfree(conf);
>>>> +		if (block == NULL)
>>>>  			return nlm_granted;
>>>> -		}
>>>> -		block->b_fl = conf;
>>>
>>> NAK. The b_fl member is not unused, as is evidenced by the assignment
>>> above.
>>
>> Sorry for my bad title, Maybe I should use a good name, sorry!
>>
>>>
>>> Joe's patch removed the conflock from the lm_grant callback since the
>>> filesystem never set that parameter in the lm_grant callback. This call
>>> however has nothing to do with lm_grant. It's done when the client
>>> issues a NLM_TEST operation.
>>>
>>>>  	}
>>>>  	if (block->b_flags & B_QUEUED) {
>>>> -		dprintk("lockd: nlmsvc_testlock deferred block %p flags %d fl %p\n",
>>>> -			block, block->b_flags, block->b_fl);
>>>> +		dprintk("lockd: nlmsvc_testlock deferred block %p flags %d\n",
>>>> +			block, block->b_flags);
>>>>  		if (block->b_flags & B_TIMED_OUT) {
>>>>  			nlmsvc_unlink_block(block);
>>>>  			ret = nlm_lck_denied;
>>>> @@ -544,14 +535,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>>>>  		}
>>>>  		if (block->b_flags & B_GOT_CALLBACK) {
>>>>  			nlmsvc_unlink_block(block);
>>>> -			if (block->b_fl != NULL
>>>> -					&& block->b_fl->fl_type != F_UNLCK) {
>>>> -				lock->fl = *block->b_fl;
>>>> -				goto conf_lock;
>>
>> block->b_fl = conf just set an all-zero filed structure to block above,
>> and never be updated later.
>> If lockd enter here, lock->fl will contains all filed with zero,
>> I don't know whether is it OK.
>>
>> thanks,
>> Kinglong Mee
>>
> 
> Not quite....You can end up getting back FILE_LOCK_DEFERRED from an
> initial vfs_test_lock request. At that point, a block will be queued
> and we'll end up retrying that until the fs comes back. The result of
> those retries will end up in b_fl and that's what will end up being
> copied to lock->fl.

Yes, that's right.
What I　care is that block->b_fl contains with all zero for all field, 
block->b_fl->fl_type == 0 == F_RDLCK.

For block with b_flags & B_GOT_CALLBACK, block->b_fl will always be non-NULL,
and block->b_fl->fl_type always be F_RDLCK (Cannot be updated after initial),
so that, nlmsvc_testlock will return nlm_lck_denied,
but I think should return nlm_granted.

So, I think commit 5ea0d75037b9 (lockd: handle test_lock deferrals)
introduces the bug. After Joe's patch, we should remove b_fl in struct block.

Cc Marc Eshel

506 __be32
507 nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
508                 struct nlm_host *host, struct nlm_lock *lock,
509                 struct nlm_lock *conflock, struct nlm_cookie *cookie)
510 {
511         struct nlm_block        *block = NULL;
... ...
536         }
537         if (block->b_flags & B_QUEUED) {
538                 dprintk("lockd: nlmsvc_testlock deferred block %p flags %d fl %p\n",
539                         block, block->b_flags, block->b_fl);
540                 if (block->b_flags & B_TIMED_OUT) {
541                         nlmsvc_unlink_block(block);
542                         ret = nlm_lck_denied;
543                         goto out;
544                 }
545                 if (block->b_flags & B_GOT_CALLBACK) {
546                         nlmsvc_unlink_block(block);
547                         if (block->b_fl != NULL
548                                         && block->b_fl->fl_type != F_UNLCK) {
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

549                                 lock->fl = *block->b_fl;
550                                 goto conf_lock;
551                         } else {
552                                 ret = nlm_granted;
553                                 goto out;
554                         }
555                 }

thanks,
Kinglong Mee

Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block

[Jeff Layton]

On Thu, 21 Aug 2014 21:16:27 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:

> On 8/20/2014 21:04, Jeff Layton wrote:
> > On Wed, 20 Aug 2014 20:10:51 +0800
> > Kinglong Mee <kinglongmee@gmail.com> wrote:
> > 
> >> On 8/20/2014 18:58, Jeff Layton wrote:
> >>> On Wed, 20 Aug 2014 18:07:19 +0800
> >>> Kinglong Mee <kinglongmee@gmail.com> wrote:
> >>>
> >>>> Fix left code by Joe Perches's patch,
> >>>> "locks: Remove unused conf argument from lm_grant"
> >>>>
> >>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> >>>> ---
> >>>>  fs/lockd/svclock.c          | 26 +++++---------------------
> >>>>  include/linux/lockd/lockd.h |  1 -
> >>>>  2 files changed, 5 insertions(+), 22 deletions(-)
> >>>>
> >>>> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> >>>> index 2a61701..796e63b 100644
> >>>> --- a/fs/lockd/svclock.c
> >>>> +++ b/fs/lockd/svclock.c
> >>>> @@ -245,7 +245,6 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
> >>>>  	block->b_daemon = rqstp->rq_server;
> >>>>  	block->b_host   = host;
> >>>>  	block->b_file   = file;
> >>>> -	block->b_fl = NULL;
> >>>>  	file->f_count++;
> >>>>  
> >>>>  	/* Add to file's list of blocks */
> >>>> @@ -295,7 +294,6 @@ static void nlmsvc_free_block(struct kref *kref)
> >>>>  	nlmsvc_freegrantargs(block->b_call);
> >>>>  	nlmsvc_release_call(block->b_call);
> >>>>  	nlm_release_file(block->b_file);
> >>>> -	kfree(block->b_fl);
> >>>>  	kfree(block);
> >>>>  }
> >>>>  
> >>>> @@ -523,20 +521,13 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> >>>>  	block = nlmsvc_lookup_block(file, lock);
> >>>>  
> >>>>  	if (block == NULL) {
> >>>> -		struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> >>>> -
> >>>> -		if (conf == NULL)
> >>>> -			return nlm_granted;
> >>>>  		block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> >>>> -		if (block == NULL) {
> >>>> -			kfree(conf);
> >>>> +		if (block == NULL)
> >>>>  			return nlm_granted;
> >>>> -		}
> >>>> -		block->b_fl = conf;
> >>>
> >>> NAK. The b_fl member is not unused, as is evidenced by the assignment
> >>> above.
> >>
> >> Sorry for my bad title, Maybe I should use a good name, sorry!
> >>
> >>>
> >>> Joe's patch removed the conflock from the lm_grant callback since the
> >>> filesystem never set that parameter in the lm_grant callback. This call
> >>> however has nothing to do with lm_grant. It's done when the client
> >>> issues a NLM_TEST operation.
> >>>
> >>>>  	}
> >>>>  	if (block->b_flags & B_QUEUED) {
> >>>> -		dprintk("lockd: nlmsvc_testlock deferred block %p flags %d fl %p\n",
> >>>> -			block, block->b_flags, block->b_fl);
> >>>> +		dprintk("lockd: nlmsvc_testlock deferred block %p flags %d\n",
> >>>> +			block, block->b_flags);
> >>>>  		if (block->b_flags & B_TIMED_OUT) {
> >>>>  			nlmsvc_unlink_block(block);
> >>>>  			ret = nlm_lck_denied;
> >>>> @@ -544,14 +535,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> >>>>  		}
> >>>>  		if (block->b_flags & B_GOT_CALLBACK) {
> >>>>  			nlmsvc_unlink_block(block);
> >>>> -			if (block->b_fl != NULL
> >>>> -					&& block->b_fl->fl_type != F_UNLCK) {
> >>>> -				lock->fl = *block->b_fl;
> >>>> -				goto conf_lock;
> >>
> >> block->b_fl = conf just set an all-zero filed structure to block above,
> >> and never be updated later.
> >> If lockd enter here, lock->fl will contains all filed with zero,
> >> I don't know whether is it OK.
> >>
> >> thanks,
> >> Kinglong Mee
> >>
> > 
> > Not quite....You can end up getting back FILE_LOCK_DEFERRED from an
> > initial vfs_test_lock request. At that point, a block will be queued
> > and we'll end up retrying that until the fs comes back. The result of
> > those retries will end up in b_fl and that's what will end up being
> > copied to lock->fl.
> 
> Yes, that's right.
> What I　care is that block->b_fl contains with all zero for all field, 
> block->b_fl->fl_type == 0 == F_RDLCK.
> 
> For block with b_flags & B_GOT_CALLBACK, block->b_fl will always be non-NULL,
> and block->b_fl->fl_type always be F_RDLCK (Cannot be updated after initial),
> so that, nlmsvc_testlock will return nlm_lck_denied,
> but I think should return nlm_granted.
> 
> So, I think commit 5ea0d75037b9 (lockd: handle test_lock deferrals)
> introduces the bug. After Joe's patch, we should remove b_fl in struct block.
> 
> Cc Marc Eshel
> 
> 506 __be32
> 507 nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> 508                 struct nlm_host *host, struct nlm_lock *lock,
> 509                 struct nlm_lock *conflock, struct nlm_cookie *cookie)
> 510 {
> 511         struct nlm_block        *block = NULL;
> ... ...
> 536         }
> 537         if (block->b_flags & B_QUEUED) {
> 538                 dprintk("lockd: nlmsvc_testlock deferred block %p flags %d fl %p\n",
> 539                         block, block->b_flags, block->b_fl);
> 540                 if (block->b_flags & B_TIMED_OUT) {
> 541                         nlmsvc_unlink_block(block);
> 542                         ret = nlm_lck_denied;
> 543                         goto out;
> 544                 }
> 545                 if (block->b_flags & B_GOT_CALLBACK) {
> 546                         nlmsvc_unlink_block(block);
> 547                         if (block->b_fl != NULL
> 548                                         && block->b_fl->fl_type != F_UNLCK) {
>                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> 549                                 lock->fl = *block->b_fl;
> 550                                 goto conf_lock;
> 551                         } else {
> 552                                 ret = nlm_granted;
> 553                                 goto out;
> 554                         }
> 555                 }
> 
> thanks,
> Kinglong Mee

Yeah, that certainly looks wrong, and now that I look I don't see where
the callback code touches b_fl at all. Maybe you're right here...

Furthermore, I don't see how you can get FILE_LOCK_DEFERRED in this
codepath at all. The generic locking code will only send that back if
FL_SLEEP is set in the request (and it isn't here).

The DLM code just looks broken. It never returns FILE_LOCK_DEFERRED in
the GETLK codepath and instead ignores FL_SLEEP, does a blocking upcall
and waits on the reply. That likely makes lockd stall out regularly...

Wonder if there are any out of tree filesystems that rely on this? GPFS
maybe?

Ok, I'm sold. I'll take your patch and let it stew in linux-next for a
bit, and we can look at merging it for v3.18.

-- 
Jeff Layton <jlayton@primarydata.com>