* [PATCH] lockd: Remove unused b_fl member from struct nlm_block @ 2014-08-20 10:07 Kinglong Mee 2014-08-20 10:58 ` Jeff Layton 2014-08-20 11:16 ` Jeff Layton 0 siblings, 2 replies; 7+ messages in thread From: Kinglong Mee @ 2014-08-20 10:07 UTC (permalink / raw) To: Jeff Layton, J. Bruce Fields, kinglongmee Cc: Linux NFS Mailing List, Joe Perches 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block 2014-08-20 10:07 [PATCH] lockd: Remove unused b_fl member from struct nlm_block Kinglong Mee @ 2014-08-20 10:58 ` Jeff Layton 2014-08-20 12:10 ` Kinglong Mee 2014-08-20 11:16 ` Jeff Layton 1 sibling, 1 reply; 7+ messages in thread From: Jeff Layton @ 2014-08-20 10:58 UTC (permalink / raw) To: Kinglong Mee Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List, Joe Perches 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block 2014-08-20 10:58 ` Jeff Layton @ 2014-08-20 12:10 ` Kinglong Mee 2014-08-20 13:04 ` Jeff Layton 0 siblings, 1 reply; 7+ messages in thread From: Kinglong Mee @ 2014-08-20 12:10 UTC (permalink / raw) To: Jeff Layton Cc: J. Bruce Fields, Linux NFS Mailing List, Joe Perches, 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 */ > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block 2014-08-20 12:10 ` Kinglong Mee @ 2014-08-20 13:04 ` Jeff Layton 2014-08-21 13:16 ` Kinglong Mee 0 siblings, 1 reply; 7+ messages in thread From: Jeff Layton @ 2014-08-20 13:04 UTC (permalink / raw) To: Kinglong Mee Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List, Joe Perches 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block 2014-08-20 13:04 ` Jeff Layton @ 2014-08-21 13:16 ` Kinglong Mee 2014-08-21 14:02 ` Jeff Layton 0 siblings, 1 reply; 7+ messages in thread From: Kinglong Mee @ 2014-08-21 13:16 UTC (permalink / raw) To: Jeff Layton Cc: J. Bruce Fields, Linux NFS Mailing List, Joe Perches, Kinglong Mee, eshel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block 2014-08-21 13:16 ` Kinglong Mee @ 2014-08-21 14:02 ` Jeff Layton 0 siblings, 0 replies; 7+ messages in thread From: Jeff Layton @ 2014-08-21 14:02 UTC (permalink / raw) To: Kinglong Mee Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List, Joe Perches, eshel 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lockd: Remove unused b_fl member from struct nlm_block 2014-08-20 10:07 [PATCH] lockd: Remove unused b_fl member from struct nlm_block Kinglong Mee 2014-08-20 10:58 ` Jeff Layton @ 2014-08-20 11:16 ` Jeff Layton 1 sibling, 0 replies; 7+ messages in thread From: Jeff Layton @ 2014-08-20 11:16 UTC (permalink / raw) To: Kinglong Mee Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List, Joe Perches 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-08-21 14:02 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-20 10:07 [PATCH] lockd: Remove unused b_fl member from struct nlm_block Kinglong Mee 2014-08-20 10:58 ` Jeff Layton 2014-08-20 12:10 ` Kinglong Mee 2014-08-20 13:04 ` Jeff Layton 2014-08-21 13:16 ` Kinglong Mee 2014-08-21 14:02 ` Jeff Layton 2014-08-20 11:16 ` Jeff Layton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).