On Wed, 20 Aug 2014 20:10:51 +0800
Kinglong Mee <kinglongmee@xxxxxxxxx> wrote:
> On 8/20/2014 18:58, Jeff Layton wrote:
> > On Wed, 20 Aug 2014 18:07:19 +0800
> > Kinglong Mee <kinglongmee@xxxxxxxxx> wrote:
> >
> >> Fix left code by Joe Perches's patch,
> >> "locks: Remove unused conf argument from lm_grant"
> >>
> >> Signed-off-by: Kinglong Mee <kinglongmee@xxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
