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

From: Jeff Layton <jeff.layton@primarydata.com>
To: Kinglong Mee <kinglongmee@gmail.com>
Cc: Jeff Layton <jeff.layton@primarydata.com>,
	"J. Bruce Fields" <bfields@fieldses.org>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
	Joe Perches <joe@perches.com>
Subject: Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block
Date: Wed, 20 Aug 2014 09:04:12 -0400	[thread overview]
Message-ID: <20140820090412.309b7599@tlielax.poochiereds.net> (raw)
In-Reply-To: <53F4904B.6080204@gmail.com>

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>