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

From: Xiubo Li <xiubli@redhat.com>
To: "Luís Henriques" <lhenriques@suse.de>
Cc: ceph-devel@vger.kernel.org
Subject: Re: [PATCH] ceph: fall back to use old method to get xattr
Date: Wed, 27 Jul 2022 17:41:35 +0800	[thread overview]
Message-ID: <e6acea1f-0e80-9628-43a3-a9261248ad06@redhat.com> (raw)
In-Reply-To: <YuD/yDOwJaqg7q+X@suse.de>

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
>>