diff mbox series

[v4,2/2] ceph: add ceph.{cluster_fsid/client_id} vxattrs suppport

Message ID 20201111012940.468289-3-xiubli@redhat.com
State New
Headers show
Series ceph: add vxattrs to get ids and status debug file support | expand

Commit Message

Xiubo Li Nov. 11, 2020, 1:29 a.m. UTC 
From: Xiubo Li <xiubli@redhat.com>

These two vxattrs will only exist in local client side, with which
we can easily know which mountpoint the file belongs to and also
they can help locate the debugfs path quickly.

URL: https://tracker.ceph.com/issues/48057
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/xattr.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

Comments

Patrick Donnelly Nov. 13, 2020, 7:40 p.m. UTC | #1 
On Tue, Nov 10, 2020 at 5:29 PM <xiubli@redhat.com> wrote:
>

> From: Xiubo Li <xiubli@redhat.com>

>

> These two vxattrs will only exist in local client side, with which

> we can easily know which mountpoint the file belongs to and also

> they can help locate the debugfs path quickly.

>

> URL: https://tracker.ceph.com/issues/48057

> Signed-off-by: Xiubo Li <xiubli@redhat.com>

> ---

>  fs/ceph/xattr.c | 42 ++++++++++++++++++++++++++++++++++++++++++

>  1 file changed, 42 insertions(+)

>

> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c

> index 0fd05d3d4399..e89750a1f039 100644

> --- a/fs/ceph/xattr.c

> +++ b/fs/ceph/xattr.c

> @@ -304,6 +304,23 @@ static ssize_t ceph_vxattrcb_snap_btime(struct ceph_inode_info *ci, char *val,

>                                 ci->i_snap_btime.tv_nsec);

>  }

>

> +static ssize_t ceph_vxattrcb_cluster_fsid(struct ceph_inode_info *ci,

> +                                         char *val, size_t size)

> +{

> +       struct ceph_fs_client *fsc = ceph_sb_to_client(ci->vfs_inode.i_sb);

> +

> +       return ceph_fmt_xattr(val, size, "%pU", &fsc->client->fsid);

> +}

> +

> +static ssize_t ceph_vxattrcb_client_id(struct ceph_inode_info *ci,

> +                                      char *val, size_t size)

> +{

> +       struct ceph_fs_client *fsc = ceph_sb_to_client(ci->vfs_inode.i_sb);

> +

> +       return ceph_fmt_xattr(val, size, "client%lld",

> +                             ceph_client_gid(fsc->client));

> +}


Let's just have this return the id number. The caller can concatenate
that with "client" however they require. Otherwise, parsing it out is
needlessly troublesome.

>  #define CEPH_XATTR_NAME(_type, _name)  XATTR_CEPH_PREFIX #_type "." #_name

>  #define CEPH_XATTR_NAME2(_type, _name, _name2) \

>         XATTR_CEPH_PREFIX #_type "." #_name "." #_name2

> @@ -407,6 +424,24 @@ static struct ceph_vxattr ceph_file_vxattrs[] = {

>         { .name = NULL, 0 }     /* Required table terminator */

>  };

>

> +static struct ceph_vxattr ceph_common_vxattrs[] = {

> +       {

> +               .name = "ceph.cluster_fsid",

> +               .name_size = sizeof("ceph.cluster_fsid"),

> +               .getxattr_cb = ceph_vxattrcb_cluster_fsid,

> +               .exists_cb = NULL,

> +               .flags = VXATTR_FLAG_READONLY,

> +       },


I would prefer just "ceph.fsid".

> +       {

> +               .name = "ceph.client_id",

> +               .name_size = sizeof("ceph.client_id"),

> +               .getxattr_cb = ceph_vxattrcb_client_id,

> +               .exists_cb = NULL,

> +               .flags = VXATTR_FLAG_READONLY,

> +       },

> +       { .name = NULL, 0 }     /* Required table terminator */

> +};

> +

>  static struct ceph_vxattr *ceph_inode_vxattrs(struct inode *inode)

>  {

>         if (S_ISDIR(inode->i_mode))

> @@ -429,6 +464,13 @@ static struct ceph_vxattr *ceph_match_vxattr(struct inode *inode,

>                 }

>         }

>

> +       vxattr = ceph_common_vxattrs;

> +       while (vxattr->name) {

> +               if (!strcmp(vxattr->name, name))

> +                       return vxattr;

> +               vxattr++;

> +       }

> +

>         return NULL;

>  }


Please also be sure to wire up the same vxattrs in the userspace Client.


-- 
Patrick Donnelly, Ph.D.
He / Him / His
Principal Software Engineer
Red Hat Sunnyvale, CA
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
Jeff Layton Nov. 13, 2020, 7:45 p.m. UTC | #2 
On Fri, 2020-11-13 at 11:40 -0800, Patrick Donnelly wrote:
> On Tue, Nov 10, 2020 at 5:29 PM <xiubli@redhat.com> wrote:

> > 

> > From: Xiubo Li <xiubli@redhat.com>

> > 

> > These two vxattrs will only exist in local client side, with which

> > we can easily know which mountpoint the file belongs to and also

> > they can help locate the debugfs path quickly.

> > 

> > URL: https://tracker.ceph.com/issues/48057

> > Signed-off-by: Xiubo Li <xiubli@redhat.com>

> > ---

> >  fs/ceph/xattr.c | 42 ++++++++++++++++++++++++++++++++++++++++++

> >  1 file changed, 42 insertions(+)

> > 

> > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c

> > index 0fd05d3d4399..e89750a1f039 100644

> > --- a/fs/ceph/xattr.c

> > +++ b/fs/ceph/xattr.c

> > @@ -304,6 +304,23 @@ static ssize_t ceph_vxattrcb_snap_btime(struct ceph_inode_info *ci, char *val,

> >                                 ci->i_snap_btime.tv_nsec);

> >  }

> > 

> > +static ssize_t ceph_vxattrcb_cluster_fsid(struct ceph_inode_info *ci,

> > +                                         char *val, size_t size)

> > +{

> > +       struct ceph_fs_client *fsc = ceph_sb_to_client(ci->vfs_inode.i_sb);

> > +

> > +       return ceph_fmt_xattr(val, size, "%pU", &fsc->client->fsid);

> > +}

> > +

> > +static ssize_t ceph_vxattrcb_client_id(struct ceph_inode_info *ci,

> > +                                      char *val, size_t size)

> > +{

> > +       struct ceph_fs_client *fsc = ceph_sb_to_client(ci->vfs_inode.i_sb);

> > +

> > +       return ceph_fmt_xattr(val, size, "client%lld",

> > +                             ceph_client_gid(fsc->client));

> > +}

> 

> Let's just have this return the id number. The caller can concatenate

> that with "client" however they require. Otherwise, parsing it out is

> needlessly troublesome.

> 


That does seem nicer, but it would be inconsistent with the existing rbd
attributes. Ilya, would you object?

> >  #define CEPH_XATTR_NAME(_type, _name)  XATTR_CEPH_PREFIX #_type "." #_name

> >  #define CEPH_XATTR_NAME2(_type, _name, _name2) \

> >         XATTR_CEPH_PREFIX #_type "." #_name "." #_name2

> > @@ -407,6 +424,24 @@ static struct ceph_vxattr ceph_file_vxattrs[] = {

> >         { .name = NULL, 0 }     /* Required table terminator */

> >  };

> > 

> > +static struct ceph_vxattr ceph_common_vxattrs[] = {

> > +       {

> > +               .name = "ceph.cluster_fsid",

> > +               .name_size = sizeof("ceph.cluster_fsid"),

> > +               .getxattr_cb = ceph_vxattrcb_cluster_fsid,

> > +               .exists_cb = NULL,

> > +               .flags = VXATTR_FLAG_READONLY,

> > +       },

> 

> I would prefer just "ceph.fsid".

> 


Ilya suggested this name to match existing rbd attributes.
 
> > +       {

> > +               .name = "ceph.client_id",

> > +               .name_size = sizeof("ceph.client_id"),

> > +               .getxattr_cb = ceph_vxattrcb_client_id,

> > +               .exists_cb = NULL,

> > +               .flags = VXATTR_FLAG_READONLY,

> > +       },

> > +       { .name = NULL, 0 }     /* Required table terminator */

> > +};

> > +

> >  static struct ceph_vxattr *ceph_inode_vxattrs(struct inode *inode)

> >  {

> >         if (S_ISDIR(inode->i_mode))

> > @@ -429,6 +464,13 @@ static struct ceph_vxattr *ceph_match_vxattr(struct inode *inode,

> >                 }

> >         }

> > 

> > +       vxattr = ceph_common_vxattrs;

> > +       while (vxattr->name) {

> > +               if (!strcmp(vxattr->name, name))

> > +                       return vxattr;

> > +               vxattr++;

> > +       }

> > +

> >         return NULL;

> >  }

> 

> Please also be sure to wire up the same vxattrs in the userspace Client.

> 

> 


-- 
Jeff Layton <jlayton@kernel.org>
Ilya Dryomov Nov. 13, 2020, 8:16 p.m. UTC | #3 
On Fri, Nov 13, 2020 at 8:45 PM Jeff Layton <jlayton@kernel.org> wrote:
>

> On Fri, 2020-11-13 at 11:40 -0800, Patrick Donnelly wrote:

> > On Tue, Nov 10, 2020 at 5:29 PM <xiubli@redhat.com> wrote:

> > >

> > > From: Xiubo Li <xiubli@redhat.com>

> > >

> > > These two vxattrs will only exist in local client side, with which

> > > we can easily know which mountpoint the file belongs to and also

> > > they can help locate the debugfs path quickly.

> > >

> > > URL: https://tracker.ceph.com/issues/48057

> > > Signed-off-by: Xiubo Li <xiubli@redhat.com>

> > > ---

> > >  fs/ceph/xattr.c | 42 ++++++++++++++++++++++++++++++++++++++++++

> > >  1 file changed, 42 insertions(+)

> > >

> > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c

> > > index 0fd05d3d4399..e89750a1f039 100644

> > > --- a/fs/ceph/xattr.c

> > > +++ b/fs/ceph/xattr.c

> > > @@ -304,6 +304,23 @@ static ssize_t ceph_vxattrcb_snap_btime(struct ceph_inode_info *ci, char *val,

> > >                                 ci->i_snap_btime.tv_nsec);

> > >  }

> > >

> > > +static ssize_t ceph_vxattrcb_cluster_fsid(struct ceph_inode_info *ci,

> > > +                                         char *val, size_t size)

> > > +{

> > > +       struct ceph_fs_client *fsc = ceph_sb_to_client(ci->vfs_inode.i_sb);

> > > +

> > > +       return ceph_fmt_xattr(val, size, "%pU", &fsc->client->fsid);

> > > +}

> > > +

> > > +static ssize_t ceph_vxattrcb_client_id(struct ceph_inode_info *ci,

> > > +                                      char *val, size_t size)

> > > +{

> > > +       struct ceph_fs_client *fsc = ceph_sb_to_client(ci->vfs_inode.i_sb);

> > > +

> > > +       return ceph_fmt_xattr(val, size, "client%lld",

> > > +                             ceph_client_gid(fsc->client));

> > > +}

> >

> > Let's just have this return the id number. The caller can concatenate

> > that with "client" however they require. Otherwise, parsing it out is

> > needlessly troublesome.

> >

>

> That does seem nicer, but it would be inconsistent with the existing rbd

> attributes. Ilya, would you object?


I'm not married to it, but staying consistent would be good.  Yes,
it concatenates with "client" and yes, without a dot, but the kernel
client has been doing that for a very long time.  This behaviour
actually predates the rbd attributes I mentioned.

>

> > >  #define CEPH_XATTR_NAME(_type, _name)  XATTR_CEPH_PREFIX #_type "." #_name

> > >  #define CEPH_XATTR_NAME2(_type, _name, _name2) \

> > >         XATTR_CEPH_PREFIX #_type "." #_name "." #_name2

> > > @@ -407,6 +424,24 @@ static struct ceph_vxattr ceph_file_vxattrs[] = {

> > >         { .name = NULL, 0 }     /* Required table terminator */

> > >  };

> > >

> > > +static struct ceph_vxattr ceph_common_vxattrs[] = {

> > > +       {

> > > +               .name = "ceph.cluster_fsid",

> > > +               .name_size = sizeof("ceph.cluster_fsid"),

> > > +               .getxattr_cb = ceph_vxattrcb_cluster_fsid,

> > > +               .exists_cb = NULL,

> > > +               .flags = VXATTR_FLAG_READONLY,

> > > +       },

> >

> > I would prefer just "ceph.fsid".

> >

>

> Ilya suggested this name to match existing rbd attributes.


I think "cluster" is particularly important here because people
might mistake it for a filesystem identifier.

Thanks,

                Ilya
Xiubo Li Nov. 14, 2020, 12:54 a.m. UTC | #4 
On 2020/11/14 3:40, Patrick Donnelly wrote:
> On Tue, Nov 10, 2020 at 5:29 PM <xiubli@redhat.com> wrote:

[...]
>>   static struct ceph_vxattr *ceph_inode_vxattrs(struct inode *inode)

>>   {

>>          if (S_ISDIR(inode->i_mode))

>> @@ -429,6 +464,13 @@ static struct ceph_vxattr *ceph_match_vxattr(struct inode *inode,

>>                  }

>>          }

>>

>> +       vxattr = ceph_common_vxattrs;

>> +       while (vxattr->name) {

>> +               if (!strcmp(vxattr->name, name))

>> +                       return vxattr;

>> +               vxattr++;

>> +       }

>> +

>>          return NULL;

>>   }

> Please also be sure to wire up the same vxattrs in the userspace Client.

>

>

Sure, will do.

Thanks.
diff mbox series

Patch

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 0fd05d3d4399..e89750a1f039 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -304,6 +304,23 @@  static ssize_t ceph_vxattrcb_snap_btime(struct ceph_inode_info *ci, char *val,
 				ci->i_snap_btime.tv_nsec);
 }
 
+static ssize_t ceph_vxattrcb_cluster_fsid(struct ceph_inode_info *ci,
+					  char *val, size_t size)
+{
+	struct ceph_fs_client *fsc = ceph_sb_to_client(ci->vfs_inode.i_sb);
+
+	return ceph_fmt_xattr(val, size, "%pU", &fsc->client->fsid);
+}
+
+static ssize_t ceph_vxattrcb_client_id(struct ceph_inode_info *ci,
+				       char *val, size_t size)
+{
+	struct ceph_fs_client *fsc = ceph_sb_to_client(ci->vfs_inode.i_sb);
+
+	return ceph_fmt_xattr(val, size, "client%lld",
+			      ceph_client_gid(fsc->client));
+}
+
 #define CEPH_XATTR_NAME(_type, _name)	XATTR_CEPH_PREFIX #_type "." #_name
 #define CEPH_XATTR_NAME2(_type, _name, _name2)	\
 	XATTR_CEPH_PREFIX #_type "." #_name "." #_name2
@@ -407,6 +424,24 @@  static struct ceph_vxattr ceph_file_vxattrs[] = {
 	{ .name = NULL, 0 }	/* Required table terminator */
 };
 
+static struct ceph_vxattr ceph_common_vxattrs[] = {
+	{
+		.name = "ceph.cluster_fsid",
+		.name_size = sizeof("ceph.cluster_fsid"),
+		.getxattr_cb = ceph_vxattrcb_cluster_fsid,
+		.exists_cb = NULL,
+		.flags = VXATTR_FLAG_READONLY,
+	},
+	{
+		.name = "ceph.client_id",
+		.name_size = sizeof("ceph.client_id"),
+		.getxattr_cb = ceph_vxattrcb_client_id,
+		.exists_cb = NULL,
+		.flags = VXATTR_FLAG_READONLY,
+	},
+	{ .name = NULL, 0 }	/* Required table terminator */
+};
+
 static struct ceph_vxattr *ceph_inode_vxattrs(struct inode *inode)
 {
 	if (S_ISDIR(inode->i_mode))
@@ -429,6 +464,13 @@  static struct ceph_vxattr *ceph_match_vxattr(struct inode *inode,
 		}
 	}
 
+	vxattr = ceph_common_vxattrs;
+	while (vxattr->name) {
+		if (!strcmp(vxattr->name, name))
+			return vxattr;
+		vxattr++;
+	}
+
 	return NULL;
 }