[PATCH] ceph: add a new vxattr to return auth mds for an inode

[PATCH] ceph: add a new vxattr to return auth mds for an inode

[Jeff Layton]

Add a new vxattr that shows what MDS is authoritative for an inode (if
we happen to have auth caps). If we don't have an auth cap for the inode
then just return -1.

URL: https://tracker.ceph.com/issues/1276
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/xattr.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 1242db8d3444..70664a19b8dc 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -340,6 +340,15 @@ static ssize_t ceph_vxattrcb_caps(struct ceph_inode_info *ci, char *val,
 			      ceph_cap_string(issued), issued);
 }
 
+static ssize_t ceph_vxattrcb_auth_mds(struct ceph_inode_info *ci,
+				       char *val, size_t size)
+{
+	/* return -1 if we don't have auth caps (and thus can't tell) */
+	if (!ci->i_auth_cap)
+		return ceph_fmt_xattr(val, size, "-1");
+	return ceph_fmt_xattr(val, size, "%d", ci->i_auth_cap->session->s_mds);
+}
+
 #define CEPH_XATTR_NAME(_type, _name)	XATTR_CEPH_PREFIX #_type "." #_name
 #define CEPH_XATTR_NAME2(_type, _name, _name2)	\
 	XATTR_CEPH_PREFIX #_type "." #_name "." #_name2
@@ -473,6 +482,13 @@ static struct ceph_vxattr ceph_common_vxattrs[] = {
 		.exists_cb = NULL,
 		.flags = VXATTR_FLAG_READONLY,
 	},
+	{
+		.name = "ceph.auth_mds",
+		.name_size = sizeof("ceph.auth_mds"),
+		.getxattr_cb = ceph_vxattrcb_auth_mds,
+		.exists_cb = NULL,
+		.flags = VXATTR_FLAG_READONLY,
+	},
 	{ .name = NULL, 0 }	/* Required table terminator */
 };
 
-- 
2.31.1

Re: [PATCH] ceph: add a new vxattr to return auth mds for an inode

[Luis Henriques]

On Tue, Jul 27, 2021 at 07:35:09AM -0400, Jeff Layton wrote:
> Add a new vxattr that shows what MDS is authoritative for an inode (if
> we happen to have auth caps). If we don't have an auth cap for the inode
> then just return -1.
> 
> URL: https://tracker.ceph.com/issues/1276
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/xattr.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 1242db8d3444..70664a19b8dc 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -340,6 +340,15 @@ static ssize_t ceph_vxattrcb_caps(struct ceph_inode_info *ci, char *val,
>  			      ceph_cap_string(issued), issued);
>  }
>  
> +static ssize_t ceph_vxattrcb_auth_mds(struct ceph_inode_info *ci,
> +				       char *val, size_t size)
> +{
> +	/* return -1 if we don't have auth caps (and thus can't tell) */
> +	if (!ci->i_auth_cap)
> +		return ceph_fmt_xattr(val, size, "-1");

I don't really have an opinion on this as I don't have a usecase for this
xattr (other than debug, of course).  But I just checked a similar function
ceph_vxattrcb_layout_pool_namespace() and, if there's no value for ns for an
inode, it just returns 0.

Anyway, just my 5c, as I'm OK with returning a '-1' string too.

Cheers,
--
Luís

> +	return ceph_fmt_xattr(val, size, "%d", ci->i_auth_cap->session->s_mds);
> +}
> +
>  #define CEPH_XATTR_NAME(_type, _name)	XATTR_CEPH_PREFIX #_type "." #_name
>  #define CEPH_XATTR_NAME2(_type, _name, _name2)	\
>  	XATTR_CEPH_PREFIX #_type "." #_name "." #_name2
> @@ -473,6 +482,13 @@ static struct ceph_vxattr ceph_common_vxattrs[] = {
>  		.exists_cb = NULL,
>  		.flags = VXATTR_FLAG_READONLY,
>  	},
> +	{
> +		.name = "ceph.auth_mds",
> +		.name_size = sizeof("ceph.auth_mds"),
> +		.getxattr_cb = ceph_vxattrcb_auth_mds,
> +		.exists_cb = NULL,
> +		.flags = VXATTR_FLAG_READONLY,
> +	},
>  	{ .name = NULL, 0 }	/* Required table terminator */
>  };
>  
> -- 
> 2.31.1
> 

Re: [PATCH] ceph: add a new vxattr to return auth mds for an inode

[Jeff Layton]

On Tue, 2021-07-27 at 15:46 +0100, Luis Henriques wrote:
> On Tue, Jul 27, 2021 at 07:35:09AM -0400, Jeff Layton wrote:
> > Add a new vxattr that shows what MDS is authoritative for an inode (if
> > we happen to have auth caps). If we don't have an auth cap for the inode
> > then just return -1.
> > 
> > URL: https://tracker.ceph.com/issues/1276
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/xattr.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > index 1242db8d3444..70664a19b8dc 100644
> > --- a/fs/ceph/xattr.c
> > +++ b/fs/ceph/xattr.c
> > @@ -340,6 +340,15 @@ static ssize_t ceph_vxattrcb_caps(struct ceph_inode_info *ci, char *val,
> >  			      ceph_cap_string(issued), issued);
> >  }
> >  
> > +static ssize_t ceph_vxattrcb_auth_mds(struct ceph_inode_info *ci,
> > +				       char *val, size_t size)
> > +{
> > +	/* return -1 if we don't have auth caps (and thus can't tell) */
> > +	if (!ci->i_auth_cap)
> > +		return ceph_fmt_xattr(val, size, "-1");
> 
> I don't really have an opinion on this as I don't have a usecase for this
> xattr (other than debug, of course).  But I just checked a similar function
> ceph_vxattrcb_layout_pool_namespace() and, if there's no value for ns for an
> inode, it just returns 0.
> 
> Anyway, just my 5c, as I'm OK with returning a '-1' string too.
> 


TBH, I don't have much of a use-case for this either, but it was
requested by Sage long ago. That said, I figure it might be useful in
some cases, particularly when troubleshooting pinning issues.

Pool numbering is a bit different, as I think pool 0 is not valid,
whereas we index mds's starting with 0. I'm fine with a different
convention here, but I considered it as a safe way to say "I don't know"
in this situation.

> > +	return ceph_fmt_xattr(val, size, "%d", ci->i_auth_cap->session->s_mds);
> > +}
> > +
> >  #define CEPH_XATTR_NAME(_type, _name)	XATTR_CEPH_PREFIX #_type "." #_name
> >  #define CEPH_XATTR_NAME2(_type, _name, _name2)	\
> >  	XATTR_CEPH_PREFIX #_type "." #_name "." #_name2
> > @@ -473,6 +482,13 @@ static struct ceph_vxattr ceph_common_vxattrs[] = {
> >  		.exists_cb = NULL,
> >  		.flags = VXATTR_FLAG_READONLY,
> >  	},
> > +	{
> > +		.name = "ceph.auth_mds",
> > +		.name_size = sizeof("ceph.auth_mds"),
> > +		.getxattr_cb = ceph_vxattrcb_auth_mds,
> > +		.exists_cb = NULL,
> > +		.flags = VXATTR_FLAG_READONLY,
> > +	},
> >  	{ .name = NULL, 0 }	/* Required table terminator */
> >  };
> >  
> > -- 
> > 2.31.1
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: add a new vxattr to return auth mds for an inode

[Luis Henriques]

On Tue, Jul 27, 2021 at 11:17:39AM -0400, Jeff Layton wrote:
> On Tue, 2021-07-27 at 15:46 +0100, Luis Henriques wrote:
> > On Tue, Jul 27, 2021 at 07:35:09AM -0400, Jeff Layton wrote:
> > > Add a new vxattr that shows what MDS is authoritative for an inode (if
> > > we happen to have auth caps). If we don't have an auth cap for the inode
> > > then just return -1.
> > > 
> > > URL: https://tracker.ceph.com/issues/1276
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/ceph/xattr.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > > index 1242db8d3444..70664a19b8dc 100644
> > > --- a/fs/ceph/xattr.c
> > > +++ b/fs/ceph/xattr.c
> > > @@ -340,6 +340,15 @@ static ssize_t ceph_vxattrcb_caps(struct ceph_inode_info *ci, char *val,
> > >  			      ceph_cap_string(issued), issued);
> > >  }
> > >  
> > > +static ssize_t ceph_vxattrcb_auth_mds(struct ceph_inode_info *ci,
> > > +				       char *val, size_t size)
> > > +{
> > > +	/* return -1 if we don't have auth caps (and thus can't tell) */
> > > +	if (!ci->i_auth_cap)
> > > +		return ceph_fmt_xattr(val, size, "-1");
> > 
> > I don't really have an opinion on this as I don't have a usecase for this
> > xattr (other than debug, of course).  But I just checked a similar function
> > ceph_vxattrcb_layout_pool_namespace() and, if there's no value for ns for an
> > inode, it just returns 0.
> > 
> > Anyway, just my 5c, as I'm OK with returning a '-1' string too.
> > 
> 
> 
> TBH, I don't have much of a use-case for this either, but it was
> requested by Sage long ago. That said, I figure it might be useful in
> some cases, particularly when troubleshooting pinning issues.
> 
> Pool numbering is a bit different, as I think pool 0 is not valid,
> whereas we index mds's starting with 0. I'm fine with a different
> convention here, but I considered it as a safe way to say "I don't know"
> in this situation.

Right, but what I meant by returning 0 was to really do:

	if (!ci->i_auth_cap)
		return 0;

and not to return the string "0".  Anyway, as I said, I don't have an
opinion on this and returning the "-1" string for the MDS index sounds
good too.

Cheers,
--
Luís

> > > +	return ceph_fmt_xattr(val, size, "%d", ci->i_auth_cap->session->s_mds);
> > > +}
> > > +
> > >  #define CEPH_XATTR_NAME(_type, _name)	XATTR_CEPH_PREFIX #_type "." #_name
> > >  #define CEPH_XATTR_NAME2(_type, _name, _name2)	\
> > >  	XATTR_CEPH_PREFIX #_type "." #_name "." #_name2
> > > @@ -473,6 +482,13 @@ static struct ceph_vxattr ceph_common_vxattrs[] = {
> > >  		.exists_cb = NULL,
> > >  		.flags = VXATTR_FLAG_READONLY,
> > >  	},
> > > +	{
> > > +		.name = "ceph.auth_mds",
> > > +		.name_size = sizeof("ceph.auth_mds"),
> > > +		.getxattr_cb = ceph_vxattrcb_auth_mds,
> > > +		.exists_cb = NULL,
> > > +		.flags = VXATTR_FLAG_READONLY,
> > > +	},
> > >  	{ .name = NULL, 0 }	/* Required table terminator */
> > >  };
> > >  
> > > -- 
> > > 2.31.1
> > > 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

Re: [PATCH] ceph: add a new vxattr to return auth mds for an inode

[Jeff Layton]

On Tue, 2021-07-27 at 16:45 +0100, Luis Henriques wrote:
> On Tue, Jul 27, 2021 at 11:17:39AM -0400, Jeff Layton wrote:
> > On Tue, 2021-07-27 at 15:46 +0100, Luis Henriques wrote:
> > > On Tue, Jul 27, 2021 at 07:35:09AM -0400, Jeff Layton wrote:
> > > > Add a new vxattr that shows what MDS is authoritative for an inode (if
> > > > we happen to have auth caps). If we don't have an auth cap for the inode
> > > > then just return -1.
> > > > 
> > > > URL: https://tracker.ceph.com/issues/1276
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/ceph/xattr.c | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > > 
> > > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > > > index 1242db8d3444..70664a19b8dc 100644
> > > > --- a/fs/ceph/xattr.c
> > > > +++ b/fs/ceph/xattr.c
> > > > @@ -340,6 +340,15 @@ static ssize_t ceph_vxattrcb_caps(struct ceph_inode_info *ci, char *val,
> > > >  			      ceph_cap_string(issued), issued);
> > > >  }
> > > >  
> > > > +static ssize_t ceph_vxattrcb_auth_mds(struct ceph_inode_info *ci,
> > > > +				       char *val, size_t size)
> > > > +{
> > > > +	/* return -1 if we don't have auth caps (and thus can't tell) */
> > > > +	if (!ci->i_auth_cap)
> > > > +		return ceph_fmt_xattr(val, size, "-1");
> > > 
> > > I don't really have an opinion on this as I don't have a usecase for this
> > > xattr (other than debug, of course).  But I just checked a similar function
> > > ceph_vxattrcb_layout_pool_namespace() and, if there's no value for ns for an
> > > inode, it just returns 0.
> > > 
> > > Anyway, just my 5c, as I'm OK with returning a '-1' string too.
> > > 
> > 
> > 
> > TBH, I don't have much of a use-case for this either, but it was
> > requested by Sage long ago. That said, I figure it might be useful in
> > some cases, particularly when troubleshooting pinning issues.
> > 
> > Pool numbering is a bit different, as I think pool 0 is not valid,
> > whereas we index mds's starting with 0. I'm fine with a different
> > convention here, but I considered it as a safe way to say "I don't know"
> > in this situation.
> 
> Right, but what I meant by returning 0 was to really do:
> 
> 	if (!ci->i_auth_cap)
> 		return 0;
> 
> and not to return the string "0".  Anyway, as I said, I don't have an
> opinion on this and returning the "-1" string for the MDS index sounds
> good too.
> 
> Cheers,
> --
> Luís
> 

Oh yeah, ok. We could do that I guess. I don't care much either way, but
that might be cleaner. Let me think on it a bit.

I also think we'll need to hold the ci->i_ceph_lock around the
i_auth_cap access. I'll need to send a v2 patch for that anyway.

> > > > +	return ceph_fmt_xattr(val, size, "%d", ci->i_auth_cap->session->s_mds);
> > > > +}
> > > > +
> > > >  #define CEPH_XATTR_NAME(_type, _name)	XATTR_CEPH_PREFIX #_type "." #_name
> > > >  #define CEPH_XATTR_NAME2(_type, _name, _name2)	\
> > > >  	XATTR_CEPH_PREFIX #_type "." #_name "." #_name2
> > > > @@ -473,6 +482,13 @@ static struct ceph_vxattr ceph_common_vxattrs[] = {
> > > >  		.exists_cb = NULL,
> > > >  		.flags = VXATTR_FLAG_READONLY,
> > > >  	},
> > > > +	{
> > > > +		.name = "ceph.auth_mds",
> > > > +		.name_size = sizeof("ceph.auth_mds"),
> > > > +		.getxattr_cb = ceph_vxattrcb_auth_mds,
> > > > +		.exists_cb = NULL,
> > > > +		.flags = VXATTR_FLAG_READONLY,
> > > > +	},
> > > >  	{ .name = NULL, 0 }	/* Required table terminator */
> > > >  };
> > > >  
> > > > -- 
> > > > 2.31.1
> > > > 
> > 
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 

-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v2] ceph: add a new vxattr to return auth mds for an inode

[Jeff Layton]

Add a new vxattr that shows what MDS is authoritative for an inode (if
we happen to have auth caps). If we don't have an auth cap for the inode
then just return -1.

URL: https://tracker.ceph.com/issues/1276
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/xattr.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

v2: ensure we hold the i_ceph_lock when working with the i_auth_cap.

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 1242db8d3444..159a1ffa4f4b 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -340,6 +340,18 @@ static ssize_t ceph_vxattrcb_caps(struct ceph_inode_info *ci, char *val,
 			      ceph_cap_string(issued), issued);
 }
 
+static ssize_t ceph_vxattrcb_auth_mds(struct ceph_inode_info *ci,
+				       char *val, size_t size)
+{
+	int ret;
+
+	spin_lock(&ci->i_ceph_lock);
+	ret = ceph_fmt_xattr(val, size, "%d",
+			     ci->i_auth_cap ? ci->i_auth_cap->session->s_mds : -1);
+	spin_unlock(&ci->i_ceph_lock);
+	return ret;
+}
+
 #define CEPH_XATTR_NAME(_type, _name)	XATTR_CEPH_PREFIX #_type "." #_name
 #define CEPH_XATTR_NAME2(_type, _name, _name2)	\
 	XATTR_CEPH_PREFIX #_type "." #_name "." #_name2
@@ -473,6 +485,13 @@ static struct ceph_vxattr ceph_common_vxattrs[] = {
 		.exists_cb = NULL,
 		.flags = VXATTR_FLAG_READONLY,
 	},
+	{
+		.name = "ceph.auth_mds",
+		.name_size = sizeof("ceph.auth_mds"),
+		.getxattr_cb = ceph_vxattrcb_auth_mds,
+		.exists_cb = NULL,
+		.flags = VXATTR_FLAG_READONLY,
+	},
 	{ .name = NULL, 0 }	/* Required table terminator */
 };
 
-- 
2.31.1

Re: [PATCH v2] ceph: add a new vxattr to return auth mds for an inode

[Luis Henriques]

On Tue, Jul 27, 2021 at 02:42:53PM -0400, Jeff Layton wrote:
> Add a new vxattr that shows what MDS is authoritative for an inode (if
> we happen to have auth caps). If we don't have an auth cap for the inode
> then just return -1.
> 
> URL: https://tracker.ceph.com/issues/1276
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/xattr.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> v2: ensure we hold the i_ceph_lock when working with the i_auth_cap.

Yeah, this lock is definitely needed here.  LGTM.

Cheers,
--
Luís


> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 1242db8d3444..159a1ffa4f4b 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -340,6 +340,18 @@ static ssize_t ceph_vxattrcb_caps(struct ceph_inode_info *ci, char *val,
>  			      ceph_cap_string(issued), issued);
>  }
>  
> +static ssize_t ceph_vxattrcb_auth_mds(struct ceph_inode_info *ci,
> +				       char *val, size_t size)
> +{
> +	int ret;
> +
> +	spin_lock(&ci->i_ceph_lock);
> +	ret = ceph_fmt_xattr(val, size, "%d",
> +			     ci->i_auth_cap ? ci->i_auth_cap->session->s_mds : -1);
> +	spin_unlock(&ci->i_ceph_lock);
> +	return ret;
> +}
> +
>  #define CEPH_XATTR_NAME(_type, _name)	XATTR_CEPH_PREFIX #_type "." #_name
>  #define CEPH_XATTR_NAME2(_type, _name, _name2)	\
>  	XATTR_CEPH_PREFIX #_type "." #_name "." #_name2
> @@ -473,6 +485,13 @@ static struct ceph_vxattr ceph_common_vxattrs[] = {
>  		.exists_cb = NULL,
>  		.flags = VXATTR_FLAG_READONLY,
>  	},
> +	{
> +		.name = "ceph.auth_mds",
> +		.name_size = sizeof("ceph.auth_mds"),
> +		.getxattr_cb = ceph_vxattrcb_auth_mds,
> +		.exists_cb = NULL,
> +		.flags = VXATTR_FLAG_READONLY,
> +	},
>  	{ .name = NULL, 0 }	/* Required table terminator */
>  };
>  
> -- 
> 2.31.1
>