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

[DNM] jewel: rgw: fix memory fragmentation problem reading data from client. #20698

Closed

Conversation

@mdw-at-linuxbox
Copy link
Contributor

commented Mar 4, 2018

STREAM_IO(s)->read() can, and nearly always returns "short" reads.
(Experimentally, about 1% of a 1m buffer gets filled.) The resulting
buffer append operations result in constructing a list of buffers
of mostly unused data, which bloats up memory usage considerably.
The fix here is to loop and try to fill the buffer more aggressively
to start with.

mg_read() returns 0 upon EOF. The next read to mg_read()
returns -1. The above change that improved buffer filling logic
also makes it possible for it to try to read past the end of
the actual data. So, add a flag to remember that
an actual EOF was seen and another mg_read should not be attempted.

Fixes: https://tracker.ceph.com/issues/23207

Signed-off-by: Marcus Watts mwatts@redhat.com

@mdw-at-linuxbox mdw-at-linuxbox force-pushed the mdw-at-linuxbox:wip-jewel-rgw-fixbuf branch from 9363a4f to 8b0d039 Mar 4, 2018

@mdw-at-linuxbox

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2018

I've pushed an update to this PR. I moved the read-fill loop down into the civetweb code. This has the same effect as the previous code for PUT, but it extends the same fill logic for every other call to STREAM_IO(s)->read. I do not think any callers of this were expecting short reads, and I believe this may explain reports from the field about hard to reproduce signature failures.

@smithfarm smithfarm added this to the jewel milestone Mar 4, 2018

@smithfarm smithfarm added the rgw label Mar 4, 2018

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2018

@mdw-at-linuxbox Could you add to the commit message a short sentence explaining why this fix cannot be implemented as a cherry-pick from master?

@smithfarm smithfarm changed the title rgw: fix memory fragmentation problem reading data from client. jewel: rgw: fix memory fragmentation problem reading data from client. Mar 5, 2018

@smithfarm smithfarm assigned smithfarm and unassigned mattbenjamin and cbodley Mar 5, 2018

@smithfarm smithfarm added the DNM label Mar 5, 2018

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

Adding DNM until it can be clarified whether this issue affects master.

@mattbenjamin

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

@smithfarm agree, this PR reflects Marcus' work direct debugging Jewel, and he is now looking at whether it applies on L or M; if it does, we can re-organize history as needed.

@mattbenjamin
Copy link
Contributor

left a comment

clarity suggestion

@@ -32,7 +32,8 @@ int RGWMongoose::write_data(const char *buf, int len)
RGWMongoose::RGWMongoose(mg_connection *_conn)
: conn(_conn), status_num(0), header_done(false),
sent_header(false), has_content_length(false),
explicit_keepalive(false), explicit_conn_close(false)
explicit_keepalive(false), explicit_conn_close(false),
no_more_data_to_read(false)

This comment has been minimized.

Copy link
@mattbenjamin

mattbenjamin Mar 5, 2018

Contributor

no_? I think it is more natural to pass the non-negated version

@@ -25,6 +25,7 @@ class RGWMongoose : public RGWStreamIO
bool has_content_length;
bool explicit_keepalive;
bool explicit_conn_close;
bool no_more_data_to_read;

This comment has been minimized.

Copy link
@mattbenjamin

mattbenjamin Mar 5, 2018

Contributor

no_? I think it is more natural to save the non-negated version; or to rename this something non-negated (e.g., eod)

@smithfarm smithfarm changed the title jewel: rgw: fix memory fragmentation problem reading data from client. [DNM] jewel: rgw: fix memory fragmentation problem reading data from client. Mar 5, 2018

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

This is a backport of #20724 - marked DNM until that one is merged.

rgw: fix memory fragmentation problem reading data from client.
mg_read returns 0 on EOF.  It also can and often does "short" reads.
The logic in the rest of ceph depends on a read always filling its buffer
when possible.  So loop here and fill the buffer.  Looping to fill the
buffer means we must also track when mg_read returns EOF, because
after that further calls to mg_read will return -1.

Fixes: https://tracker.ceph.com/issues/23207

Signed-off-by: Marcus Watts <mwatts@redhat.com>
(cherry picked from commit d9a150b)

@mdw-at-linuxbox mdw-at-linuxbox force-pushed the mdw-at-linuxbox:wip-jewel-rgw-fixbuf branch from 8b0d039 to dc3e8c1 Mar 10, 2018

@mdw-at-linuxbox

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

I pushed a new version of this. Changed var name to match what I used in master, added cherry pick line.

@smithfarm
Copy link
Contributor

left a comment

The commit message lacks a "Conflicts" section, giving rise to a false notion that the commit cherry-picked to jewel cleanly.

Please add that section. Just append to the commit message:

Conflicts:
    src/rgw/rgw_civetweb.cc

(Ideally, after the filename there would follow a description of the conflicts and how they were resolved.)

@prallabh

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

Can this be targeted for Jewel 10.2.11, thanks!

@mattbenjamin

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

@prallabh that should happen, @theanalyst is there any scheduling obstacle to this that you know of?

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

@prallabh The cherry-pick lacks a conflicts section (see my review). It cannot be accepted into jewel 10.2.11 as it is.

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

@mattbenjamin Someone can re-do the PR, if necessary, to get the conflicts section in.

EDIT: that someone would be me

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2018

closing in favor of #21098

@smithfarm smithfarm closed this Mar 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.