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

os/bluestore/BlueFS: Rebuild memcopy for bufferlist::page_aligned_app… #15728

Merged
merged 3 commits into from Jun 20, 2017

Conversation

Projects
None yet
3 participants
@majianpeng
Member

majianpeng commented Jun 16, 2017

…ender

BlueFS use bufferlist::page_aligned_appender to make data aligned
w/ memory. But bufferlist::rebuild_aligned_size_and_memory don't handle
this case. For example:

buffer::list(len=4096,
  buffer::ptr(90112~2018 0x55f4de51e000 in raw 0x55f4de508000 len 1048576 nref 7),
  buffer::ptr(92130~2078 0x55f4de51e7e2 in raw 0x55f4de508000 len 1048576 nref 7)
)

After bufferlist::rebuild_aligned_size_and_memory()

buffer::list(
len=4096, buffer::ptr(0~4096 0x55f4de6de000 in raw 0x55f4de6de000 len 4096 nref 1)
)

Using bufferlist::append(ptr, offset, len) function to make all continue
prt merge.

Signed-off-by: Jianpeng Ma jianpeng.ma@intel.com

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jun 17, 2017

Member

retest this please

Member

xiexingguo commented Jun 17, 2017

retest this please

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jun 20, 2017

Member

I think the bug here is that page_aligned_appender isn't working as designed. It is expecting an append that is part of the tail buffer to simply adjust that tail buffer.

Member

liewegas commented Jun 20, 2017

I think the bug here is that page_aligned_appender isn't working as designed. It is expecting an append that is part of the tail buffer to simply adjust that tail buffer.

@majianpeng

This comment has been minimized.

Show comment
Hide comment
@majianpeng

majianpeng Jun 20, 2017

Member

I don't think so. The function append there is no problem. For example:
append(0, 3K)--> tail(0,3k)
append(0,1k)---> tail + append
tail && append are continue. But rebuild_aligned_size_and_memory can't handle this case.

Member

majianpeng commented Jun 20, 2017

I don't think so. The function append there is no problem. For example:
append(0, 3K)--> tail(0,3k)
append(0,1k)---> tail + append
tail && append are continue. But rebuild_aligned_size_and_memory can't handle this case.

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jun 20, 2017

Member

#15767 tests out okay..

Member

liewegas commented Jun 20, 2017

#15767 tests out okay..

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jun 20, 2017

Member

I'm not following where rebuild_aligned_size_and_memory enters the picture. Where did you see that fragmented buffer above?

Member

liewegas commented Jun 20, 2017

I'm not following where rebuild_aligned_size_and_memory enters the picture. Where did you see that fragmented buffer above?

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jun 20, 2017

Member

Oh.. is it because the _flush() method is called, transfering the tail ptr to the other bufferlist, and the contiguous appender continues adding bits to the writer buffer?

Member

liewegas commented Jun 20, 2017

Oh.. is it because the _flush() method is called, transfering the tail ptr to the other bufferlist, and the contiguous appender continues adding bits to the writer buffer?

@majianpeng

This comment has been minimized.

Show comment
Hide comment
@majianpeng

majianpeng Jun 20, 2017

Member

Yes.

Member

majianpeng commented Jun 20, 2017

Yes.

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jun 20, 2017

Member

Ok I see now! Can you introduce a new method, claim_append_piecewise() (or similar) that does this instead of duplicating the loop multiple times?

Member

liewegas commented Jun 20, 2017

Ok I see now! Can you introduce a new method, claim_append_piecewise() (or similar) that does this instead of duplicating the loop multiple times?

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jun 20, 2017

Member

Good catch!

Member

liewegas commented Jun 20, 2017

Good catch!

@majianpeng

This comment has been minimized.

Show comment
Hide comment
@majianpeng

majianpeng Jun 20, 2017

Member

Ok. I'll do it.

Member

majianpeng commented Jun 20, 2017

Ok. I'll do it.

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jun 20, 2017

Member

Can you also add a test (similar to the one in #15767... maybe cherry-pick that commit into this pr too) showing that it merges them?

Member

liewegas commented Jun 20, 2017

Can you also add a test (similar to the one in #15767... maybe cherry-pick that commit into this pr too) showing that it merges them?

@majianpeng

This comment has been minimized.

Show comment
Hide comment
@majianpeng
Member

majianpeng commented Jun 20, 2017

OK.

@majianpeng

This comment has been minimized.

Show comment
Hide comment
@majianpeng

majianpeng Jun 20, 2017

Member

update by your comments
a)add new function claim_append_piecewise; and related test case
b)cherry-pick your #15767

Member

majianpeng commented Jun 20, 2017

update by your comments
a)add new function claim_append_piecewise; and related test case
b)cherry-pick your #15767

@liewegas liewegas merged commit 503de20 into ceph:master Jun 20, 2017

3 of 4 checks passed

arm64 make check arm64 make check failed
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details

majianpeng and others added some commits Jun 20, 2017

common/buffer: add function bufferlist::claim_append_piecewise(list& …
…bl).

This only useful for bl is bufferlist::page_aligned_appender. Using
this function can remove memcopy for continue ptrs.
Because page_aligned_appender::flush will split a ptr into two or
more ptrs. For this case, rebuild_aligned_size_and_memory can't handle,
it  will rebuild.

For example
a=bl.get_page_aligned_appender(1);
a.append(3K)
a.flush();
t.claim_append(bl);
a.append(1K);
a.flush();
t.claim_append(bl);

dst.claim_append(t);
//3K and 1K ptr are continue in memory. But they are two ptrs..
dst.is_aligned_size_and_memory(4096,4096) is false.

We add new function claim_append_piecewise() to specially
handle this case.

Signed-off-by: Jianpeng Ma <jianpeng.ma@intel.com>
os/bluestore/BlueFS: replace claim_append with claim_append_piecewise…
… to reduce memory copy.

Signed-off-by: Jianpeng Ma <jianpeng.ma@intel.com>
unittest_bufferlist: test page_aligned_appender
Signed-off-by: Sage Weil <sage@redhat.com>

@majianpeng majianpeng deleted the majianpeng:bluefs-remove-memcopy-for-continue-memory branch Jun 21, 2017

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