[v2,01/13] ceph: add new r_req_flags field to ceph_mds_request
diff mbox

Message ID 20170201114914.20808-2-jlayton@redhat.com
State New
Headers show

Commit Message

Jeff Layton Feb. 1, 2017, 11:49 a.m. UTC
...and start moving bool flags into it.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/dir.c        | 2 +-
 fs/ceph/mds_client.c | 2 +-
 fs/ceph/mds_client.h | 4 +++-
 3 files changed, 5 insertions(+), 3 deletions(-)

Comments

Ilya Dryomov Feb. 1, 2017, 2:08 p.m. UTC | #1
On Wed, Feb 1, 2017 at 12:49 PM, Jeff Layton <jlayton@redhat.com> wrote:
> ...and start moving bool flags into it.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/ceph/dir.c        | 2 +-
>  fs/ceph/mds_client.c | 2 +-
>  fs/ceph/mds_client.h | 4 +++-
>  3 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index d4385563b70a..04fa4ae3deca 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -371,7 +371,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>                 /* hints to request -> mds selection code */
>                 req->r_direct_mode = USE_AUTH_MDS;
>                 req->r_direct_hash = ceph_frag_value(frag);
> -               req->r_direct_is_hash = true;
> +               set_bit(CEPH_MDS_R_DIRECT_IS_HASH, &req->r_req_flags);

Hi Jeff,

Just a couple of nits:

These are atomic -- should probably mention in the commit message why
is atomicity needed.

>                 if (fi->last_name) {
>                         req->r_path2 = kstrdup(fi->last_name, GFP_KERNEL);
>                         if (!req->r_path2) {
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 176512960b14..1f2ef02832d9 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -705,7 +705,7 @@ static int __choose_mds(struct ceph_mds_client *mdsc,
>         int mode = req->r_direct_mode;
>         int mds = -1;
>         u32 hash = req->r_direct_hash;
> -       bool is_hash = req->r_direct_is_hash;
> +       bool is_hash = test_bit(CEPH_MDS_R_DIRECT_IS_HASH, &req->r_req_flags);
>
>         /*
>          * is there a specific mds we should try?  ignore hint if we have
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index 3c6f77b7bb02..a58cacccc986 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -205,6 +205,9 @@ struct ceph_mds_request {
>         struct inode *r_locked_dir; /* dir (if any) i_mutex locked by vfs */
>         struct inode *r_target_inode;       /* resulting inode */
>
> +#define CEPH_MDS_R_DIRECT_IS_HASH      (1) /* r_direct_hash is valid */

Why parens?

> +       unsigned long   r_req_flags;
> +
>         struct mutex r_fill_mutex;
>
>         union ceph_mds_request_args r_args;
> @@ -216,7 +219,6 @@ struct ceph_mds_request {
>         /* for choosing which mds to send this request to */
>         int r_direct_mode;
>         u32 r_direct_hash;      /* choose dir frag based on this dentry hash */
> -       bool r_direct_is_hash;  /* true if r_direct_hash is valid */
>
>         /* data payload is used for xattr ops */
>         struct ceph_pagelist *r_pagelist;

3, 4, 5 and 6 can be merged into this patch, IMO.  They are trivial and
some change the same if statement over and over again.

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Feb. 1, 2017, 5:47 p.m. UTC | #2
On Wed, 2017-02-01 at 15:08 +0100, Ilya Dryomov wrote:
> On Wed, Feb 1, 2017 at 12:49 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > ...and start moving bool flags into it.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/ceph/dir.c        | 2 +-
> >  fs/ceph/mds_client.c | 2 +-
> >  fs/ceph/mds_client.h | 4 +++-
> >  3 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > index d4385563b70a..04fa4ae3deca 100644
> > --- a/fs/ceph/dir.c
> > +++ b/fs/ceph/dir.c
> > @@ -371,7 +371,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> >                 /* hints to request -> mds selection code */
> >                 req->r_direct_mode = USE_AUTH_MDS;
> >                 req->r_direct_hash = ceph_frag_value(frag);
> > -               req->r_direct_is_hash = true;
> > +               set_bit(CEPH_MDS_R_DIRECT_IS_HASH, &req->r_req_flags);
> 
> Hi Jeff,
> 
> Just a couple of nits:
> 
> These are atomic -- should probably mention in the commit message why
> is atomicity needed.
> 

It's not strictly needed for most of this stuff. DIRECT_IS_HASH in
particular could just use __set_bit.

The rest of the flags, I think are already protected from concurrent
writes by mutexes in the code. What I'm not sure of is whether they are
protected by the _same_ lock. That's a requirement if we mix all of the
flags together into the same word and don't want to use the atomic
*_bit macros.

I can look and see if that's possible. Even if it's not though, using
the atomic *_bit macros is generally not that expensive (particularly
in a situation where we're already using sleeping locks anyway).

> >                 if (fi->last_name) {
> >                         req->r_path2 = kstrdup(fi->last_name, GFP_KERNEL);
> >                         if (!req->r_path2) {
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 176512960b14..1f2ef02832d9 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -705,7 +705,7 @@ static int __choose_mds(struct ceph_mds_client *mdsc,
> >         int mode = req->r_direct_mode;
> >         int mds = -1;
> >         u32 hash = req->r_direct_hash;
> > -       bool is_hash = req->r_direct_is_hash;
> > +       bool is_hash = test_bit(CEPH_MDS_R_DIRECT_IS_HASH, &req->r_req_flags);
> > 
> >         /*
> >          * is there a specific mds we should try?  ignore hint if we have
> > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > index 3c6f77b7bb02..a58cacccc986 100644
> > --- a/fs/ceph/mds_client.h
> > +++ b/fs/ceph/mds_client.h
> > @@ -205,6 +205,9 @@ struct ceph_mds_request {
> >         struct inode *r_locked_dir; /* dir (if any) i_mutex locked by vfs */
> >         struct inode *r_target_inode;       /* resulting inode */
> > 
> > +#define CEPH_MDS_R_DIRECT_IS_HASH      (1) /* r_direct_hash is valid */
> 
> Why parens?
> 

Habit. I think we can safely remove them here.

> > +       unsigned long   r_req_flags;
> > +
> >         struct mutex r_fill_mutex;
> > 
> >         union ceph_mds_request_args r_args;
> > @@ -216,7 +219,6 @@ struct ceph_mds_request {
> >         /* for choosing which mds to send this request to */
> >         int r_direct_mode;
> >         u32 r_direct_hash;      /* choose dir frag based on this dentry hash */
> > -       bool r_direct_is_hash;  /* true if r_direct_hash is valid */
> > 
> >         /* data payload is used for xattr ops */
> >         struct ceph_pagelist *r_pagelist;
> 
> 3, 4, 5 and 6 can be merged into this patch, IMO.  They are trivial and
> some change the same if statement over and over again.
> 

Sure. I did it this way as that's how I put it together, but they can
definitely be squashed together. I'll plan to do that before the next
posting or before merging into testing branch.
Ilya Dryomov Feb. 1, 2017, 7:10 p.m. UTC | #3
On Wed, Feb 1, 2017 at 6:47 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Wed, 2017-02-01 at 15:08 +0100, Ilya Dryomov wrote:
>> On Wed, Feb 1, 2017 at 12:49 PM, Jeff Layton <jlayton@redhat.com> wrote:
>> > ...and start moving bool flags into it.
>> >
>> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
>> > ---
>> >  fs/ceph/dir.c        | 2 +-
>> >  fs/ceph/mds_client.c | 2 +-
>> >  fs/ceph/mds_client.h | 4 +++-
>> >  3 files changed, 5 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> > index d4385563b70a..04fa4ae3deca 100644
>> > --- a/fs/ceph/dir.c
>> > +++ b/fs/ceph/dir.c
>> > @@ -371,7 +371,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>> >                 /* hints to request -> mds selection code */
>> >                 req->r_direct_mode = USE_AUTH_MDS;
>> >                 req->r_direct_hash = ceph_frag_value(frag);
>> > -               req->r_direct_is_hash = true;
>> > +               set_bit(CEPH_MDS_R_DIRECT_IS_HASH, &req->r_req_flags);
>>
>> Hi Jeff,
>>
>> Just a couple of nits:
>>
>> These are atomic -- should probably mention in the commit message why
>> is atomicity needed.
>>
>
> It's not strictly needed for most of this stuff. DIRECT_IS_HASH in
> particular could just use __set_bit.
>
> The rest of the flags, I think are already protected from concurrent
> writes by mutexes in the code. What I'm not sure of is whether they are
> protected by the _same_ lock. That's a requirement if we mix all of the
> flags together into the same word and don't want to use the atomic
> *_bit macros.
>
> I can look and see if that's possible. Even if it's not though, using
> the atomic *_bit macros is generally not that expensive (particularly
> in a situation where we're already using sleeping locks anyway).

Auditing for whether __set_bit, etc can be used instead is probably not
worth it.  I'm not saying we shouldn't use atomic bitops here -- just
wanted to get this explanation in the commit message.

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Feb. 1, 2017, 7:14 p.m. UTC | #4
On Wed, 2017-02-01 at 20:10 +0100, Ilya Dryomov wrote:
> On Wed, Feb 1, 2017 at 6:47 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Wed, 2017-02-01 at 15:08 +0100, Ilya Dryomov wrote:
> > > On Wed, Feb 1, 2017 at 12:49 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > > > ...and start moving bool flags into it.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > ---
> > > >  fs/ceph/dir.c        | 2 +-
> > > >  fs/ceph/mds_client.c | 2 +-
> > > >  fs/ceph/mds_client.h | 4 +++-
> > > >  3 files changed, 5 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > > > index d4385563b70a..04fa4ae3deca 100644
> > > > --- a/fs/ceph/dir.c
> > > > +++ b/fs/ceph/dir.c
> > > > @@ -371,7 +371,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > >                 /* hints to request -> mds selection code */
> > > >                 req->r_direct_mode = USE_AUTH_MDS;
> > > >                 req->r_direct_hash = ceph_frag_value(frag);
> > > > -               req->r_direct_is_hash = true;
> > > > +               set_bit(CEPH_MDS_R_DIRECT_IS_HASH, &req->r_req_flags);
> > > 
> > > Hi Jeff,
> > > 
> > > Just a couple of nits:
> > > 
> > > These are atomic -- should probably mention in the commit message why
> > > is atomicity needed.
> > > 
> > 
> > It's not strictly needed for most of this stuff. DIRECT_IS_HASH in
> > particular could just use __set_bit.
> > 
> > The rest of the flags, I think are already protected from concurrent
> > writes by mutexes in the code. What I'm not sure of is whether they are
> > protected by the _same_ lock. That's a requirement if we mix all of the
> > flags together into the same word and don't want to use the atomic
> > *_bit macros.
> > 
> > I can look and see if that's possible. Even if it's not though, using
> > the atomic *_bit macros is generally not that expensive (particularly
> > in a situation where we're already using sleeping locks anyway).
> 
> Auditing for whether __set_bit, etc can be used instead is probably not
> worth it.  I'm not saying we shouldn't use atomic bitops here -- just
> wanted to get this explanation in the commit message.
> 
> 

Thanks, done. I went ahead and changed DIRECT_IS_HASH to use __set_bit
(it's pretty clear that that's safe there), but the others do need the
atomic versions as they can have other tasks manipulating them.

I went ahead and merged the squashed set into ceph-client/testing with
an updated commit message. Let me know if you see any other issues with
the set.

Patch
diff mbox

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index d4385563b70a..04fa4ae3deca 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -371,7 +371,7 @@  static int ceph_readdir(struct file *file, struct dir_context *ctx)
 		/* hints to request -> mds selection code */
 		req->r_direct_mode = USE_AUTH_MDS;
 		req->r_direct_hash = ceph_frag_value(frag);
-		req->r_direct_is_hash = true;
+		set_bit(CEPH_MDS_R_DIRECT_IS_HASH, &req->r_req_flags);
 		if (fi->last_name) {
 			req->r_path2 = kstrdup(fi->last_name, GFP_KERNEL);
 			if (!req->r_path2) {
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 176512960b14..1f2ef02832d9 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -705,7 +705,7 @@  static int __choose_mds(struct ceph_mds_client *mdsc,
 	int mode = req->r_direct_mode;
 	int mds = -1;
 	u32 hash = req->r_direct_hash;
-	bool is_hash = req->r_direct_is_hash;
+	bool is_hash = test_bit(CEPH_MDS_R_DIRECT_IS_HASH, &req->r_req_flags);
 
 	/*
 	 * is there a specific mds we should try?  ignore hint if we have
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 3c6f77b7bb02..a58cacccc986 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -205,6 +205,9 @@  struct ceph_mds_request {
 	struct inode *r_locked_dir; /* dir (if any) i_mutex locked by vfs */
 	struct inode *r_target_inode;       /* resulting inode */
 
+#define CEPH_MDS_R_DIRECT_IS_HASH	(1) /* r_direct_hash is valid */
+	unsigned long	r_req_flags;
+
 	struct mutex r_fill_mutex;
 
 	union ceph_mds_request_args r_args;
@@ -216,7 +219,6 @@  struct ceph_mds_request {
 	/* for choosing which mds to send this request to */
 	int r_direct_mode;
 	u32 r_direct_hash;      /* choose dir frag based on this dentry hash */
-	bool r_direct_is_hash;  /* true if r_direct_hash is valid */
 
 	/* data payload is used for xattr ops */
 	struct ceph_pagelist *r_pagelist;