mds: make num_fwd and num_retry to __u32 by lxbsz · Pull Request #45669 · ceph/ceph

lxbsz · 2022-03-28T12:22:10Z

The num_fwd in MClientRequestForward is int32_t, while the num_fwd
in ceph_mds_request_head is __u8. This is buggy when the num_fwd
is larger than 256 it will always be truncate to 0 again. But the
client couldn't recoginize this.

Fixes: https://tracker.ceph.com/issues/57854
Signed-off-by: Xiubo Li xiubli@redhat.com

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

lxbsz · 2022-03-28T13:05:41Z

IMO we need to limit the total number of the num_fwd instead of infinitely looping like in #45073.

src/include/ceph_fs.h

lxbsz · 2022-04-06T02:44:41Z

Have fixed this in #45728 and #45688 to hard limit the retry times to 256 as a workaround currently.

While we should fix this anyway, because that workaround is ugly.

src/include/ceph_fs.h

src/client/Client.cc

kotreshhr · 2022-07-06T07:32:00Z

Just curious to understand, you mentioned that this issue being fixed is easily seen with the PR #45073 (*#45073 (comment))**. The num_fwd is per request right? Does a single create request caused this to overflow ?

lxbsz · 2022-07-06T08:34:47Z

Just curious to understand, you mentioned that this issue being fixed is easily seen with the PR #45073 (*#45073 (comment))**. The num_fwd is per request right?

Correct.

Does a single create request caused this to overflow ?

Yeah, it is. A single create request was bouncing between mds.0 and mds.1 infinitely and then overflowed.

lxbsz · 2022-07-06T08:35:21Z

jenkins test make check

kotreshhr · 2022-07-06T13:05:23Z

Just curious to understand, you mentioned that this issue being fixed is easily seen with the PR #45073 (*#45073 (comment))**. The num_fwd is per request right?

Correct.

Does a single create request caused this to overflow ?

Yeah, it is. A single create request was bouncing between mds.0 and mds.1 infinitely and then overflowed.

If the CInode and CDir of the directory are on different mdses, I am failing to understand why it should take more than 256 hops? Is this some corner case that is being hit here ?

kotreshhr · 2022-07-06T13:07:25Z

The PR and the commits miss the tracker id. Doesn't this need backport ?

lxbsz · 2022-07-07T01:33:51Z

Just curious to understand, you mentioned that this issue being fixed is easily seen with the PR #45073 (*#45073 (comment))**. The num_fwd is per request right?

Correct.

Does a single create request caused this to overflow ?

Yeah, it is. A single create request was bouncing between mds.0 and mds.1 infinitely and then overflowed.

If the CInode and CDir of the directory are on different mdses, I am failing to understand why it should take more than 256 hops? Is this some corner case that is being hit here ?

As mentioned in PR #45073, this bug was found from @luis-henrix previous patches, which has been fixed after that. Except that as I know there is another case that when the pool is full, it will forward the create requests to the CInode auth MDS which could cause the requests hopping, maybe there are some others cases we haven't figured out yet which also could cause this.

For the retry attempt one, it should be triggered by the clients instead, such as when reconnecting.

Anyway the type mismatch issue should be fixed, which introduced confusion in the mds logs.

lxbsz · 2022-07-07T01:34:38Z

The PR and the commits miss the tracker id. Doesn't this need backport ?

IMO it should be okay, since the client side fixes have been backported.

kotreshhr

Looks good to me otherwise

src/client/Client.cc

gregsfortytwo · 2022-07-07T14:21:08Z

@lxbsz Can you talk about how you made sure these encoding changes maintain compatibility with old clients? I see you're changing the ceph_mds_request_head from a WRITE_RAW_ENCODER to hand-rolled encode/decode functions, and it's possible to do that correctly but makes me nervous — we need to be very careful with it.

I also note that it looks like the changes mean the MDS simply ignores the "classic" u8 num_fwd, which seems like it will break old clients if the MDS forwards requests around?

lxbsz · 2022-07-08T02:01:36Z

@lxbsz Can you talk about how you made sure these encoding changes maintain compatibility with old clients? I see you're changing the ceph_mds_request_head from a WRITE_RAW_ENCODER to hand-rolled encode/decode functions, and it's possible to do that correctly but makes me nervous — we need to be very careful with it.

In the MDS side it will only maintain ext_num_retry and increase the new ext_num_fwd counters. And at the same time will assign them to the old num_retry/num_fwd counters, which maybe overflowed.

The old clients are still using the old num_retry/num_fwd counters. For the very old clients who haven't contained #45728 and #45688 fixes yet, the bugs still exists and we cannot avoid it, otherwise with #45728 and #45688 the clients could stop retrying/forwarding the requests if exceeding 256 times.

I also note that it looks like the changes mean the MDS simply ignores the "classic" u8 num_fwd, which seems like it will break old clients if the MDS forwards requests around?

In the MDS side it will only maintain ext_num_retry and increase the new ext_num_fwd counters, and will pass them back to clients via the old counters at the same time.

NOTE: the retry counter will be increased by the clients.

As above mentioned, for the old clients they will continue parsing and using the old counters and in the MDS side the new counters are just two relay stations, receiving the values from clients and saving them and then passing them back via the old counters.

====

Okay, I found bug:

In old clients when parsing the new struct ceph_mds_request_head they will just copy it. And will left 8 bytes memories to the next decoder. For this IMO we should also test the CEPHFS_FEATURE_32BITS_RETRY_FWD feature bit to distinguish the old clients.

vshankar · 2023-02-15T14:08:20Z

This is good to merge. @lxbsz please rebase.

Prepare switching num_retry and num_fwd to __le32. Fixes: https://tracker.ceph.com/issues/57854 Signed-off-by: Xiubo Li <xiubli@redhat.com>

To make sure both the num_retry and num_fwd in all the structs have the same type, which is 32 bits. And also for old cephs and clients they still could use the 8 bits members. Fixes: https://tracker.ceph.com/issues/57854 Signed-off-by: Xiubo Li <xiubli@redhat.com>

Check the CEPHFS_FEATURE_32BITS_RETRY_FWD feature bit and if not set, that means it's connecting to an old MDS and will limit the max retry to 256 times. Fixes: https://tracker.ceph.com/issues/57854 Signed-off-by: Xiubo Li <xiubli@redhat.com>

The MDS will increase the forward count, and if the forward count is less than the one saved in request in client side, that means the MDS is old version and it was overflowed. Then just stop forwarding. Fixes: https://tracker.ceph.com/issues/57854 Signed-off-by: Xiubo Li <xiubli@redhat.com>

lxbsz · 2023-02-16T00:51:46Z

This is good to merge. @lxbsz please rebase.

Done. The conflicts were introduced by one old commit, which is doing -EMULTIHOP --> -CEPHFS_EMULTIHOP

Comments were addressed.

…quest_head_legacy When a client is too old and uses struct ceph_mds_request_head_legacy we must fill new ext_num_retry and ext_num_fwd fields from an old num_retry and num_fwd. Fixes: ceph#45669 Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>

…quest_head_legacy When a client is too old and uses struct ceph_mds_request_head_legacy we must fill new ext_num_retry and ext_num_fwd fields from an old num_retry and num_fwd. Fixes: ceph#45669 Fixes: https://tracker.ceph.com/issues/63288 Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>

…quest_head_legacy When a client is too old and uses struct ceph_mds_request_head_legacy we must fill new ext_num_retry and ext_num_fwd fields from an old num_retry and num_fwd. Fixes: ceph#45669 Fixes: https://tracker.ceph.com/issues/63288 Fixes: commit cbd7e30 ("ceph_fs.h: add 32 bits extended num_retry and num_fwd support") Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>

…quest_head_legacy When a client is too old and uses struct ceph_mds_request_head_legacy we must fill new ext_num_retry and ext_num_fwd fields from an old num_retry and num_fwd. Fixes: ceph#45669 Fixes: https://tracker.ceph.com/issues/63288 Fixes: commit cbd7e30 ("ceph_fs.h: add 32 bits extended num_retry and num_fwd support") Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> (cherry picked from commit 43f32a4)

…quest_head_legacy When a client is too old and uses struct ceph_mds_request_head_legacy we must fill new ext_num_retry and ext_num_fwd fields from an old num_retry and num_fwd. Fixes: ceph/ceph#45669 Fixes: https://tracker.ceph.com/issues/63288 Fixes: commit cbd7e30 ("ceph_fs.h: add 32 bits extended num_retry and num_fwd support") Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> (cherry picked from commit 43f32a4)

…quest_head_legacy When a client is too old and uses struct ceph_mds_request_head_legacy we must fill new ext_num_retry and ext_num_fwd fields from an old num_retry and num_fwd. Fixes: ceph/ceph#45669 Fixes: https://tracker.ceph.com/issues/63288 Fixes: commit cbd7e30 ("ceph_fs.h: add 32 bits extended num_retry and num_fwd support") Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> (cherry picked from commit 43f32a4) (cherry picked from commit 312bb5b)

…quest_head_legacy When a client is too old and uses struct ceph_mds_request_head_legacy we must fill new ext_num_retry and ext_num_fwd fields from an old num_retry and num_fwd. Fixes: ceph#45669 Fixes: https://tracker.ceph.com/issues/63288 Fixes: commit cbd7e30 ("ceph_fs.h: add 32 bits extended num_retry and num_fwd support") Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>

…quest_head_legacy When a client is too old and uses struct ceph_mds_request_head_legacy we must fill new ext_num_retry and ext_num_fwd fields from an old num_retry and num_fwd. Fixes: ceph#45669 Fixes: https://tracker.ceph.com/issues/63288 Fixes: commit cbd7e30 ("ceph_fs.h: add 32 bits extended num_retry and num_fwd support") Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> (cherry picked from commit 43f32a4)

lxbsz requested a review from a team March 28, 2022 12:22

github-actions bot added the cephfs Ceph File System label Mar 28, 2022

lxbsz requested review from jtlayton, luis-henrix and vshankar March 28, 2022 12:22

lxbsz mentioned this pull request Mar 28, 2022

mds: add protection from clients without fscrypt support #45073

Closed

gregsfortytwo previously requested changes Mar 28, 2022

View reviewed changes

src/include/ceph_fs.h Outdated Show resolved Hide resolved

lxbsz requested a review from gregsfortytwo March 28, 2022 14:17

lxbsz marked this pull request as draft March 30, 2022 06:58

jtlayton previously requested changes Apr 18, 2022

View reviewed changes

src/include/ceph_fs.h Outdated Show resolved Hide resolved

djgalloway changed the base branch from master to main May 25, 2022 20:01

lxbsz marked this pull request as ready for review July 5, 2022 08:34

lxbsz force-pushed the header branch 3 times, most recently from 6a0669b to f465af3 Compare July 6, 2022 02:33

lxbsz requested a review from a team July 6, 2022 02:34

kotreshhr reviewed Jul 6, 2022

View reviewed changes

src/client/Client.cc Outdated Show resolved Hide resolved

lxbsz force-pushed the header branch from f465af3 to ab34a10 Compare July 6, 2022 07:25

kotreshhr reviewed Jul 7, 2022

View reviewed changes

src/client/Client.cc Outdated Show resolved Hide resolved

lxbsz added 4 commits February 16, 2023 08:44

ceph_fs.h: switch to use its own encode/decode helpers

82d7d20

Prepare switching num_retry and num_fwd to __le32. Fixes: https://tracker.ceph.com/issues/57854 Signed-off-by: Xiubo Li <xiubli@redhat.com>

client: switch to use 32 bits ext_num_retry

4fb0417

Check the CEPHFS_FEATURE_32BITS_RETRY_FWD feature bit and if not set, that means it's connecting to an old MDS and will limit the max retry to 256 times. Fixes: https://tracker.ceph.com/issues/57854 Signed-off-by: Xiubo Li <xiubli@redhat.com>

lxbsz force-pushed the header branch from 1b8e990 to 3584ecc Compare February 16, 2023 00:50

github-actions bot removed the needs-rebase label Feb 16, 2023

vshankar merged commit 8ce3185 into main Feb 22, 2023

vshankar deleted the header branch February 22, 2023 09:13

This was referenced Feb 27, 2023

qa: make the xfstests tmp directories to be readable and excutable #50217

Merged

mds: memset the head by default to fix the readable.sh test failure #50268

Merged

This was referenced Mar 29, 2023

quincy: mds: make num_fwd and num_retry to __u32 #50732

Merged

pacific: mds: make num_fwd and num_retry to __u32 #50733

Merged

mihalicyn mentioned this pull request Jul 24, 2023

ceph_fs.h: add separate owner_{u,g}id fields #52575

Merged

14 tasks

Conversation

lxbsz commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

lxbsz commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lxbsz commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kotreshhr commented Jul 6, 2022

Uh oh!

lxbsz commented Jul 6, 2022

Uh oh!

lxbsz commented Jul 6, 2022

Uh oh!

kotreshhr commented Jul 6, 2022

Uh oh!

kotreshhr commented Jul 6, 2022

Uh oh!

lxbsz commented Jul 7, 2022

Uh oh!

lxbsz commented Jul 7, 2022

Uh oh!

kotreshhr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gregsfortytwo commented Jul 7, 2022

Uh oh!

lxbsz commented Jul 8, 2022

Uh oh!

vshankar commented Feb 15, 2023

Uh oh!

lxbsz commented Feb 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lxbsz commented Mar 28, 2022 •

edited

Loading

lxbsz commented Mar 28, 2022 •

edited

Loading

lxbsz commented Apr 6, 2022 •

edited

Loading