Re: [PATCH] ceph: fall back to use old method to get xattr

[Xiubo Li <xiubli@xxxxxxxxxx>]

On 7/27/22 5:05 PM, Luís Henriques wrote:
> On Wed, Jul 27, 2022 at 01:56:37PM +0800, xiubli@xxxxxxxxxx wrote:
> > From: Xiubo Li <xiubli@xxxxxxxxxx>
> >
> > If the peer MDS doesn't support getvxattr op then just fall back to
> > use old getattr method to get it. Or for the old MDSs they will crash
> > when receive an unknown op.
> >
> > URL: https://tracker.ceph.com/issues/56529
> > Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> > ---
> >   fs/ceph/mds_client.c | 10 ++++++++++
> >   fs/ceph/mds_client.h |  4 +++-
> >   fs/ceph/xattr.c      |  9 ++++++---
> >   3 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 598012ddc401..bfe6d6393eba 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -3255,6 +3255,16 @@ static void __do_request(struct ceph_mds_client *mdsc,
> >   
> >   	dout("do_request mds%d session %p state %s\n", mds, session,
> >   	     ceph_session_state_name(session->s_state));
> > +
> > +	/*
> > +	 * The old ceph will crash the MDSs when see unknown OPs
> > +	 */
> > +	if (req->r_op == CEPH_MDS_OP_GETVXATTR &&
> > +	    !test_bit(CEPHFS_FEATURE_OP_GETVXATTR, &session->s_features)) {
> > +		err = -EOPNOTSUPP;
> > +		goto out_session;
> > +	}
> > +
> >   	if (session->s_state != CEPH_MDS_SESSION_OPEN &&
> >   	    session->s_state != CEPH_MDS_SESSION_HUNG) {
> >   		/*
> > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > index e15ee2858fef..0e03efab872a 100644
> > --- a/fs/ceph/mds_client.h
> > +++ b/fs/ceph/mds_client.h
> > @@ -31,8 +31,9 @@ enum ceph_feature_type {
> >   	CEPHFS_FEATURE_METRIC_COLLECT,
> >   	CEPHFS_FEATURE_ALTERNATE_NAME,
> >   	CEPHFS_FEATURE_NOTIFY_SESSION_STATE,
> > +	CEPHFS_FEATURE_OP_GETVXATTR,
> >   
> > -	CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_NOTIFY_SESSION_STATE,
> > +	CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_OP_GETVXATTR,
> >   };
> >   
> >   #define CEPHFS_FEATURES_CLIENT_SUPPORTED {	\
> > @@ -45,6 +46,7 @@ enum ceph_feature_type {
> >   	CEPHFS_FEATURE_METRIC_COLLECT,		\
> >   	CEPHFS_FEATURE_ALTERNATE_NAME,		\
> >   	CEPHFS_FEATURE_NOTIFY_SESSION_STATE,	\
> > +	CEPHFS_FEATURE_OP_GETVXATTR,		\
> >   }
> >   
> >   /*
> > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > index b10d459c2326..8f8db621772a 100644
> > --- a/fs/ceph/xattr.c
> > +++ b/fs/ceph/xattr.c
> > @@ -984,9 +984,12 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,
> >   		return err;
> >   	} else {
> >   		err = ceph_do_getvxattr(inode, name, value, size);
> > -		/* this would happen with a new client and old server combo */
> > +		/*
> > +		 * This would happen with a new client and old server combo,
> > +		 * then fall back to use old method to get it
> > +		 */
> >   		if (err == -EOPNOTSUPP)
> > -			err = -ENODATA;
> > +			goto handle_non_vxattrs;
> >   		return err;
> Nit: maybe just do:
>
> 		if (err != -EOPNOTSUPP)
> 			return err
>
> instead of using a 'goto' statement.

Sounds better.

> >   	}
> >   handle_non_vxattrs:
> > @@ -996,7 +999,7 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,
> >   	dout("getxattr %p name '%s' ver=%lld index_ver=%lld\n", inode, name,
> >   	     ci->i_xattrs.version, ci->i_xattrs.index_version);
> >   
> > -	if (ci->i_xattrs.version == 0 ||
> > +	if (ci->i_xattrs.version == 0 || err == -EOPNOTSUPP ||
> You'll need to initialise 'err' when declaring it.

Yeah, will fix it.

Thanks Luis!

-- Xiubo

> Cheers,
> --
> Luís
>
> >   	    !((req_mask & CEPH_CAP_XATTR_SHARED) ||
> >   	      __ceph_caps_issued_mask_metric(ci, CEPH_CAP_XATTR_SHARED, 1))) {
> >   		spin_unlock(&ci->i_ceph_lock);
> > --
> > 2.36.0.rc1