lockd: Remove unused b_fl member from struct nlm_block

PATCH In review Kinglong Mee 2 participants 7 messages Aug 20, 2014 – Aug 21, 2014

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Kinglong Mee

author

2014-08-20 10:07 UTC #0 lore raw

PATCH 1/1 lockd: Remove unused b_fl member from struct nlm_block

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

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/lockd/svclock.c          | 26 +++++---------------------
 include/linux/lockd/lockd.h |  1 -
 2 files changed, 5 insertions(+), 22 deletions(-)

fs/lockd/svclock.c +5 ~~-21~~

index 2a61701..796e63b 100644
@@ -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);

include/linux/lockd/lockd.h +0 -1

index 219d796..ff82a32 100644
@@ -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 */

Signature · 1 line

1.9.3

Jeff Layton

2014-08-20 10:58 UTC #1 lore raw

Quoted 34 lines · On Wed, 20 Aug 2014 18:07:19 +0800 Kinglong Mee <[email protected]> wrote:

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

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/lockd/svclock.c          | 26 +++++---------------------
 include/linux/lockd/lockd.h |  1 -
 2 files changed, 5 insertions(+), 22 deletions(-)

fs/lockd/svclock.c +0 -3

index 2a61701..796e63b 100644
@@ -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);

fs/lockd/svclock.c nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, · line 535

Quoted 10 lines

fs/lockd/svclock.c +1 -7

-
-		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.

Quoted 46 lines

fs/lockd/svclock.c +4 ~~-11~~

 	}
 	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);

include/linux/lockd/lockd.h +0 -1

index 219d796..ff82a32 100644
@@ -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 */

Signature · 1 line

Jeff Layton <[email protected]>

Kinglong Mee

author

2014-08-20 12:11 UTC #2 lore raw

Quoted 35 lines · Jeff Layton

Quoted 32 lines · On Wed, 20 Aug 2014 18:07:19 +0800 Kinglong Mee <[email protected]> wrote:

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

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/lockd/svclock.c          | 26 +++++---------------------
 include/linux/lockd/lockd.h |  1 -
 2 files changed, 5 insertions(+), 22 deletions(-)

fs/lockd/svclock.c +0 -2

index 2a61701..796e63b 100644
@@ -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);

Quoted 15 lines

fs/lockd/svclock.c nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, · line 535
Quoted 12 lines
fs/lockd/svclock.c +1 -8
 	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!

Quoted 15 lines

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.
Quoted 9 lines
fs/lockd/svclock.c +2 -2
 	}
 	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;

Quoted 8 lines

fs/lockd/svclock.c nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, · line 550

Quoted 8 lines

fs/lockd/svclock.c +0 -4

@@ -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

Quoted 31 lines

Quoted 29 lines

fs/lockd/svclock.c +2 -5

-			} 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);

include/linux/lockd/lockd.h +0 -1

index 219d796..ff82a32 100644
@@ -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

2014-08-20 13:04 UTC #3 lore raw

Quoted 83 lines · On Wed, 20 Aug 2014 20:10:51 +0800 Kinglong Mee <[email protected]> wrote:

Quoted 35 lines · Jeff Layton
Quoted 32 lines · On Wed, 20 Aug 2014 18:07:19 +0800 Kinglong Mee <[email protected]> wrote:
Fix left code by Joe Perches's patch, "locks: Remove unused conf argument from lm_grant"
Signed-off-by Kinglong Mee <[email protected]>
---
 fs/lockd/svclock.c          | 26 +++++---------------------
 include/linux/lockd/lockd.h |  1 -
 2 files changed, 5 insertions(+), 22 deletions(-)
fs/lockd/svclock.c +0 -2
index 2a61701..796e63b 100644
@@ -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);
 
Quoted 15 lines
fs/lockd/svclock.c nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, · line 535
Quoted 12 lines
fs/lockd/svclock.c +1 -8
 	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!
Quoted 15 lines
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.
Quoted 9 lines
fs/lockd/svclock.c +2 -2
 	}
 	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;
Quoted 8 lines
fs/lockd/svclock.c nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, · line 550
Quoted 8 lines
fs/lockd/svclock.c +0 -4
@@ -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.

Quoted 3 lines

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.

Signature · 1 line

Jeff Layton <[email protected]>

Kinglong Mee

author

2014-08-21 13:17 UTC #4 lore raw

Quoted 90 lines · Jeff Layton

Quoted 83 lines · On Wed, 20 Aug 2014 20:10:51 +0800 Kinglong Mee <[email protected]> wrote:
Quoted 35 lines · Jeff Layton
Quoted 32 lines · On Wed, 20 Aug 2014 18:07:19 +0800 Kinglong Mee <[email protected]> wrote:
Fix left code by Joe Perches's patch, "locks: Remove unused conf argument from lm_grant"
Signed-off-by Kinglong Mee <[email protected]>
---
 fs/lockd/svclock.c          | 26 +++++---------------------
 include/linux/lockd/lockd.h |  1 -
 2 files changed, 5 insertions(+), 22 deletions(-)
fs/lockd/svclock.c +0 -2
index 2a61701..796e63b 100644
@@ -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);
 
Quoted 15 lines
fs/lockd/svclock.c nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, · line 535
Quoted 12 lines
fs/lockd/svclock.c +1 -8
 	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!
Quoted 15 lines
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.
Quoted 9 lines
fs/lockd/svclock.c +2 -2
 	}
 	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;
Quoted 8 lines
fs/lockd/svclock.c nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, · line 550
Quoted 8 lines
fs/lockd/svclock.c +0 -4
@@ -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.
Quoted 3 lines
thanks,
Kinglong Mee

Quoted 5 lines

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

Jeff Layton

2014-08-21 14:02 UTC #5 lore raw

Quoted 141 lines · On Thu, 21 Aug 2014 21:16:27 +0800 Kinglong Mee <[email protected]> wrote:

Quoted 90 lines · Jeff Layton
Quoted 83 lines · On Wed, 20 Aug 2014 20:10:51 +0800 Kinglong Mee <[email protected]> wrote:
Quoted 35 lines · Jeff Layton
Quoted 32 lines · On Wed, 20 Aug 2014 18:07:19 +0800 Kinglong Mee <[email protected]> wrote:
Fix left code by Joe Perches's patch, "locks: Remove unused conf argument from lm_grant"
Signed-off-by Kinglong Mee <[email protected]>
---
 fs/lockd/svclock.c          | 26 +++++---------------------
 include/linux/lockd/lockd.h |  1 -
 2 files changed, 5 insertions(+), 22 deletions(-)
fs/lockd/svclock.c +0 -2
index 2a61701..796e63b 100644
@@ -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);
 
Quoted 15 lines
fs/lockd/svclock.c nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, · line 535
Quoted 12 lines
fs/lockd/svclock.c +1 -8
 	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!
Quoted 15 lines
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.
Quoted 9 lines
fs/lockd/svclock.c +2 -2
 	}
 	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;
Quoted 8 lines
fs/lockd/svclock.c nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, · line 550
Quoted 8 lines
fs/lockd/svclock.c +0 -4
@@ -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.
Quoted 3 lines
thanks,
Kinglong Mee
Quoted 5 lines
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 }

Quoted 2 lines

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.

Signature · 1 line

Jeff Layton <[email protected]>

Jeff Layton

2014-08-20 11:16 UTC #6 lore raw

In reply to Kinglong Mee

Quoted 30 lines · On Wed, 20 Aug 2014 18:07:19 +0800 Kinglong Mee <[email protected]> wrote:

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

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/lockd/svclock.c          | 26 +++++---------------------
 include/linux/lockd/lockd.h |  1 -
 2 files changed, 5 insertions(+), 22 deletions(-)

fs/lockd/svclock.c +0 -2

index 2a61701..796e63b 100644
@@ -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);
 }

fs/lockd/svclock.c nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, · line 527

Quoted 5 lines

fs/lockd/svclock.c +0 -2

@@ -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 <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

---
 fs/lockd/svclock.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

fs/lockd/svclock.c +4 -3

index 2a6170133c1d..1eb2ae47e6b1 100644
@@ -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;

Signature · 1 line

1.9.3