[Cluster-devel] [PATCH 0/9] fs.h/dlm: Neatening and argument removals

[Cluster-devel] [PATCH 0/9] fs.h/dlm: Neatening and argument removals

[Joe Perches]

fs.h is an important file read by many.

Because of its age and the number of people that have modified it,
it's not particularly kernel coding style conformant.

Make it more so.

dlm has an unnecessary always NULL argument passed around.
Remove it along with some other neatenings while adding
function prototype argument descriptions.

Joe Perches (9):
  fs.h: Remove unnecessary extern prototypes
  fs.h: Whitespace neatening
  fs.h: A few more whitespace neatenings
  fs.h: Add argument names to struct file_lock_operations (*funcs)
  fs.h: Add member argument descriptions to lock_manager_operations
  dlm: plock: Add argument descriptions to notiy
  dlm: fs: Remove unused conf from lm_grant
  dlm: plock: Reduce indentation by rearranging order
  fs: dlm: lockd: Convert int result to unsigned char type

 fs/dlm/plock.c     |   34 +-
 fs/lockd/svclock.c |   16 +-
 include/linux/fs.h | 1201 ++++++++++++++++++++++++++--------------------------
 3 files changed, 627 insertions(+), 624 deletions(-)

-- 
1.8.1.2.459.gbcd45b4.dirty

[Cluster-devel] [PATCH 6/9] dlm: plock: Add argument descriptions to notify

[Joe Perches]

Function pointer arguments without type names
are not very clear.  Add them.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/dlm/plock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
index f704458..e59d332 100644
--- a/fs/dlm/plock.c
+++ b/fs/dlm/plock.c
@@ -30,7 +30,7 @@ struct plock_op {
 
 struct plock_xop {
 	struct plock_op xop;
-	void *callback;
+	int (*callback)(struct file_lock *, struct file_lock *, int);
 	void *fl;
 	void *file;
 	struct file_lock flc;
@@ -190,7 +190,7 @@ static int dlm_plock_callback(struct plock_op *op)
 	struct file *file;
 	struct file_lock *fl;
 	struct file_lock *flc;
-	int (*notify)(void *, void *, int) = NULL;
+	int (*notify)(struct file_lock *fl, struct file_lock *cont, int result) = NULL;
 	struct plock_xop *xop = (struct plock_xop *)op;
 	int rv = 0;
 
-- 
1.8.1.2.459.gbcd45b4.dirty

[Cluster-devel] [PATCH 7/9] dlm: fs: Remove unused conf from lm_grant

[Joe Perches]

This argument is always NULL so don't pass it around.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/dlm/plock.c     |  8 ++++----
 fs/lockd/svclock.c | 12 +++---------
 include/linux/fs.h |  2 +-
 3 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
index e59d332..e0ab3a9 100644
--- a/fs/dlm/plock.c
+++ b/fs/dlm/plock.c
@@ -30,7 +30,7 @@ struct plock_op {
 
 struct plock_xop {
 	struct plock_op xop;
-	int (*callback)(struct file_lock *, struct file_lock *, int);
+	int (*callback)(struct file_lock *fl, int result);
 	void *fl;
 	void *file;
 	struct file_lock flc;
@@ -190,7 +190,7 @@ static int dlm_plock_callback(struct plock_op *op)
 	struct file *file;
 	struct file_lock *fl;
 	struct file_lock *flc;
-	int (*notify)(struct file_lock *fl, struct file_lock *cont, int result) = NULL;
+	int (*notify)(struct file_lock *fl, int result) = NULL;
 	struct plock_xop *xop = (struct plock_xop *)op;
 	int rv = 0;
 
@@ -209,7 +209,7 @@ static int dlm_plock_callback(struct plock_op *op)
 	notify = xop->callback;
 
 	if (op->info.rv) {
-		notify(fl, NULL, op->info.rv);
+		notify(fl, op->info.rv);
 		goto out;
 	}
 
@@ -228,7 +228,7 @@ static int dlm_plock_callback(struct plock_op *op)
 			  (unsigned long long)op->info.number, file, fl);
 	}
 
-	rv = notify(fl, NULL, 0);
+	rv = notify(fl, 0);
 	if (rv) {
 		/* XXX: We need to cancel the fs lock here: */
 		log_print("dlm_plock_callback: lock granted after lock request "
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index ab798a8..2a61701 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -667,22 +667,16 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
  * deferred rpc for GETLK and SETLK.
  */
 static void
-nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf,
-			     int result)
+nlmsvc_update_deferred_block(struct nlm_block *block, int result)
 {
 	block->b_flags |= B_GOT_CALLBACK;
 	if (result == 0)
 		block->b_granted = 1;
 	else
 		block->b_flags |= B_TIMED_OUT;
-	if (conf) {
-		if (block->b_fl)
-			__locks_copy_lock(block->b_fl, conf);
-	}
 }
 
-static int nlmsvc_grant_deferred(struct file_lock *fl, struct file_lock *conf,
-					int result)
+static int nlmsvc_grant_deferred(struct file_lock *fl, int result)
 {
 	struct nlm_block *block;
 	int rc = -ENOENT;
@@ -697,7 +691,7 @@ static int nlmsvc_grant_deferred(struct file_lock *fl, struct file_lock *conf,
 					rc = -ENOLCK;
 					break;
 				}
-				nlmsvc_update_deferred_block(block, conf, result);
+				nlmsvc_update_deferred_block(block, result);
 			} else if (result == 0)
 				block->b_granted = 1;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ede4be8..d083a67 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -842,7 +842,7 @@ struct lock_manager_operations {
 	int (*lm_compare_owner)(struct file_lock *fl1, struct file_lock *fl2);
 	unsigned long (*lm_owner_key)(struct file_lock *fl);
 	void (*lm_notify)(struct file_lock *fl);	/* unblock callback */
-	int (*lm_grant)(struct file_lock *fl, struct file_lock *conf, int result);
+	int (*lm_grant)(struct file_lock *fl, int result);
 	void (*lm_break)(struct file_lock *fl);
 	int (*lm_change)(struct file_lock **fl, int type);
 };
-- 
1.8.1.2.459.gbcd45b4.dirty

[Cluster-devel] [PATCH 8/9] dlm: plock: Reduce indentation by rearranging order

[Joe Perches]

if blocks that have a goto at the end of one branch can be
simplified by reordering and unindenting.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/dlm/plock.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
index e0ab3a9..3e0b6fc 100644
--- a/fs/dlm/plock.c
+++ b/fs/dlm/plock.c
@@ -144,23 +144,23 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
 
 	send_op(op);
 
-	if (xop->callback == NULL) {
-		rv = wait_event_killable(recv_wq, (op->done != 0));
-		if (rv == -ERESTARTSYS) {
-			log_debug(ls, "dlm_posix_lock: wait killed %llx",
-				  (unsigned long long)number);
-			spin_lock(&ops_lock);
-			list_del(&op->list);
-			spin_unlock(&ops_lock);
-			kfree(xop);
-			do_unlock_close(ls, number, file, fl);
-			goto out;
-		}
-	} else {
+	if (xop->callback) {
 		rv = FILE_LOCK_DEFERRED;
 		goto out;
 	}
 
+	rv = wait_event_killable(recv_wq, (op->done != 0));
+	if (rv == -ERESTARTSYS) {
+		log_debug(ls, "dlm_posix_lock: wait killed %llx",
+			  (unsigned long long)number);
+		spin_lock(&ops_lock);
+		list_del(&op->list);
+		spin_unlock(&ops_lock);
+		kfree(xop);
+		do_unlock_close(ls, number, file, fl);
+		goto out;
+	}
+
 	spin_lock(&ops_lock);
 	if (!list_empty(&op->list)) {
 		log_error(ls, "dlm_posix_lock: op on list %llx",
-- 
1.8.1.2.459.gbcd45b4.dirty

[Cluster-devel] [PATCH 9/9] fs: dlm: lockd: Convert int result to unsigned char type

[Joe Perches]

op->info.rv is an s32, but it's only used as a u8.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/dlm/plock.c     |  6 +++---
 fs/lockd/svclock.c | 10 +++++-----
 include/linux/fs.h |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
index 3e0b6fc..267849d 100644
--- a/fs/dlm/plock.c
+++ b/fs/dlm/plock.c
@@ -30,7 +30,7 @@ struct plock_op {
 
 struct plock_xop {
 	struct plock_op xop;
-	int (*callback)(struct file_lock *fl, int result);
+	int (*callback)(struct file_lock *fl, unsigned char type);
 	void *fl;
 	void *file;
 	struct file_lock flc;
@@ -190,7 +190,7 @@ static int dlm_plock_callback(struct plock_op *op)
 	struct file *file;
 	struct file_lock *fl;
 	struct file_lock *flc;
-	int (*notify)(struct file_lock *fl, int result) = NULL;
+	int (*notify)(struct file_lock *fl, unsigned char type) = NULL;
 	struct plock_xop *xop = (struct plock_xop *)op;
 	int rv = 0;
 
@@ -209,7 +209,7 @@ static int dlm_plock_callback(struct plock_op *op)
 	notify = xop->callback;
 
 	if (op->info.rv) {
-		notify(fl, op->info.rv);
+		notify(fl, (unsigned char)op->info.rv);
 		goto out;
 	}
 
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 2a61701..15532b9 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -667,16 +667,16 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
  * deferred rpc for GETLK and SETLK.
  */
 static void
-nlmsvc_update_deferred_block(struct nlm_block *block, int result)
+nlmsvc_update_deferred_block(struct nlm_block *block, unsigned char type)
 {
 	block->b_flags |= B_GOT_CALLBACK;
-	if (result == 0)
+	if (type == 0)
 		block->b_granted = 1;
 	else
 		block->b_flags |= B_TIMED_OUT;
 }
 
-static int nlmsvc_grant_deferred(struct file_lock *fl, int result)
+static int nlmsvc_grant_deferred(struct file_lock *fl, unsigned char type)
 {
 	struct nlm_block *block;
 	int rc = -ENOENT;
@@ -691,8 +691,8 @@ static int nlmsvc_grant_deferred(struct file_lock *fl, int result)
 					rc = -ENOLCK;
 					break;
 				}
-				nlmsvc_update_deferred_block(block, result);
-			} else if (result == 0)
+				nlmsvc_update_deferred_block(block, type);
+			} else if (type == 0)
 				block->b_granted = 1;
 
 			nlmsvc_insert_block_locked(block, 0);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d083a67..7fbce66 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -842,7 +842,7 @@ struct lock_manager_operations {
 	int (*lm_compare_owner)(struct file_lock *fl1, struct file_lock *fl2);
 	unsigned long (*lm_owner_key)(struct file_lock *fl);
 	void (*lm_notify)(struct file_lock *fl);	/* unblock callback */
-	int (*lm_grant)(struct file_lock *fl, int result);
+	int (*lm_grant)(struct file_lock *fl, unsigned char type);
 	void (*lm_break)(struct file_lock *fl);
 	int (*lm_change)(struct file_lock **fl, int type);
 };
-- 
1.8.1.2.459.gbcd45b4.dirty

[Cluster-devel] [PATCH 7/9] dlm: fs: Remove unused conf from lm_grant

[Jeff Layton]

On Sun, 20 Jul 2014 11:23:41 -0700
Joe Perches <joe@perches.com> wrote:

> This argument is always NULL so don't pass it around.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/dlm/plock.c     |  8 ++++----
>  fs/lockd/svclock.c | 12 +++---------
>  include/linux/fs.h |  2 +-
>  3 files changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
> index e59d332..e0ab3a9 100644
> --- a/fs/dlm/plock.c
> +++ b/fs/dlm/plock.c
> @@ -30,7 +30,7 @@ struct plock_op {
>  
>  struct plock_xop {
>  	struct plock_op xop;
> -	int (*callback)(struct file_lock *, struct file_lock *, int);
> +	int (*callback)(struct file_lock *fl, int result);
>  	void *fl;
>  	void *file;
>  	struct file_lock flc;
> @@ -190,7 +190,7 @@ static int dlm_plock_callback(struct plock_op *op)
>  	struct file *file;
>  	struct file_lock *fl;
>  	struct file_lock *flc;
> -	int (*notify)(struct file_lock *fl, struct file_lock *cont, int result) = NULL;
> +	int (*notify)(struct file_lock *fl, int result) = NULL;
>  	struct plock_xop *xop = (struct plock_xop *)op;
>  	int rv = 0;
>  
> @@ -209,7 +209,7 @@ static int dlm_plock_callback(struct plock_op *op)
>  	notify = xop->callback;
>  
>  	if (op->info.rv) {
> -		notify(fl, NULL, op->info.rv);
> +		notify(fl, op->info.rv);
>  		goto out;
>  	}
>  
> @@ -228,7 +228,7 @@ static int dlm_plock_callback(struct plock_op *op)
>  			  (unsigned long long)op->info.number, file, fl);
>  	}
>  
> -	rv = notify(fl, NULL, 0);
> +	rv = notify(fl, 0);
>  	if (rv) {
>  		/* XXX: We need to cancel the fs lock here: */
>  		log_print("dlm_plock_callback: lock granted after lock request "
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index ab798a8..2a61701 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -667,22 +667,16 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
>   * deferred rpc for GETLK and SETLK.
>   */
>  static void
> -nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf,
> -			     int result)
> +nlmsvc_update_deferred_block(struct nlm_block *block, int result)
>  {
>  	block->b_flags |= B_GOT_CALLBACK;
>  	if (result == 0)
>  		block->b_granted = 1;
>  	else
>  		block->b_flags |= B_TIMED_OUT;
> -	if (conf) {
> -		if (block->b_fl)
> -			__locks_copy_lock(block->b_fl, conf);
> -	}
>  }
>  
> -static int nlmsvc_grant_deferred(struct file_lock *fl, struct file_lock *conf,
> -					int result)
> +static int nlmsvc_grant_deferred(struct file_lock *fl, int result)
>  {
>  	struct nlm_block *block;
>  	int rc = -ENOENT;
> @@ -697,7 +691,7 @@ static int nlmsvc_grant_deferred(struct file_lock *fl, struct file_lock *conf,
>  					rc = -ENOLCK;
>  					break;
>  				}
> -				nlmsvc_update_deferred_block(block, conf, result);
> +				nlmsvc_update_deferred_block(block, result);
>  			} else if (result == 0)
>  				block->b_granted = 1;
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ede4be8..d083a67 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -842,7 +842,7 @@ struct lock_manager_operations {
>  	int (*lm_compare_owner)(struct file_lock *fl1, struct file_lock *fl2);
>  	unsigned long (*lm_owner_key)(struct file_lock *fl);
>  	void (*lm_notify)(struct file_lock *fl);	/* unblock callback */
> -	int (*lm_grant)(struct file_lock *fl, struct file_lock *conf, int result);
> +	int (*lm_grant)(struct file_lock *fl, int result);
>  	void (*lm_break)(struct file_lock *fl);
>  	int (*lm_change)(struct file_lock **fl, int type);
>  };

Looks good to me. Nice cleanup.

Reviewed-by: Jeff Layton <jlayton@poochiereds.net>

[Cluster-devel] [PATCH 8/9] dlm: plock: Reduce indentation by rearranging order

[Jeff Layton]

On Sun, 20 Jul 2014 11:23:42 -0700
Joe Perches <joe@perches.com> wrote:

> if blocks that have a goto at the end of one branch can be
> simplified by reordering and unindenting.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/dlm/plock.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
> index e0ab3a9..3e0b6fc 100644
> --- a/fs/dlm/plock.c
> +++ b/fs/dlm/plock.c
> @@ -144,23 +144,23 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
>  
>  	send_op(op);
>  
> -	if (xop->callback == NULL) {
> -		rv = wait_event_killable(recv_wq, (op->done != 0));
> -		if (rv == -ERESTARTSYS) {
> -			log_debug(ls, "dlm_posix_lock: wait killed %llx",
> -				  (unsigned long long)number);
> -			spin_lock(&ops_lock);
> -			list_del(&op->list);
> -			spin_unlock(&ops_lock);
> -			kfree(xop);
> -			do_unlock_close(ls, number, file, fl);
> -			goto out;
> -		}
> -	} else {
> +	if (xop->callback) {
>  		rv = FILE_LOCK_DEFERRED;
>  		goto out;
>  	}
>  
> +	rv = wait_event_killable(recv_wq, (op->done != 0));
> +	if (rv == -ERESTARTSYS) {
> +		log_debug(ls, "dlm_posix_lock: wait killed %llx",
> +			  (unsigned long long)number);
> +		spin_lock(&ops_lock);
> +		list_del(&op->list);
> +		spin_unlock(&ops_lock);
> +		kfree(xop);
> +		do_unlock_close(ls, number, file, fl);
> +		goto out;
> +	}
> +
>  	spin_lock(&ops_lock);
>  	if (!list_empty(&op->list)) {
>  		log_error(ls, "dlm_posix_lock: op on list %llx",

Looks right.

Reviewed-by: Jeff Layton <jlayton@poochiereds.net>

[Cluster-devel] [PATCH 9/9] fs: dlm: lockd: Convert int result to unsigned char type

[Jeff Layton]

On Sun, 20 Jul 2014 11:23:43 -0700
Joe Perches <joe@perches.com> wrote:

> op->info.rv is an s32, but it's only used as a u8.
> 

I don't understand this patch. info.rv is s32 (and I assume that "rv"
stands for "return value"). What I don't get is why you think it's just
used as a u8. It seems to be used more like a bool than anything else,
and I'm not sure that "type" is really a good description for it. Maybe
it should be a "bool" and named "conflict", given the comments in
dlm_posix_get ?

> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/dlm/plock.c     |  6 +++---
>  fs/lockd/svclock.c | 10 +++++-----
>  include/linux/fs.h |  2 +-
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
> index 3e0b6fc..267849d 100644
> --- a/fs/dlm/plock.c
> +++ b/fs/dlm/plock.c
> @@ -30,7 +30,7 @@ struct plock_op {
>  
>  struct plock_xop {
>  	struct plock_op xop;
> -	int (*callback)(struct file_lock *fl, int result);
> +	int (*callback)(struct file_lock *fl, unsigned char type);
>  	void *fl;
>  	void *file;
>  	struct file_lock flc;
> @@ -190,7 +190,7 @@ static int dlm_plock_callback(struct plock_op *op)
>  	struct file *file;
>  	struct file_lock *fl;
>  	struct file_lock *flc;
> -	int (*notify)(struct file_lock *fl, int result) = NULL;
> +	int (*notify)(struct file_lock *fl, unsigned char type) = NULL;
>  	struct plock_xop *xop = (struct plock_xop *)op;
>  	int rv = 0;
>  
> @@ -209,7 +209,7 @@ static int dlm_plock_callback(struct plock_op *op)
>  	notify = xop->callback;
>  
>  	if (op->info.rv) {
> -		notify(fl, op->info.rv);
> +		notify(fl, (unsigned char)op->info.rv);
>  		goto out;
>  	}
>  
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 2a61701..15532b9 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -667,16 +667,16 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
>   * deferred rpc for GETLK and SETLK.
>   */
>  static void
> -nlmsvc_update_deferred_block(struct nlm_block *block, int result)
> +nlmsvc_update_deferred_block(struct nlm_block *block, unsigned char type)
>  {
>  	block->b_flags |= B_GOT_CALLBACK;
> -	if (result == 0)
> +	if (type == 0)
>  		block->b_granted = 1;
>  	else
>  		block->b_flags |= B_TIMED_OUT;
>  }
>  
> -static int nlmsvc_grant_deferred(struct file_lock *fl, int result)
> +static int nlmsvc_grant_deferred(struct file_lock *fl, unsigned char type)
>  {
>  	struct nlm_block *block;
>  	int rc = -ENOENT;
> @@ -691,8 +691,8 @@ static int nlmsvc_grant_deferred(struct file_lock *fl, int result)
>  					rc = -ENOLCK;
>  					break;
>  				}
> -				nlmsvc_update_deferred_block(block, result);
> -			} else if (result == 0)
> +				nlmsvc_update_deferred_block(block, type);
> +			} else if (type == 0)
>  				block->b_granted = 1;
>  
>  			nlmsvc_insert_block_locked(block, 0);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index d083a67..7fbce66 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -842,7 +842,7 @@ struct lock_manager_operations {
>  	int (*lm_compare_owner)(struct file_lock *fl1, struct file_lock *fl2);
>  	unsigned long (*lm_owner_key)(struct file_lock *fl);
>  	void (*lm_notify)(struct file_lock *fl);	/* unblock callback */
> -	int (*lm_grant)(struct file_lock *fl, int result);
> +	int (*lm_grant)(struct file_lock *fl, unsigned char type);
>  	void (*lm_break)(struct file_lock *fl);
>  	int (*lm_change)(struct file_lock **fl, int type);
>  };


-- 
Jeff Layton <jlayton@poochiereds.net>

[Cluster-devel] [PATCH 9/9] fs: dlm: lockd: Convert int result to unsigned char type

[David Teigland]

On Wed, Jul 23, 2014 at 02:11:39PM -0400, Jeff Layton wrote:
> On Sun, 20 Jul 2014 11:23:43 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > op->info.rv is an s32, but it's only used as a u8.
> > 
> 
> I don't understand this patch. info.rv is s32 (and I assume that "rv"
> stands for "return value"). What I don't get is why you think it's just
> used as a u8. It seems to be used more like a bool than anything else,

Thank you, Jeff.

        /* info.rv from userspace is 1 for conflict, 0 for no-conflict,
           -ENOENT if there are no locks on the file */

        rv = op->info.rv;

> and I'm not sure that "type" is really a good description for it. Maybe
> it should be a "bool" and named "conflict", given the comments in
> dlm_posix_get ?

type is not a good name.

Sorry Joe, I'm not a fan of your patches.

Dave

[Cluster-devel] [PATCH 9/9] fs: dlm: lockd: Convert int result to unsigned char type

[Joe Perches]

On Wed, 2014-07-23 at 14:11 -0400, Jeff Layton wrote:
> On Sun, 20 Jul 2014 11:23:43 -0700 Joe Perches <joe@perches.com> wrote:
> > op->info.rv is an s32, but it's only used as a u8.
> I don't understand this patch. info.rv is s32 (and I assume that "rv"
> stands for "return value").

In this case it's not a return value but an input.

> What I don't get is why you think it's just
> used as a u8.

Because it's tested only in nlmsvc_grant_deferred
and nlmsvc_update_deferred_block against 0.

As far as I can tell, it's not possible to set it
to a negative value.

> It seems to be used more like a bool than anything else,
> and I'm not sure that "type" is really a good description for it. Maybe
> it should be a "bool" and named "conflict",

Maybe.  But it seemed likely and possible to expand
it from a single bool to a value.

> given the comments in dlm_posix_get ?

Maybe, though I don't see how the comments relate to
this change.  The rv value returned from that call
is either -ENOMEM or 0 and is unchanged by this patch.

> > diff --git a/include/linux/fs.h b/include/linux/fs.h
[]
> > @@ -842,7 +842,7 @@ struct lock_manager_operations {
> >  	int (*lm_compare_owner)(struct file_lock *fl1, struct file_lock *fl2);
> >  	unsigned long (*lm_owner_key)(struct file_lock *fl);
> >  	void (*lm_notify)(struct file_lock *fl);	/* unblock callback */
> > -	int (*lm_grant)(struct file_lock *fl, int result);
> > +	int (*lm_grant)(struct file_lock *fl, unsigned char type);
> >  	void (*lm_break)(struct file_lock *fl);
> >  	int (*lm_change)(struct file_lock **fl, int type);
> >  };

I used variable name "type" because that's what
lm_change uses.  No worries if you think a name
like conflict is better.

The only in-kernel setter of lm_grant is:

fs/lockd/svclock.c:     .lm_grant = nlmsvc_grant_deferred,

and for that, I think using a variable name of
"result" is misleading at best.

[Cluster-devel] [PATCH 9/9] fs: dlm: lockd: Convert int result to unsigned char type

[Jeff Layton]

On Wed, 23 Jul 2014 20:53:59 -0700
Joe Perches <joe@perches.com> wrote:

> On Wed, 2014-07-23 at 14:11 -0400, Jeff Layton wrote:
> > On Sun, 20 Jul 2014 11:23:43 -0700 Joe Perches <joe@perches.com> wrote:
> > > op->info.rv is an s32, but it's only used as a u8.
> > I don't understand this patch. info.rv is s32 (and I assume that "rv"
> > stands for "return value").
> 
> In this case it's not a return value but an input.
> 

Well, it's an input into the lm_grant callback, but it originally comes
in the downcall from userland (AFAICT). In this case, I'm referring to
the field in the downcall.

> > What I don't get is why you think it's just
> > used as a u8.
> 
> Because it's tested only in nlmsvc_grant_deferred
> and nlmsvc_update_deferred_block against 0.
> 
> As far as I can tell, it's not possible to set it
> to a negative value.
> 

It's been a while since I've looked over the lockd code, but I believe
it's just a flag that indicates whether there is still a conflict
between the block and the lock on the file.

> > It seems to be used more like a bool than anything else,
> > and I'm not sure that "type" is really a good description for it. Maybe
> > it should be a "bool" and named "conflict",
> 
> Maybe.  But it seemed likely and possible to expand
> it from a single bool to a value.
> 
> > given the comments in dlm_posix_get ?
> 
> Maybe, though I don't see how the comments relate to
> this change.  The rv value returned from that call
> is either -ENOMEM or 0 and is unchanged by this patch.
> 

I don't think that patch will break anything. I just don't see it as an
improvement on what's already there.

The rationale for this is lost in antiquity, but I think the basic idea
was that you're either granting or updating the block based on the
_result_ from some check for a lock conflict. While "result" as a name
is a little confusing, "type" is even more so, IMO.

If you're hell-bent on changing this, then my suggestion would be
to turn it into a bool and call it "conflict" or something similar. If
you do decide to do that, adding some helpful kerneldoc comments would
be a nice improvement too.

> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> []
> > > @@ -842,7 +842,7 @@ struct lock_manager_operations {
> > >  	int (*lm_compare_owner)(struct file_lock *fl1, struct file_lock *fl2);
> > >  	unsigned long (*lm_owner_key)(struct file_lock *fl);
> > >  	void (*lm_notify)(struct file_lock *fl);	/* unblock callback */
> > > -	int (*lm_grant)(struct file_lock *fl, int result);
> > > +	int (*lm_grant)(struct file_lock *fl, unsigned char type);
> > >  	void (*lm_break)(struct file_lock *fl);
> > >  	int (*lm_change)(struct file_lock **fl, int type);
> > >  };
> 
> I used variable name "type" because that's what
> lm_change uses.  No worries if you think a name
> like conflict is better.
> 
> The only in-kernel setter of lm_grant is:
> 
> fs/lockd/svclock.c:     .lm_grant = nlmsvc_grant_deferred,
> 
> and for that, I think using a variable name of
> "result" is misleading at best.
> 


-- 
Jeff Layton <jlayton@poochiereds.net>

[Cluster-devel] [PATCH 9/9] fs: dlm: lockd: Convert int result to unsigned char type

[Joe Perches]

On Thu, 2014-07-24 at 12:24 -0400, Jeff Layton wrote:
> On Wed, 23 Jul 2014 20:53:59 -0700 Joe Perches <joe@perches.com> wrote:
> > On Wed, 2014-07-23 at 14:11 -0400, Jeff Layton wrote:
> > > On Sun, 20 Jul 2014 11:23:43 -0700 Joe Perches <joe@perches.com> wrote:
> > > > op->info.rv is an s32, but it's only used as a u8.
> > > I don't understand this patch. info.rv is s32 (and I assume that "rv"
> > > stands for "return value").
> > 
> > In this case it's not a return value but an input.
[]
> Well, it's an input into the lm_grant callback, but it originally comes
> in the downcall from userland (AFAICT). In this case, I'm referring to
[]
> It's been a while since I've looked over the lockd code, but I believe
> it's just a flag that indicates whether there is still a conflict
> between the block and the lock on the file.

Yes, that is how it is used.

> I don't think that patch will break anything. I just don't see it as an
> improvement on what's already there.
> 
> The rationale for this is lost in antiquity, but I think the basic idea
> was that you're either granting or updating the block based on the
> _result_ from some check for a lock conflict. While "result" as a name
> is a little confusing, "type" is even more so, IMO.
> 
> If you're hell-bent on changing this, then my suggestion would be
> to turn it into a bool and call it "conflict" or something similar. If
> you do decide to do that, adding some helpful kerneldoc comments would
> be a nice improvement too.

I hope I'm never hell-bent on patches.

I do prefer easier to read, clear code and I agree
that using it as a bool would make the code better.

I'll see about kernel-doc changes too.

cheers, Joe