Re: [PATCH v3 2/2] ceph: add ceph.{clusterid/clientid} vxattrs suppport

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2020-11-10 at 16:59 +0100, Ilya Dryomov wrote:
> On Tue, Nov 10, 2020 at 3:17 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..4a41db46e191 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_clusterid(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_clientid(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_vxattrs[] = {
> > +       {
> > +               .name = "ceph.clusterid",
> 
> I think this should be "ceph.cluster_fsid"
> 
> > +               .name_size = sizeof("ceph.clusterid"),
> > +               .getxattr_cb = ceph_vxattrcb_clusterid,
> > +               .exists_cb = NULL,
> > +               .flags = VXATTR_FLAG_READONLY,
> > +       },
> > +       {
> > +               .name = "ceph.clientid",
> 
> and this should be "ceph.client_id".  It's easier to read, consistent
> with "ceph fsid" command and with existing rbd attributes:
> 
>   static DEVICE_ATTR(client_id, 0444, rbd_client_id_show, NULL);
>   static DEVICE_ATTR(cluster_fsid, 0444, rbd_cluster_fsid_show, NULL);
> 

That sounds like a good idea. Xiubo, would you mind sending a v4?

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>