[PATCH v4 0/2] ceph: add vxattrs to get ids and status debug file support

[PATCH v4 0/2] ceph: add vxattrs to get ids and status debug file support

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

V4:
- s/bloclisted/blocklisted/
- rename clientid to client_id
- rename clusterid to cluster_fsid
- rename "inst_str" to "instance"
- rename ceph_vxattrs to ceph_common_vxattrs

V3:
- switch ioctl to vxattr.

V2:
- some typo fixings
- switch to use ceph_client_gid() and ceph_client_addr() helpers
- for ioctl cmd will return in text for cluster/client ids

Xiubo Li (2):
  ceph: add status debug file support
  ceph: add ceph.{cluster_fsid/client_id} vxattrs suppport

 fs/ceph/debugfs.c | 20 ++++++++++++++++++++
 fs/ceph/super.h   |  1 +
 fs/ceph/xattr.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

-- 
2.27.0

[PATCH v4 1/2] ceph: add status debug file support

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

This will help list some useful client side info, like the client
entity address/name and blocklisted status, etc.

URL: https://tracker.ceph.com/issues/48057
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/debugfs.c | 20 ++++++++++++++++++++
 fs/ceph/super.h   |  1 +
 2 files changed, 21 insertions(+)

diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
index 7a8fbe3e4751..66989c880adb 100644
--- a/fs/ceph/debugfs.c
+++ b/fs/ceph/debugfs.c
@@ -304,11 +304,25 @@ static int mds_sessions_show(struct seq_file *s, void *ptr)
 	return 0;
 }
 
+static int status_show(struct seq_file *s, void *p)
+{
+	struct ceph_fs_client *fsc = s->private;
+	struct ceph_entity_inst *inst = &fsc->client->msgr.inst;
+	struct ceph_entity_addr *client_addr = ceph_client_addr(fsc->client);
+
+	seq_printf(s, "instance: %s.%lld %s/%u\n", ENTITY_NAME(inst->name),
+		   ceph_pr_addr(client_addr), le32_to_cpu(client_addr->nonce));
+	seq_printf(s, "blocklisted: %s\n", fsc->blocklisted ? "true" : "false");
+
+	return 0;
+}
+
 DEFINE_SHOW_ATTRIBUTE(mdsmap);
 DEFINE_SHOW_ATTRIBUTE(mdsc);
 DEFINE_SHOW_ATTRIBUTE(caps);
 DEFINE_SHOW_ATTRIBUTE(mds_sessions);
 DEFINE_SHOW_ATTRIBUTE(metric);
+DEFINE_SHOW_ATTRIBUTE(status);
 
 
 /*
@@ -394,6 +408,12 @@ void ceph_fs_debugfs_init(struct ceph_fs_client *fsc)
 						fsc->client->debugfs_dir,
 						fsc,
 						&caps_fops);
+
+	fsc->debugfs_status = debugfs_create_file("status",
+						  0400,
+						  fsc->client->debugfs_dir,
+						  fsc,
+						  &status_fops);
 }
 
 
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index f097237a5ad3..5138b75923f9 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -128,6 +128,7 @@ struct ceph_fs_client {
 	struct dentry *debugfs_bdi;
 	struct dentry *debugfs_mdsc, *debugfs_mdsmap;
 	struct dentry *debugfs_metric;
+	struct dentry *debugfs_status;
 	struct dentry *debugfs_mds_sessions;
 #endif
 
-- 
2.27.0

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

[xiubli]

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));
+}
+
 #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;
 }
 
-- 
2.27.0

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

[Patrick Donnelly]

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

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

[Jeff Layton]

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>

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

[Ilya Dryomov]

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

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

[Xiubo Li]

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.