ceph: fall back to use old method to get xattr

From: Xiubo Li <xiubli@redhat.com>

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@redhat.com>
---
 fs/ceph/mds_client.c | 10 ++++++++++
 fs/ceph/mds_client.h |  4 +++-
 fs/ceph/xattr.c      |  9 ++++++---
 3 files changed, 19 insertions(+), 4 deletions(-)

On 7/27/22 5:05 PM, Luís Henriques wrote:
> On Wed, Jul 27, 2022 at 01:56:37PM +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> 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@redhat.com>
>> ---
>>   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
>>