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

log: use one write system call per message #11955

Merged
merged 1 commit into from Jan 16, 2017

Conversation

Projects
None yet
5 participants
@batrick
Member

batrick commented Nov 14, 2016

Two separate safe_write calls would result in having two write syscalls
like so:

25098 write(2, "2016-10-04 11:33:46.357326 7fb025968f00 10 client.4143 did not get mds through better means, so chos"..., 114) = 114
25098 write(2, "\n", 1)                 = 1
25098 write(2, "2016-10-04 11:33:46.357333 7fb025968f00 20 client.4143 mds is 0", 63) = 63
25098 write(2, "\n", 1)                 = 1
25098 write(2, "2016-10-04 11:33:46.357336 7fb025968f00 10 client.4143 send_request rebuilding request 1 for mds.0", 98) = 98
25098 write(2, "\n", 1)                 = 1
25098 write(2, "2016-10-04 11:33:46.357341 7fb025968f00 20 client.4143 encode_cap_releases enter (req: 0x7fb02e7502c"..., 110) = 110
25098 write(2, "\n", 1)                 = 1

Signed-off-by: Patrick Donnelly pdonnell@redhat.com

int r = safe_write(m_fd, s, strlen(s));
if (r >= 0)
r = safe_write(m_fd, "\n", 1);
std::string b {s, '\n'};

This comment has been minimized.

@gregsfortytwo

gregsfortytwo Nov 14, 2016

Member

I'm trying to match this initializer to the std::string reference at http://en.cppreference.com/w/cpp/string/basic_string/basic_string, and the only one I'm seeing I think is interpreting '\n' as an integer for "count", rather than as something to be appended. (Constructor 4)
Am I missing something here?

This comment has been minimized.

@batrick

batrick Nov 14, 2016

Member

Ya, you're right. I've sat on this branch for too long and forgot to double-check that was correct...

@gregsfortytwo

This looks good to me.

if (r >= 0)
r = safe_write(m_fd, "\n", 1);
std::string b(s);
b += '\n';

This comment has been minimized.

@cbodley

cbodley Nov 15, 2016

Contributor

the std::string constructor is likely to allocate only enough memory to fit s, so the += will probably do an extra allocation

you could use std::string::reserve() to fit everything in one heap allocation, but instead of using the heap, maybe consider putting it on the stack with alloca()?

This comment has been minimized.

@batrick

batrick Nov 15, 2016

Member

I went ahead and made the change with std::string::reserve`.

r = safe_write(m_fd, "\n", 1);
size_t len = strlen(s);
std::string b;
b.reserve(len + 2); /* + "\n\0" */

This comment has been minimized.

@tchaikov

tchaikov Nov 15, 2016

Contributor

i don't think we need to take the '\0' into consideration.

This comment has been minimized.

@batrick

batrick Nov 15, 2016

Member

okay, fixed!

log: use one write system call per message
Two separate safe_write calls would result in having two write syscalls
like so:

    25098 write(2, "2016-10-04 11:33:46.357326 7fb025968f00 10 client.4143 did not get mds through better means, so chos"..., 114) = 114
    25098 write(2, "\n", 1)                 = 1
    25098 write(2, "2016-10-04 11:33:46.357333 7fb025968f00 20 client.4143 mds is 0", 63) = 63
    25098 write(2, "\n", 1)                 = 1
    25098 write(2, "2016-10-04 11:33:46.357336 7fb025968f00 10 client.4143 send_request rebuilding request 1 for mds.0", 98) = 98
    25098 write(2, "\n", 1)                 = 1
    25098 write(2, "2016-10-04 11:33:46.357341 7fb025968f00 20 client.4143 encode_cap_releases enter (req: 0x7fb02e7502c"..., 110) = 110
    25098 write(2, "\n", 1)                 = 1

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
int r = safe_write(m_fd, s, strlen(s));
if (r >= 0)
r = safe_write(m_fd, "\n", 1);
size_t len = strlen(s);

This comment has been minimized.

@tchaikov

tchaikov Nov 15, 2016

Contributor

or, is it possible to use VLA here? i.e.

char b[strlen(s) + 1];

?

This comment has been minimized.

@batrick

batrick Nov 15, 2016

Member

Sure but I normally avoid VLA because stack allocation failures cannot be caught.

This comment has been minimized.

@cbodley

cbodley Nov 15, 2016

Contributor

not sure about VLA support in c++11, is it part of the standard? or as a gnu extension?

i suggested using alloca() for stack allocation, which would accomplish the same thing

This comment has been minimized.

@batrick

batrick Nov 15, 2016

Member

A stack allocation (VLA or alloca) would be dangerous here IMO because the message has unknown size. I'm not in favor of that change.

@ghost

This comment has been minimized.

ghost commented Nov 17, 2016

jenkins test this please (eio ignored in master)

@batrick

This comment has been minimized.

Member

batrick commented Jan 10, 2017

@tchaikov, please merge if it looks good to you still.

@tchaikov tchaikov added needs-qa and removed needs-review labels Jan 10, 2017

@tchaikov tchaikov self-assigned this Jan 10, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 10, 2017

sure, will run it thru rados qa suite before merge.

@yuriw yuriw merged commit d3fd1d4 into ceph:master Jan 16, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@batrick batrick deleted the batrick:log-2-to-1-write branch Jan 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment