Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rgw: disable prefetch in rgw_file to fix 3x read amplification #38167

Merged

Conversation

jettjaniak
Copy link
Contributor

@jettjaniak jettjaniak commented Nov 18, 2020

rgw: This PR disables prefetch in rgw_file (used by NFS-Ganesha).

Each call to rgw_read (rgw_file.cc) invokes three calls to RGWRados::get_obj_state with RGWObjState::prefetch_data = true. It results in great read amplification. If length argument in rgw_read call is smaller than rgw_max_chunk_size, then the amplification is threefold.

Tracker issue: https://tracker.ceph.com/issues/48289

Signed-off-by: Kajetan Janiak kjaniak@cloudferro.com

Each call to rgw_read (rgw_file.cc) invokes three calls to RGWRados::get_obj_state with s->prefetch_data=true. It results in great read amplification. If length argument in rgw_read call is smaller than rgw_max_chunk_size, then the amplification is threefold.

Signed-off-by: Kajetan Janiak <kjaniak@cloudferro.com>
@mattbenjamin
Copy link
Contributor

mattbenjamin commented Nov 18, 2020

@kajetanJ thanks for spotting this; at the moment, I think it probably is correct to avoid prefetching in general, but we might be able to address this in future and also take advantage of range prefetch (@ofriedma) and some additional infrastructure to get benefit from prefetch in future? It's definitely not intended that there be multiple prefetch attempts, so we'd have to fix that, also

@mattbenjamin
Copy link
Contributor

@kajetanJ can you create a tracker issue and attach it to his PR?

Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm (but please update with tracker issue)

@jettjaniak
Copy link
Contributor Author

jettjaniak commented Nov 18, 2020

@mattbenjamin my account on tracker is pending administrator approval. Would you like me to include all reproduction steps or just describe the issue?

@mattbenjamin
Copy link
Contributor

@kajetanJ either is fine; what's your account name in tracker? I can try to expedite it

@jettjaniak
Copy link
Contributor Author

@mattbenjamin it is kjaniak. Thanks.

Copy link
Contributor

@ofriedma ofriedma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be with less changes just to change line 1975 from true to false

RGWGetObj::get_data = false; // XXX

@mattbenjamin
Copy link
Contributor

I think it would be with less changes just to change line 1975 from true to false

RGWGetObj::get_data = false; // XXX

I don't agree with this advice. @kajetanJ 's proposal is fine, and will inline just fine.

Matt

@jettjaniak
Copy link
Contributor Author

@mattbenjamin Tracker issue added (link in the first comment).
@ofriedma

I think it would be with less changes just to change line 1975 from true to false

RGWGetObj::get_data = false; // XXX

it would cause problems in RGWGetObj::execute, rgw_op.cc:2220

  if (!get_data || ofs > end) {
    send_response_data(bl, 0, 0);
    return;
  }

@ofriedma ofriedma self-requested a review November 19, 2020 13:09
@jettjaniak
Copy link
Contributor Author

Could we backport this to Nautilus?

@dang
Copy link
Contributor

dang commented Nov 19, 2020

Just mark the tracker with the backports you want.

@jettjaniak
Copy link
Contributor Author

@dang it seems I can't edit an existing issue, so I added a note

@mattbenjamin
Copy link
Contributor

done: we want octopus, nautilus, I believe

@mattbenjamin
Copy link
Contributor

@dang tested by hand in a Nautilus backport, passed librgw_file_nfsns, the main unit file.

@mattbenjamin
Copy link
Contributor

I've proved non-regression in unit tests run by hand, maybe worth confirming perf result

@mattbenjamin mattbenjamin merged commit d81be06 into ceph:master Dec 3, 2020
@xelexin xelexin deleted the wip-disable-prefetch-in-rgw-file branch January 15, 2021 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants