| Message ID | 20231103064027.316296-1-vshankar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: reinitialize mds feature bit even when session in open | expand |
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 >
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); + 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 >> >
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 > >> > > >
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 >>>> >
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 --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;
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(+)