client: readdir_r_cb: get rstat for dir only if using rbytes for size by xdavidwu · Pull Request #46960 · ceph/ceph

xdavidwu · 2022-07-05T08:43:56Z

When client_dirsize_rbytes is off, there should be no need for getting
rstat on readdir operations.

This fixes performance when client_dirsize_rbytes is off after #38222.

Fixes: https://tracker.ceph.com/issues/61999
Signed-off-by: Pinghao Wu xdavidwuph@gmail.com

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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

src/client/Client.cc

lxbsz

LGTM

vshankar

LGTM. @rishabh-d-dave please pick thi sup in your next test run.

rishabh-d-dave · 2023-03-31T18:47:00Z

https://tracker.ceph.com/projects/cephfs/wiki/Main#31-Mar-2023

rishabh-d-dave · 2023-03-31T18:51:13Z

@xdavidwu Following job failed integration tests several times.

http://pulpito.front.sepia.ceph.com/rishabh-2023-03-03_21:39:49-fs-wip-rishabh-2023Mar03-2316-testing-default-smithi/7193834
http://pulpito.front.sepia.ceph.com/rishabh-2023-03-03_21:39:49-fs-wip-rishabh-2023Mar03-2316-testing-default-smithi/7194026
http://pulpito.front.sepia.ceph.com/rishabh-2023-03-11_05:54:03-fs-wip-rishabh-2023Mar10-1727-testing-default-smithi/7204055

And when this PR was removed from the testing branch this jobs passed -

http://pulpito.front.sepia.ceph.com/rishabh-2023-03-23_08:27:28-fs-wip-rishabh-2023Mar20-2250-testing-default-smithi/7217379

So this failure is due to this PR. Please take a look.

When this PR is ready for testing, please let me know. I'll put it through test again.

xdavidwu · 2023-04-01T14:22:17Z

@rishabh-d-dave Thanks, I think this is ready for another test round.

vshankar · 2023-04-03T02:53:53Z

@rishabh-d-dave Thanks, I think this is ready for another test round.

@xdavidwu Could you describe the bug that @rishabh-d-dave found in the test run and the fix? Its becomes helpful to not go over the entire changes again.

xdavidwu · 2023-04-03T03:57:09Z

I think the cause of test failures were that in tests, ceph-fuse get the client_dirsize_rbytes = false via mon, not directly from config, and in previous version, mds-client: make client_dirsize_rbytes unchangeable in runtime, no_mon_update flag was added to client_dirsize_rbytes so it was not possible to set it via mon.

The original intention is to prevent client_dirsize_rbytes being changable at runtime. After some tests I found out that only startup flag is needed for that, not no_mon_update.

In the newer version, no_mon_update on client_dirsize_rbytes is removed.

vshankar · 2023-04-03T11:02:17Z

I think the cause of test failures were that in tests, ceph-fuse get the client_dirsize_rbytes = false via mon, not directly from config, and in previous version, mds-client: make client_dirsize_rbytes unchangeable in runtime, no_mon_update flag was added to client_dirsize_rbytes so it was not possible to set it via mon.

Ah! I see client_dirsize_rbytes = False in the test run.

github-actions · 2023-06-04T15:01:20Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

rishabh-d-dave

Tests were successful - https://tracker.ceph.com/projects/cephfs/wiki/Main#13-Jul-2023

When `client_dirsize_rbytes` is off, there should be no need for getting rstat on readdir operations. This fixes performance when `client_dirsize_rbytes` is off after ceph#38222. Fixes: https://tracker.ceph.com/issues/61999 Signed-off-by: Pinghao Wu <xdavidwuph@gmail.com>

kclient also has this option, and is unchangeable unless remounted. Let's keep parity with kclient. Signed-off-by: Pinghao Wu <xdavidwuph@gmail.com>

rishabh-d-dave · 2023-07-13T08:51:37Z

@xdavidwu Hi. The tests ran successfully. This PR is ready for merge. But this patch needs to backported to older version of Ceph and therefore a tracker ticket is required. I've created the tracker ticket for you - https://tracker.ceph.com/issues/61999. You need to refer it in your commit message by adding this line: Fixes: https://tracker.ceph.com/issues/61999. After this is done, I can proceed to merge the PR.

Thank you for the contributing to CephFS. :)

rishabh-d-dave · 2023-07-13T08:58:32Z

@xdavidwu I've just updated the commit message for you (earlier I couldn't update the PR branch).

rishabh-d-dave · 2023-07-13T08:59:40Z

@xdavidwu I've just updated the commit message for you (earlier I couldn't update the PR branch).

Oh, on a refresh I see you had done it already. I am sorry about that. Feel free to push again if there's some mistake in branch I've pushed.

Sorry to bother you. This would've been unnecessary if I pushing to your branch was successful earlier.

github-actions bot added the cephfs Ceph File System label Jul 5, 2022

lxbsz reviewed Jul 5, 2022

View reviewed changes

src/client/Client.cc Show resolved Hide resolved

vshankar requested a review from a team July 8, 2022 05:04

github-actions bot added the common label Jul 14, 2022

lxbsz approved these changes Jul 15, 2022

View reviewed changes

vshankar reviewed Dec 1, 2022

View reviewed changes

rishabh-d-dave added needs-qa wip-rishabh-testing Rishabh's testing label labels Jan 31, 2023

rishabh-d-dave removed needs-qa wip-rishabh-testing Rishabh's testing label labels Mar 31, 2023

xdavidwu force-pushed the client-readdir-performance branch from 30c8546 to b43e31c Compare April 1, 2023 13:16

rishabh-d-dave added needs-qa wip-rishabh-testing Rishabh's testing label labels Apr 5, 2023

github-actions bot added the stale label Jun 4, 2023

rishabh-d-dave removed the stale label Jun 28, 2023

rishabh-d-dave reviewed Jul 13, 2023

View reviewed changes

rishabh-d-dave approved these changes Jul 13, 2023

View reviewed changes

rishabh-d-dave removed the needs-qa label Jul 13, 2023

xdavidwu added 2 commits July 13, 2023 14:15

mds-client: make client_dirsize_rbytes unchangeable in runtime

05a48a4

kclient also has this option, and is unchangeable unless remounted. Let's keep parity with kclient. Signed-off-by: Pinghao Wu <xdavidwuph@gmail.com>

xdavidwu force-pushed the client-readdir-performance branch from b43e31c to a66e969 Compare July 13, 2023 08:56

rishabh-d-dave force-pushed the client-readdir-performance branch from a66e969 to 05a48a4 Compare July 13, 2023 08:56

rishabh-d-dave merged commit 2bdb0ae into ceph:main Jul 13, 2023

This was referenced Sep 10, 2023

reef: client: readdir_r_cb: get rstat for dir only if using rbytes for size #53359

Merged

quincy: client: readdir_r_cb: get rstat for dir only if using rbytes for size #53360

Closed

rishabh-d-dave mentioned this pull request Oct 25, 2023

pacific: client: readdir_r_cb: get rstat for dir only if using rbytes for size #54179

Closed

Conversation

xdavidwu commented Jul 5, 2022 • edited by rishabh-d-dave Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

lxbsz left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave commented Mar 31, 2023

Uh oh!

rishabh-d-dave commented Mar 31, 2023

Uh oh!

xdavidwu commented Apr 1, 2023

Uh oh!

vshankar commented Apr 3, 2023

Uh oh!

xdavidwu commented Apr 3, 2023

Uh oh!

vshankar commented Apr 3, 2023

Uh oh!

github-actions bot commented Jun 4, 2023

Uh oh!

rishabh-d-dave left a comment

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave commented Jul 13, 2023

Uh oh!

rishabh-d-dave commented Jul 13, 2023

Uh oh!

rishabh-d-dave commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xdavidwu commented Jul 5, 2022 •

edited by rishabh-d-dave

Loading

rishabh-d-dave commented Jul 13, 2023 •

edited

Loading