diff mbox series

ceph: reinitialize mds feature bit even when session in open

Message ID 20231103064027.316296-1-vshankar@redhat.com
State Superseded
Headers show
Series ceph: reinitialize mds feature bit even when session in open | expand

Commit Message

Venky Shankar Nov. 3, 2023, 6:40 a.m. UTC
Following along the same lines as per the user-space fix. Right
now this isn't really an issue with the ceph kernel driver because
of the feature bit laginess, however, that can change over time
(when the new snaprealm info type is ported to the kernel driver)
and depending on the MDS version that's being upgraded can cause
message decoding issues - so, fix that early on.

URL: Fixes: http://tracker.ceph.com/issues/63188
Signed-off-by: Venky Shankar <vshankar@redhat.com>
---
 fs/ceph/mds_client.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Venky Shankar Nov. 3, 2023, 6:43 a.m. UTC | #1
On Fri, Nov 3, 2023 at 12:10 PM Venky Shankar <vshankar@redhat.com> wrote:
>
> Following along the same lines as per the user-space fix. Right
> now this isn't really an issue with the ceph kernel driver because
> of the feature bit laginess, however, that can change over time
> (when the new snaprealm info type is ported to the kernel driver)
> and depending on the MDS version that's being upgraded can cause
> message decoding issues - so, fix that early on.
>
> URL: Fixes: http://tracker.ceph.com/issues/63188
> Signed-off-by: Venky Shankar <vshankar@redhat.com>
> ---
>  fs/ceph/mds_client.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index a7bffb030036..48d75e17115c 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -4192,6 +4192,7 @@ static void handle_session(struct ceph_mds_session *session,
>                 if (session->s_state == CEPH_MDS_SESSION_OPEN) {
>                         pr_notice_client(cl, "mds%d is already opened\n",
>                                          session->s_mds);
> +                       session->s_features = features;

Xiubo, the metrics stuff isn't done here (as it's done in the else
case). That's probably required I guess??

>                 } else {
>                         session->s_state = CEPH_MDS_SESSION_OPEN;
>                         session->s_features = features;
> --
> 2.39.3
>
Venky Shankar Nov. 3, 2023, 9:29 a.m. UTC | #2
On Fri, Nov 3, 2023 at 1:23 PM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 11/3/23 14:43, Venky Shankar wrote:
> > On Fri, Nov 3, 2023 at 12:10 PM Venky Shankar <vshankar@redhat.com> wrote:
> >> Following along the same lines as per the user-space fix. Right
> >> now this isn't really an issue with the ceph kernel driver because
> >> of the feature bit laginess, however, that can change over time
> >> (when the new snaprealm info type is ported to the kernel driver)
> >> and depending on the MDS version that's being upgraded can cause
> >> message decoding issues - so, fix that early on.
> >>
> >> URL: Fixes: http://tracker.ceph.com/issues/63188
> >> Signed-off-by: Venky Shankar <vshankar@redhat.com>
> >> ---
> >>   fs/ceph/mds_client.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> >> index a7bffb030036..48d75e17115c 100644
> >> --- a/fs/ceph/mds_client.c
> >> +++ b/fs/ceph/mds_client.c
> >> @@ -4192,6 +4192,7 @@ static void handle_session(struct ceph_mds_session *session,
> >>                  if (session->s_state == CEPH_MDS_SESSION_OPEN) {
> >>                          pr_notice_client(cl, "mds%d is already opened\n",
> >>                                           session->s_mds);
> >> +                       session->s_features = features;
> > Xiubo, the metrics stuff isn't done here (as it's done in the else
> > case). That's probably required I guess??
>
> That should be okay, but it harmless to do it here.
>
> So let's just fix it by:
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 41be58baaa57..de3c6b6cbd07 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -4263,17 +4263,16 @@ static void handle_session(struct
> ceph_mds_session *session,
>                          pr_info_client(cl, "mds%d reconnect success\n",
>                                         session->s_mds);
>
> -               if (session->s_state == CEPH_MDS_SESSION_OPEN) {
> +               if (session->s_state == CEPH_MDS_SESSION_OPEN)
>                          pr_notice_client(cl, "mds%d is already opened\n",
>                                           session->s_mds);
> -               } else {
> +               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);
> -               }
> +               session->s_features = features;
> +               renewed_caps(mdsc, session, 0);

Call to renewed_caps() isn't really required if the session state is
already open, isn't it? Doesn't harm to call it I guess, but...

> +               if (test_bit(CEPHFS_FEATURE_METRIC_COLLECT,
> +                            &session->s_features))
> + metric_schedule_delayed(&mdsc->metric);
>
>                  /*
>                   * The connection maybe broken and the session in client
>
> Thanks
>
> - Xiubo
>
>
> >
> >>                  } else {
> >>                          session->s_state = CEPH_MDS_SESSION_OPEN;
> >>                          session->s_features = features;
> >> --
> >> 2.39.3
> >>
> >
>
Xiubo Li Nov. 6, 2023, 12:25 a.m. UTC | #3
On 11/3/23 17:29, Venky Shankar wrote:
> On Fri, Nov 3, 2023 at 1:23 PM Xiubo Li <xiubli@redhat.com> wrote:
>>
>> On 11/3/23 14:43, Venky Shankar wrote:
>>> On Fri, Nov 3, 2023 at 12:10 PM Venky Shankar <vshankar@redhat.com> wrote:
>>>> Following along the same lines as per the user-space fix. Right
>>>> now this isn't really an issue with the ceph kernel driver because
>>>> of the feature bit laginess, however, that can change over time
>>>> (when the new snaprealm info type is ported to the kernel driver)
>>>> and depending on the MDS version that's being upgraded can cause
>>>> message decoding issues - so, fix that early on.
>>>>
>>>> URL: Fixes: http://tracker.ceph.com/issues/63188
>>>> Signed-off-by: Venky Shankar <vshankar@redhat.com>
>>>> ---
>>>>    fs/ceph/mds_client.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>>> index a7bffb030036..48d75e17115c 100644
>>>> --- a/fs/ceph/mds_client.c
>>>> +++ b/fs/ceph/mds_client.c
>>>> @@ -4192,6 +4192,7 @@ static void handle_session(struct ceph_mds_session *session,
>>>>                   if (session->s_state == CEPH_MDS_SESSION_OPEN) {
>>>>                           pr_notice_client(cl, "mds%d is already opened\n",
>>>>                                            session->s_mds);
>>>> +                       session->s_features = features;
>>> Xiubo, the metrics stuff isn't done here (as it's done in the else
>>> case). That's probably required I guess??
>> That should be okay, but it harmless to do it here.
>>
>> So let's just fix it by:
>>
>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>> index 41be58baaa57..de3c6b6cbd07 100644
>> --- a/fs/ceph/mds_client.c
>> +++ b/fs/ceph/mds_client.c
>> @@ -4263,17 +4263,16 @@ static void handle_session(struct
>> ceph_mds_session *session,
>>                           pr_info_client(cl, "mds%d reconnect success\n",
>>                                          session->s_mds);
>>
>> -               if (session->s_state == CEPH_MDS_SESSION_OPEN) {
>> +               if (session->s_state == CEPH_MDS_SESSION_OPEN)
>>                           pr_notice_client(cl, "mds%d is already opened\n",
>>                                            session->s_mds);
>> -               } else {
>> +               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);
>> -               }
>> +               session->s_features = features;
>> +               renewed_caps(mdsc, session, 0);
> Call to renewed_caps() isn't really required if the session state is
> already open, isn't it? Doesn't harm to call it I guess, but...

Yeah.

Then let's just do:

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 41be58baaa57..45d0f445cdef 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -4263,12 +4263,12 @@ static void handle_session(struct 
ceph_mds_session *session,
                         pr_info_client(cl, "mds%d reconnect success\n",
                                        session->s_mds);

+               session->s_features = features;
                 if (session->s_state == CEPH_MDS_SESSION_OPEN) {
                         pr_notice_client(cl, "mds%d is already opened\n",
                                          session->s_mds);
                 } 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))

Thanks

- Xiubo


>> +               if (test_bit(CEPHFS_FEATURE_METRIC_COLLECT,
>> +                            &session->s_features))
>> + metric_schedule_delayed(&mdsc->metric);
>>
>>                   /*
>>                    * The connection maybe broken and the session in client
>>
>> Thanks
>>
>> - Xiubo
>>
>>
>>>>                   } else {
>>>>                           session->s_state = CEPH_MDS_SESSION_OPEN;
>>>>                           session->s_features = features;
>>>> --
>>>> 2.39.3
>>>>
>
Venky Shankar Nov. 6, 2023, 4:33 a.m. UTC | #4
Done - updated (didn't add v2 to the pathset though).

On Mon, Nov 6, 2023 at 5:55 AM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 11/3/23 17:29, Venky Shankar wrote:
> > On Fri, Nov 3, 2023 at 1:23 PM Xiubo Li <xiubli@redhat.com> wrote:
> >>
> >> On 11/3/23 14:43, Venky Shankar wrote:
> >>> On Fri, Nov 3, 2023 at 12:10 PM Venky Shankar <vshankar@redhat.com> wrote:
> >>>> Following along the same lines as per the user-space fix. Right
> >>>> now this isn't really an issue with the ceph kernel driver because
> >>>> of the feature bit laginess, however, that can change over time
> >>>> (when the new snaprealm info type is ported to the kernel driver)
> >>>> and depending on the MDS version that's being upgraded can cause
> >>>> message decoding issues - so, fix that early on.
> >>>>
> >>>> URL: Fixes: http://tracker.ceph.com/issues/63188
> >>>> Signed-off-by: Venky Shankar <vshankar@redhat.com>
> >>>> ---
> >>>>    fs/ceph/mds_client.c | 1 +
> >>>>    1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> >>>> index a7bffb030036..48d75e17115c 100644
> >>>> --- a/fs/ceph/mds_client.c
> >>>> +++ b/fs/ceph/mds_client.c
> >>>> @@ -4192,6 +4192,7 @@ static void handle_session(struct ceph_mds_session *session,
> >>>>                   if (session->s_state == CEPH_MDS_SESSION_OPEN) {
> >>>>                           pr_notice_client(cl, "mds%d is already opened\n",
> >>>>                                            session->s_mds);
> >>>> +                       session->s_features = features;
> >>> Xiubo, the metrics stuff isn't done here (as it's done in the else
> >>> case). That's probably required I guess??
> >> That should be okay, but it harmless to do it here.
> >>
> >> So let's just fix it by:
> >>
> >> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> >> index 41be58baaa57..de3c6b6cbd07 100644
> >> --- a/fs/ceph/mds_client.c
> >> +++ b/fs/ceph/mds_client.c
> >> @@ -4263,17 +4263,16 @@ static void handle_session(struct
> >> ceph_mds_session *session,
> >>                           pr_info_client(cl, "mds%d reconnect success\n",
> >>                                          session->s_mds);
> >>
> >> -               if (session->s_state == CEPH_MDS_SESSION_OPEN) {
> >> +               if (session->s_state == CEPH_MDS_SESSION_OPEN)
> >>                           pr_notice_client(cl, "mds%d is already opened\n",
> >>                                            session->s_mds);
> >> -               } else {
> >> +               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);
> >> -               }
> >> +               session->s_features = features;
> >> +               renewed_caps(mdsc, session, 0);
> > Call to renewed_caps() isn't really required if the session state is
> > already open, isn't it? Doesn't harm to call it I guess, but...
>
> Yeah.
>
> Then let's just do:
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 41be58baaa57..45d0f445cdef 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -4263,12 +4263,12 @@ static void handle_session(struct
> ceph_mds_session *session,
>                          pr_info_client(cl, "mds%d reconnect success\n",
>                                         session->s_mds);
>
> +               session->s_features = features;
>                  if (session->s_state == CEPH_MDS_SESSION_OPEN) {
>                          pr_notice_client(cl, "mds%d is already opened\n",
>                                           session->s_mds);
>                  } 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))
>
> Thanks
>
> - Xiubo
>
>
> >> +               if (test_bit(CEPHFS_FEATURE_METRIC_COLLECT,
> >> +                            &session->s_features))
> >> + metric_schedule_delayed(&mdsc->metric);
> >>
> >>                   /*
> >>                    * The connection maybe broken and the session in client
> >>
> >> Thanks
> >>
> >> - Xiubo
> >>
> >>
> >>>>                   } else {
> >>>>                           session->s_state = CEPH_MDS_SESSION_OPEN;
> >>>>                           session->s_features = features;
> >>>> --
> >>>> 2.39.3
> >>>>
> >
>
diff mbox series

Patch

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index a7bffb030036..48d75e17115c 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -4192,6 +4192,7 @@  static void handle_session(struct ceph_mds_session *session,
 		if (session->s_state == CEPH_MDS_SESSION_OPEN) {
 			pr_notice_client(cl, "mds%d is already opened\n",
 					 session->s_mds);
+			session->s_features = features;
 		} else {
 			session->s_state = CEPH_MDS_SESSION_OPEN;
 			session->s_features = features;