Re: [PATCH] ceph: add session already open notify support

From: Xiubo Li <xiubli@redhat.com>
To: Jeff Layton <jlayton@kernel.org>, idryomov@gmail.com
Cc: lhenriques@suse.de, vshankar@redhat.com, ceph-devel@vger.kernel.org
Subject: Re: [PATCH] ceph: add session already open notify support
Date: Fri, 27 May 2022 08:28:26 +0800	[thread overview]
Message-ID: <b4378e48-913f-dca4-2b7c-dc0fa3e5b83d@redhat.com> (raw)
In-Reply-To: <278dc8e098cf144b04b58f70f0a25fb106cd6d40.camel@kernel.org>

On 5/26/22 10:20 PM, Jeff Layton wrote:
> On Thu, 2022-05-26 at 20:45 +0800, Xiubo Li wrote:
>> On 5/26/22 6:44 PM, Jeff Layton wrote:
>>> On Thu, 2022-05-26 at 14:06 +0800, Xiubo Li wrote:
>>>> If the connection was accidently closed due to the socket issue or
>>>> something else the clients will try to open the opened sessions, the
>>>> MDSes will send the session open reply one more time if the clients
>>>> support the notify feature.
>>>>
>>>> When the clients retry to open the sessions the s_seq will be 0 as
>>>> default, we need to update it anyway.
>>>>
>>>> URL: https://tracker.ceph.com/issues/53911
>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>> ---
>>>>    fs/ceph/mds_client.c | 25 ++++++++++++++++++++-----
>>>>    fs/ceph/mds_client.h |  5 ++++-
>>>>    2 files changed, 24 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>>> index 4ced8d1e18ba..3e528b89b77a 100644
>>>> --- a/fs/ceph/mds_client.c
>>>> +++ b/fs/ceph/mds_client.c
>>>> @@ -3569,11 +3569,26 @@ static void handle_session(struct ceph_mds_session *session,
>>>>    	case CEPH_SESSION_OPEN:
>>>>    		if (session->s_state == CEPH_MDS_SESSION_RECONNECTING)
>>>>    			pr_info("mds%d reconnect success\n", session->s_mds);
>>>> -		session->s_state = CEPH_MDS_SESSION_OPEN;
>>>> -		session->s_features = features;
>>>> -		renewed_caps(mdsc, session, 0);
>>>> -		if (test_bit(CEPHFS_FEATURE_METRIC_COLLECT, &session->s_features))
>>>> -			metric_schedule_delayed(&mdsc->metric);
>>>> +
>>>> +		if (session->s_state == CEPH_MDS_SESSION_OPEN) {
>>>> +			pr_info("mds%d already opened\n", session->s_mds);
>>> Does the above pr_info actually help anything? What will the admin do
>>> with this info? I'd probably just skip this since this is sort of
>>> expected to happen from time to time.
>> Currently from the MDS side code, it won't be allowed to send the
>> session open reply more than once. So this should be something wrong
>> somewhere just like this issue we are fixing. So it's should be okay,
>> but maybe a warning instead ?
>>
> Ok. I'd probably go with pr_notice instead. It's not indicative of a
> kernel bug, necessarily, and we don't really need the admin to take any
> action (other than maybe report it to the ceph administrators).

Sure, will switch to notice instead.

Thanks

-- Xiubo

>>>> +		} else {
>>>> +			session->s_state = CEPH_MDS_SESSION_OPEN;
>>>> +			session->s_features = features;
>>>> +			renewed_caps(mdsc, session, 0);
>>>> +			if (test_bit(CEPHFS_FEATURE_METRIC_COLLECT,
>>>> +				     &session->s_features))
>>>> +				metric_schedule_delayed(&mdsc->metric);
>>>> +		}
>>>> +
>>>> +		/*
>>>> +		 * The connection maybe broken and the session in client
>>>> +		 * side has been reinitialized, need to update the seq
>>>> +		 * anyway.
>>>> +		 */
>>>> +		if (!session->s_seq && seq)
>>>> +			session->s_seq = seq;
>>>> +
>>>>    		wake = 1;
>>>>    		if (mdsc->stopping)
>>>>    			__close_session(mdsc, session);
>>>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>>>> index d8ec2ac93da3..256e3eada6c1 100644
>>>> --- a/fs/ceph/mds_client.h
>>>> +++ b/fs/ceph/mds_client.h
>>>> @@ -29,8 +29,10 @@ enum ceph_feature_type {
>>>>    	CEPHFS_FEATURE_MULTI_RECONNECT,
>>>>    	CEPHFS_FEATURE_DELEG_INO,
>>>>    	CEPHFS_FEATURE_METRIC_COLLECT,
>>>> +	CEPHFS_FEATURE_ALTERNATE_NAME,
>>>> +	CEPHFS_FEATURE_NOTIFY_SESSION_STATE,
>>>>    
>>>> -	CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_METRIC_COLLECT,
>>>> +	CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_NOTIFY_SESSION_STATE,
>>>>    };
>>>>    
>>>>    #define CEPHFS_FEATURES_CLIENT_SUPPORTED {	\
>>>> @@ -41,6 +43,7 @@ enum ceph_feature_type {
>>>>    	CEPHFS_FEATURE_MULTI_RECONNECT,		\
>>>>    	CEPHFS_FEATURE_DELEG_INO,		\
>>>>    	CEPHFS_FEATURE_METRIC_COLLECT,		\
>>>> +	CEPHFS_FEATURE_NOTIFY_SESSION_STATE,	\
>>>>    }
>>>>    
>>>>    /*
>>> The rest looks OK though.