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
On Sun, 20 Jul 2014 11:23:43 -0700
Joe Perches <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
On Wed, 23 Jul 2014 20:53:59 -0700
Joe Perches <[email protected]> wrote:
> On Wed, 2014-07-23 at 14:11 -0400, Jeff Layton wrote:
> > On Sun, 20 Jul 2014 11:23:43 -0700 Joe Perches <[email protected]> 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 <[email protected]>
On Sun, 20 Jul 2014 11:23:41 -0700
Joe Perches <[email protected]> wrote:
> This argument is always NULL so don't pass it around.
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> 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 <[email protected]>
On Wed, 2014-07-23 at 14:11 -0400, Jeff Layton wrote:
> On Sun, 20 Jul 2014 11:23:43 -0700 Joe Perches <[email protected]> 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.
This argument is always NULL so don't pass it around.
Signed-off-by: Joe Perches <[email protected]>
---
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
op->info.rv is an s32, but it's only used as a u8.
Signed-off-by: Joe Perches <[email protected]>
---
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
On Wed, Jul 23, 2014 at 02:11:39PM -0400, Jeff Layton wrote:
> On Sun, 20 Jul 2014 11:23:43 -0700
> Joe Perches <[email protected]> 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
On Thu, 2014-07-24 at 12:24 -0400, Jeff Layton wrote:
> On Wed, 23 Jul 2014 20:53:59 -0700 Joe Perches <[email protected]> wrote:
> > On Wed, 2014-07-23 at 14:11 -0400, Jeff Layton wrote:
> > > On Sun, 20 Jul 2014 11:23:43 -0700 Joe Perches <[email protected]> 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