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.

Sign up for GitHub

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

Jump to bottom

buffer: use move construct to append/push_back/push_front #7455

Merged
merged 4 commits into from Feb 4, 2016
Merged

buffer: use move construct to append/push_back/push_front #7455

merged 4 commits into from Feb 4, 2016

Conversation

yuyuyu101
Copy link
Member

No description provided.

@yuyuyu101 yuyuyu101 force-pushed the bufferlist-move branch 3 times, most recently from b73a429 to dc15bb5 Compare February 1, 2016 14:53
cbodley
cbodley reviewed Feb 1, 2016
View reviewed changes
src/common/buffer.cc
void buffer::list::append(ptr&& bp)
{
if (bp.length())
push_back(bp);
Copy link
Contributor

Choose a reason for hiding this comment

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

bp is an lvalue in the body of the function, so the call to push_back() needs to cast it again: push_back(std::move(bp));

Copy link
Member Author

Choose a reason for hiding this comment

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

OMG, I correct my thought now....

@yuyuyu101
Copy link
Member Author

done

cbodley
cbodley reviewed Feb 1, 2016
View reviewed changes
src/include/buffer.h
void push_front(ptr&& bp) {
if (bp.length() == 0)
return;
_buffers.push_front(std::move(bp));
Copy link
Contributor

Choose a reason for hiding this comment

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

need to call bp.length() before push_front() here. it looks like you applied this fix to void push_front(ptr& bp) { instead

@yuyuyu101
Copy link
Member Author

@cbodley thanks for quick comments, fixed

@cbodley
Copy link
Contributor

cbodley commented Feb 1, 2016

@yuyuyu101 looks good to me 👍

tchaikov
tchaikov reviewed Feb 1, 2016
View reviewed changes
src/osdc/ObjectCacher.cc
<< dendl;
bl.push_back(bp);
bl.append_zero();
Copy link
Contributor

Choose a reason for hiding this comment

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

should be

bl.append_zero(length - bl.length());

@tchaikov
Copy link
Contributor

tchaikov commented Feb 1, 2016

apart from #7455 (comment), lgtm

might need to extract the change to bufferlist into another commit.

@yuyuyu101
Copy link
Member Author

@tchaikov thanks, done.

@tchaikov
Copy link
Contributor

tchaikov commented Feb 1, 2016

lgtm with qa run.

@liewegas
Copy link
Member

liewegas commented Feb 2, 2016

In file included from os/bluestore/KernelDevice.h:18:0,
from os/bluestore/KernelDevice.cc:22:
./os/fs/FS.h: In member function ‘void FS::aio_t::pread(uint64_t, uint64_t)’:
./os/fs/FS.h:79:32: error: ‘p’ was not declared in this scope
io_prep_pread(&iocb, fd, p.c_str(), length, offset);
^

@yuyuyu101
buffer: add ptr move construct
dd4e6e1 
Signed-off-by: Haomai Wang <haomai@xsky.com>
@yuyuyu101 yuyuyu101 force-pushed the bufferlist-move branch 3 times, most recently from 7ebcb1d to 4c17b1d Compare February 2, 2016 17:51
@yuyuyu101
Copy link
Member Author

@cbodley @tchaikov after tested locally, I found operator= is missing for rvalue, so I added this commit!

plz review again!

@yuyuyu101
misc: using move construct to avoid extra atomic inc/dec
0305cee 
Signed-off-by: Haomai Wang <haomai@xsky.com>
@yuyuyu101
buffer: add operator= for ptr/bufferlist rvalue
669f143 
Signed-off-by: Haomai Wang <haomai@xsky.com>
@cbodley
Copy link
Contributor

cbodley commented Feb 2, 2016

@yuyuyu101 the operator= looks correct to me. i see that we have pretty good test coverage for the bufferlist constructors and operators in src/test/bufferlist.cc; could you add some test cases for the new operations?

@yuyuyu101 yuyuyu101 force-pushed the bufferlist-move branch 2 times, most recently from 86b5e0f to f969207 Compare February 3, 2016 07:58
@yuyuyu101
test_bufferlist: add move tests for bufferlist
41c3dc2 
Signed-off-by: Haomai Wang <haomai@xsky.com>
@yuyuyu101
Copy link
Member Author

@cbodley @liewegas updated

@cbodley
Copy link
Contributor

cbodley commented Feb 3, 2016

@yuyuyu101 looks good, thanks!

liewegas added a commit that referenced this pull request Feb 4, 2016
@liewegas
Merge pull request #7455 from yuyuyu101/bufferlist-move
fb1abc3 
buffer: use move construct to append/push_back/push_front

Reviewed-by: Sage Weil <sage@redhat.com>
@liewegas liewegas merged commit fb1abc3 into ceph:master Feb 4, 2016
@yuyuyu101 yuyuyu101 deleted the bufferlist-move branch February 4, 2016 13:38
@dillaman
Copy link

dillaman commented Feb 9, 2016

@yuyuyu101 Running git bisect, it looks like ceph_test_librbd_fsx fails after its first clone validation due to commit dd4e6e1:

truncating image image_client.0-clone1 from 0xea8df03 (overlap 0x1a7ae16) to 0x54bc6a
checking clone #0, image image_client.0 against file /home/jdillaman/archive/fsx-image_client.0-parent1
READ BAD DATA: offset = 0x0, size = 0x8abe78a, fname = image_client.0

The comparison fails on a section where there is no backing RBD object but the read operation returned a non-zeroed buffer.

Here is a simple reproducer: ceph_test_librbd_fsx -d -W -R -p 100 -P ~/archive -r 1 -w 1 -t 1 -h 1 -l 250000000 -S 4621 -N 1000 pool_client.0 image_client.0 -U

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
core needs-qa performance wip-sage2-testing
Projects
None yet
Milestone
No milestone
5 participants
@yuyuyu101 @cbodley @tchaikov @liewegas @dillaman