Re: [PATCH 5/5] ceph: fix ceph feature bits

[Xiubo Li <xiubli@redhat.com>]

On 6/30/21 8:52 PM, Ilya Dryomov wrote:
> On Wed, Jun 30, 2021 at 2:05 PM Jeff Layton <jlayton@kernel.org> wrote:
>> On Wed, 2021-06-30 at 08:52 +0800, Xiubo Li wrote:
>>> On 6/29/21 11:38 PM, Jeff Layton wrote:
>>>> On Tue, 2021-06-29 at 12:42 +0800, xiubli@redhat.com wrote:
>>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>>
>>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>>> ---
>>>>>    fs/ceph/mds_client.h | 4 ++++
>>>>>    1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>>>>> index 79d5b8ed62bf..b18eded84ede 100644
>>>>> --- a/fs/ceph/mds_client.h
>>>>> +++ b/fs/ceph/mds_client.h
>>>>> @@ -27,7 +27,9 @@ enum ceph_feature_type {
>>>>>            CEPHFS_FEATURE_RECLAIM_CLIENT,
>>>>>            CEPHFS_FEATURE_LAZY_CAP_WANTED,
>>>>>            CEPHFS_FEATURE_MULTI_RECONNECT,
>>>>> + CEPHFS_FEATURE_NAUTILUS,
>>>>>            CEPHFS_FEATURE_DELEG_INO,
>>>>> + CEPHFS_FEATURE_OCTOPUS,
>>>>>            CEPHFS_FEATURE_METRIC_COLLECT,
>>>>>
>>>>>            CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_METRIC_COLLECT,
>>>>> @@ -43,7 +45,9 @@ enum ceph_feature_type {
>>>>>            CEPHFS_FEATURE_REPLY_ENCODING,          \
>>>>>            CEPHFS_FEATURE_LAZY_CAP_WANTED,         \
>>>>>            CEPHFS_FEATURE_MULTI_RECONNECT,         \
>>>>> + CEPHFS_FEATURE_NAUTILUS,                \
>>>>>            CEPHFS_FEATURE_DELEG_INO,               \
>>>>> + CEPHFS_FEATURE_OCTOPUS,                 \
>>>>>            CEPHFS_FEATURE_METRIC_COLLECT,          \
>>>>>                                                    \
>>>>>            CEPHFS_FEATURE_MAX,                     \
>>>> Do we need this? I thought we had decided to deprecate the whole concept
>>>> of release-based feature flags.
>>> This was inconsistent with the MDS side, that means if the MDS only
>>> support CEPHFS_FEATURE_DELEG_INO at most in lower version of ceph
>>> cluster, then the kclients will try to send the metric messages to
>>> MDSes, which could crash the MDS daemons.
>>>
>>> For the ceph version feature flags they are redundant since we can check
>>> this from the con's, since pacific the MDS code stopped updating it. I
>>> assume we should deprecate it since Pacific ?
>>>
>> I believe so. Basically the version-based features aren't terribly
>> useful. Mostly we want to check feature flags for specific features
>> themselves. Since there are no other occurrences of
>> CEPHFS_FEATURE_NAUTILUS or CEPHFS_FEATURE_OCTOPUS symbols, it's probably
>> best not to define them at all.

Without add this two symbols, we could just correct the number of them, 
make sense ?


> Not only that but this patch as is would break CEPHFS_FEATURE_DELEG_INO
> and CEPHFS_FEATURE_METRIC_COLLECT checks in the kernel because their bit
> numbers would change...

Yeah, the problem is that the numbers of them was incorrect, which was 
introducing crash bugs.

Thanks

Xiubo

> Thanks,
>
>                  Ilya
>