diff mbox series

[v3] ceph: reverse MDSMap dencoding of max_xattr_size/bal_rank_mask

Message ID 20240221045158.75644-1-xiubli@redhat.com
State Superseded
Headers show
Series [v3] ceph: reverse MDSMap dencoding of max_xattr_size/bal_rank_mask | expand

Commit Message

Xiubo Li Feb. 21, 2024, 4:51 a.m. UTC 
From: Xiubo Li <xiubli@redhat.com>

Ceph added the bal_rank_mask with encoded (ev) version 17.  This
was merged into main Oct 2022 and made it into the reef release
normally. While a latter commit added the max_xattr_size also
with encoded (ev) version 17 but places it before bal_rank_mask.

And this will breaks some usages, for example when upgrading old
cephs to newer versions.

URL: https://tracker.ceph.com/issues/64440
Reported-by: Patrick Donnelly <pdonnell@redhat.com>
Signed-off-by: Xiubo Li <xiubli@redhat.com>
Reviewed-by: Patrick Donnelly <pdonnell@ibm.com>
Reviewed-by: Venky Shankar <vshankar@redhat.com>
---

V3:
- Fix the comment suggested by Patrick in V2.


 fs/ceph/mdsmap.c | 7 ++++---
 fs/ceph/mdsmap.h | 6 +++++-
 2 files changed, 9 insertions(+), 4 deletions(-)

Comments

Ilya Dryomov Feb. 26, 2024, 6:32 p.m. UTC | #1 
On Wed, Feb 21, 2024 at 5:54 AM <xiubli@redhat.com> wrote:
>
> From: Xiubo Li <xiubli@redhat.com>
>
> Ceph added the bal_rank_mask with encoded (ev) version 17.  This
> was merged into main Oct 2022 and made it into the reef release
> normally. While a latter commit added the max_xattr_size also
> with encoded (ev) version 17 but places it before bal_rank_mask.
>
> And this will breaks some usages, for example when upgrading old
> cephs to newer versions.
>
> URL: https://tracker.ceph.com/issues/64440
> Reported-by: Patrick Donnelly <pdonnell@redhat.com>
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> Reviewed-by: Patrick Donnelly <pdonnell@ibm.com>
> Reviewed-by: Venky Shankar <vshankar@redhat.com>
> ---
>
> V3:
> - Fix the comment suggested by Patrick in V2.
>
>
>  fs/ceph/mdsmap.c | 7 ++++---
>  fs/ceph/mdsmap.h | 6 +++++-
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/mdsmap.c b/fs/ceph/mdsmap.c
> index fae97c25ce58..8109aba66e02 100644
> --- a/fs/ceph/mdsmap.c
> +++ b/fs/ceph/mdsmap.c
> @@ -380,10 +380,11 @@ struct ceph_mdsmap *ceph_mdsmap_decode(struct ceph_mds_client *mdsc, void **p,
>                 ceph_decode_skip_8(p, end, bad_ext);
>                 /* required_client_features */
>                 ceph_decode_skip_set(p, end, 64, bad_ext);
> +               /* bal_rank_mask */
> +               ceph_decode_skip_string(p, end, bad_ext);
> +       }
> +       if (mdsmap_ev >= 18) {
>                 ceph_decode_64_safe(p, end, m->m_max_xattr_size, bad_ext);
> -       } else {
> -               /* This forces the usage of the (sync) SETXATTR Op */
> -               m->m_max_xattr_size = 0;
>         }
>  bad_ext:
>         doutc(cl, "m_enabled: %d, m_damaged: %d, m_num_laggy: %d\n",
> diff --git a/fs/ceph/mdsmap.h b/fs/ceph/mdsmap.h
> index 89f1931f1ba6..1f2171dd01bf 100644
> --- a/fs/ceph/mdsmap.h
> +++ b/fs/ceph/mdsmap.h
> @@ -27,7 +27,11 @@ struct ceph_mdsmap {
>         u32 m_session_timeout;          /* seconds */
>         u32 m_session_autoclose;        /* seconds */
>         u64 m_max_file_size;
> -       u64 m_max_xattr_size;           /* maximum size for xattrs blob */
> +       /*
> +        * maximum size for xattrs blob.
> +        * Zeroed by default to force the usage of the (sync) SETXATTR Op.
> +        */
> +       u64 m_max_xattr_size;
>         u32 m_max_mds;                  /* expected up:active mds number */
>         u32 m_num_active_mds;           /* actual up:active mds number */
>         u32 possible_max_rank;          /* possible max rank index */
> --
> 2.43.0
>

I have expanded the changelog and tagged stable as we definitely want
this backported.  Please let me know if I got something wrong:

https://github.com/ceph/ceph-client/commit/51d31149a88b5c5a8d2d33f06df93f6187a25b4c

Thanks,

                Ilya
Xiubo Li Feb. 27, 2024, 12:01 a.m. UTC | #2 
On 2/27/24 02:32, Ilya Dryomov wrote:
> On Wed, Feb 21, 2024 at 5:54 AM <xiubli@redhat.com> wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> Ceph added the bal_rank_mask with encoded (ev) version 17.  This
>> was merged into main Oct 2022 and made it into the reef release
>> normally. While a latter commit added the max_xattr_size also
>> with encoded (ev) version 17 but places it before bal_rank_mask.
>>
>> And this will breaks some usages, for example when upgrading old
>> cephs to newer versions.
>>
>> URL: https://tracker.ceph.com/issues/64440
>> Reported-by: Patrick Donnelly <pdonnell@redhat.com>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> Reviewed-by: Patrick Donnelly <pdonnell@ibm.com>
>> Reviewed-by: Venky Shankar <vshankar@redhat.com>
>> ---
>>
>> V3:
>> - Fix the comment suggested by Patrick in V2.
>>
>>
>>   fs/ceph/mdsmap.c | 7 ++++---
>>   fs/ceph/mdsmap.h | 6 +++++-
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ceph/mdsmap.c b/fs/ceph/mdsmap.c
>> index fae97c25ce58..8109aba66e02 100644
>> --- a/fs/ceph/mdsmap.c
>> +++ b/fs/ceph/mdsmap.c
>> @@ -380,10 +380,11 @@ struct ceph_mdsmap *ceph_mdsmap_decode(struct ceph_mds_client *mdsc, void **p,
>>                  ceph_decode_skip_8(p, end, bad_ext);
>>                  /* required_client_features */
>>                  ceph_decode_skip_set(p, end, 64, bad_ext);
>> +               /* bal_rank_mask */
>> +               ceph_decode_skip_string(p, end, bad_ext);
>> +       }
>> +       if (mdsmap_ev >= 18) {
>>                  ceph_decode_64_safe(p, end, m->m_max_xattr_size, bad_ext);
>> -       } else {
>> -               /* This forces the usage of the (sync) SETXATTR Op */
>> -               m->m_max_xattr_size = 0;
>>          }
>>   bad_ext:
>>          doutc(cl, "m_enabled: %d, m_damaged: %d, m_num_laggy: %d\n",
>> diff --git a/fs/ceph/mdsmap.h b/fs/ceph/mdsmap.h
>> index 89f1931f1ba6..1f2171dd01bf 100644
>> --- a/fs/ceph/mdsmap.h
>> +++ b/fs/ceph/mdsmap.h
>> @@ -27,7 +27,11 @@ struct ceph_mdsmap {
>>          u32 m_session_timeout;          /* seconds */
>>          u32 m_session_autoclose;        /* seconds */
>>          u64 m_max_file_size;
>> -       u64 m_max_xattr_size;           /* maximum size for xattrs blob */
>> +       /*
>> +        * maximum size for xattrs blob.
>> +        * Zeroed by default to force the usage of the (sync) SETXATTR Op.
>> +        */
>> +       u64 m_max_xattr_size;
>>          u32 m_max_mds;                  /* expected up:active mds number */
>>          u32 m_num_active_mds;           /* actual up:active mds number */
>>          u32 possible_max_rank;          /* possible max rank index */
>> --
>> 2.43.0
>>
> I have expanded the changelog and tagged stable as we definitely want
> this backported.  Please let me know if I got something wrong:
>
> https://github.com/ceph/ceph-client/commit/51d31149a88b5c5a8d2d33f06df93f6187a25b4c

Thanks Ilya. Looks nice.

Yeah, we need to backport this asap.

- Xiubo


> Thanks,
>
>                  Ilya
>
diff mbox series

Patch

diff --git a/fs/ceph/mdsmap.c b/fs/ceph/mdsmap.c
index fae97c25ce58..8109aba66e02 100644
--- a/fs/ceph/mdsmap.c
+++ b/fs/ceph/mdsmap.c
@@ -380,10 +380,11 @@  struct ceph_mdsmap *ceph_mdsmap_decode(struct ceph_mds_client *mdsc, void **p,
 		ceph_decode_skip_8(p, end, bad_ext);
 		/* required_client_features */
 		ceph_decode_skip_set(p, end, 64, bad_ext);
+		/* bal_rank_mask */
+		ceph_decode_skip_string(p, end, bad_ext);
+	}
+	if (mdsmap_ev >= 18) {
 		ceph_decode_64_safe(p, end, m->m_max_xattr_size, bad_ext);
-	} else {
-		/* This forces the usage of the (sync) SETXATTR Op */
-		m->m_max_xattr_size = 0;
 	}
 bad_ext:
 	doutc(cl, "m_enabled: %d, m_damaged: %d, m_num_laggy: %d\n",
diff --git a/fs/ceph/mdsmap.h b/fs/ceph/mdsmap.h
index 89f1931f1ba6..1f2171dd01bf 100644
--- a/fs/ceph/mdsmap.h
+++ b/fs/ceph/mdsmap.h
@@ -27,7 +27,11 @@  struct ceph_mdsmap {
 	u32 m_session_timeout;          /* seconds */
 	u32 m_session_autoclose;        /* seconds */
 	u64 m_max_file_size;
-	u64 m_max_xattr_size;		/* maximum size for xattrs blob */
+	/*
+	 * maximum size for xattrs blob.
+	 * Zeroed by default to force the usage of the (sync) SETXATTR Op.
+	 */
+	u64 m_max_xattr_size;
 	u32 m_max_mds;			/* expected up:active mds number */
 	u32 m_num_active_mds;		/* actual up:active mds number */
 	u32 possible_max_rank;		/* possible max rank index */