librbd: send FLUSH_SOURCE_INTERNAL when do copy/deep_copy. #43659

majianpeng · 2021-10-26T05:34:10Z

copy/deep_copy use object_map to judge whether object exist.
If w/ librbdo pwl cache, flush can't flush data to osd which change
objectmap state. So we should send flush w/ FLUSH_SOURCE_INTERNAL to
make data flush to osd.

Fixes:https://tracker.ceph.com/issues/53057
Signed-off-by: Jianpeng Ma jianpeng.ma@intel.com

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

majianpeng · 2021-10-26T05:41:33Z

retest this please

majianpeng · 2021-10-26T06:00:11Z

jenkins test make check

majianpeng · 2021-10-26T11:28:42Z

retest this please

majianpeng · 2021-10-26T23:58:11Z

retest this please

tchaikov · 2021-10-27T01:10:53Z

jenkins test make check

majianpeng · 2021-10-28T00:00:54Z

jenkins test make check

majianpeng · 2021-10-28T05:14:33Z

jenkins test make check

trociny

In general I think it is correct. I have some minor questions about the fix implementation details though.

As it is "persistent cache" specific, wouldn't it better to do this flush only for this case? I don't think it matters much on practice but at least it would be less confusing for an occasional code reader why we have this.
In the deep copy case, wouldn't it better to add this flush to DeepCopyRequest instead?
Wouldn't be more correct not only flush but entirely disable the pwl cache when copying? (Ideally, switch to "write through" mode). Again, I don't think it would make much difference on practice. Just a note to consider.

src/librbd/api/Image.cc

majianpeng · 2021-10-29T03:47:04Z

For "As it is "persistent cache" specific, wouldn't it better to do this flush only for this case? I don't think it matters much on practice but at least it would be less confusing for an occasional code reader why we have this."
i copy flush login from: https://github.com/ceph/ceph/blob/master/src/librbd/api/DiffIterate.cc#L167-L181.

In general I think it is correct. I have some minor questions about the fix implementation details though.
1. As it is "persistent cache" specific, wouldn't it better to do this flush only for this case? I don't think it matters much on practice but at least it would be less confusing for an occasional code reader why we have this.

In fact, flush login copy from https://github.com/ceph/ceph/blob/master/src/librbd/api/DiffIterate.cc#L167-L181. I'm not family with librbd other part.

2. In the deep copy case, wouldn't it better to add this flush to `DeepCopyRequest` instead?

I'll try.

3. Wouldn't be more correct not only flush but entirely disable the pwl cache when copying? (Ideally, switch to "write through" mode). Again, I don't think it would make much difference on practice. Just a note to consider.

Yes, but this need user to know much. In fact cache is inner part. For example, write one image w/ pwl. Host restart and data can't flush to osd. After restart, user do copy. By your method, it should firstly flush data to osd then using write_through or disable pwl cache.
Also, currently, pwl no write-through mode.

majianpeng · 2021-10-29T06:47:51Z

@trociny for " In the deep copy case, wouldn't it better to add this flush to DeepCopyRequest instead?"
when move code into DeepCopyRequest.cc: compiler met the following errors.
librbd::io::AioCompletion::create_and_star func don't support MockTestImageCtx.

FAILED: src/test/librbd/CMakeFiles/unittest_librbd.dir/test_mock_DeepCopyRequest.cc.o 
/usr/bin/clang++ -DBOOST_ASIO_DISABLE_THREAD_KEYWORD_EXTENSION -DBOOST_ASIO_USE_TS_EXECUTOR_AS_DEFAULT -DHAVE_CONFIG_H -DTEST_LIBRBD_INTERNALS -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D_REENTRANT -D_THREAD_SAFE -D__CEPH__ -D__STDC_FORMAT_MACROS -D__linux__ -Isrc/include -I../src -isystem boost/include -isystem include -isystem ../src/xxHash -isystem ../src/rapidjson/include -isystem ../src/googletest/googlemock/include -isystem ../src/googletest/googlemock -isystem ../src/googletest/googletest/include -isystem ../src/googletest/googletest -isystem src/pmdk/src/include -O2 -g -DNDEBUG -fPIE -Wall -fno-strict-aliasing -fsigned-char -Wtype-limits -Wignored-qualifiers -Wpointer-arith -Werror=format-security -Winit-self -Wno-unknown-pragmas -Wnon-virtual-dtor -Wno-ignored-qualifiers -ftemplate-depth-1024 -Wpessimizing-move -Wredundant-move -Wno-inconsistent-missing-override -Wno-mismatched-tags -Wno-unused-private-field -Wno-address-of-packed-member -Wno-unused-function -Wno-unused-local-typedef -Wno-varargs -Wno-gnu-designator -Wno-missing-braces -Wno-parentheses -Wno-deprecated-register -fdiagnostics-color=auto -fno-builtin-malloc -fno-builtin-calloc -fno-builtin-realloc -fno-builtin-free -std=c++17 -MD -MT src/test/librbd/CMakeFiles/unittest_librbd.dir/test_mock_DeepCopyRequest.cc.o -MF src/test/librbd/CMakeFiles/unittest_librbd.dir/test_mock_DeepCopyRequest.cc.o.d -o src/test/librbd/CMakeFiles/unittest_librbd.dir/test_mock_DeepCopyRequest.cc.o -c ../src/test/librbd/test_mock_DeepCopyRequest.cc
In file included from ../src/test/librbd/test_mock_DeepCopyRequest.cc:134:
../src/librbd/DeepCopyRequest.cc:80:21: error: no matching function for call to 'create_and_start'
    auto aio_comp = librbd::io::AioCompletion::create_and_start(&flush_ctx, m_src_image_ctx,
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/test/librbd/test_mock_DeepCopyRequest.cc:278:12: note: in instantiation of member function 'librbd::DeepCopyRequest<librbd::(anonymous namespace)::MockTestImageCtx>::send' requested here
  request->send();
           ^
../src/librbd/io/AioCompletion.h:102:25: note: candidate function template not viable: no known conversion from 'librbd::(anonymous namespace)::MockTestImageCtx *' to 'librbd::ImageCtx *' for 2nd argument
  static AioCompletion *create_and_start(T *obj, ImageCtx *image_ctx,

trociny · 2021-10-29T06:59:12Z

when move code into DeepCopyRequest.cc: compiler met the following errors. librbd::io::AioCompletion::create_and_star func don't support MockTestImageCtx.

You need to wrap m_src_image_ctx into get_image_ctx(m_src_image_ctx). See e.g. deep_copy/ObjectCopyRequest.cc for an example.

trociny · 2021-10-29T07:07:35Z

Ok. Taking we already have a precedence of doing this in api/DiffIterate.cc, I think I am fine with the current implementation. I.e. there is no need to move the code into DeepCopyRequest.cc. Though please add a comment describing the reason, similarly to what we have in api/DiffIterate.cc.

majianpeng · 2021-10-29T08:13:02Z

jenkins test api

ideepika · 2021-10-31T06:19:58Z

jenkins test api

copy/deep_copy use object_map to judge whether object exist. If w/ librbdo pwl cache, flush can't flush data to osd which change objectmap state. So we should send flush w/ FLUSH_SOURCE_INTERNAL to make data flush to osd. Fixes:https://tracker.ceph.com/issues/53057 Signed-off-by: Jianpeng Ma <jianpeng.ma@intel.com>

majianpeng · 2021-11-01T02:55:10Z

jenkins test api

majianpeng · 2021-11-01T05:04:55Z

@ideepika update.

ideepika · 2021-11-01T05:08:06Z

@ideepika update.

@majianpeng is this related to #43127 (comment) as well?

majianpeng · 2021-11-01T05:10:35Z

@ideepika update.

@majianpeng is this related to #43127 (comment) as well?
No. they are different issues.

ideepika

@majianpeng can you amend commit title to PR title, lgtm ! thanks!

sunnyku

LGTM

ideepika · 2021-11-02T05:17:40Z

@majianpeng can you amend commit title to PR title, lgtm ! thanks!

@majianpeng intial tests looks good: https://pulpito.ceph.com/ideepika-2021-11-01_18:01:37-rbd-wip-deepika2-testing-2021-11-01-1948-distro-basic-smithi/
can you amend the commit to match the title?

majianpeng · 2021-11-02T05:21:58Z

@majianpeng can you amend commit title to PR title, lgtm ! thanks!

@majianpeng intial tests looks good: https://pulpito.ceph.com/ideepika-2021-11-01_18:01:37-rbd-wip-deepika2-testing-2021-11-01-1948-distro-basic-smithi/ can you amend the commit to match the title?

Sorry omit your comments. update, please review.

ideepika · 2021-11-03T12:53:14Z

@majianpeng mind taking a look into https://tracker.ceph.com/issues/53108 seems not related but still new failure seen recently

ideepika · 2021-11-03T14:52:14Z

https://pulpito.ceph.com/ideepika-2021-11-03_11:46:43-rbd-wip-ssd-cache-testing-distro-basic-smithi
https://pulpito.ceph.com/ideepika-2021-11-02_12:33:30-rbd-wip-ssd-cache-testing-distro-basic-smithi

cache failures:
https://tracker.ceph.com/issues/53110#change-205407
https://tracker.ceph.com/issues/49876

failures unrelated and tracked by:
https://tracker.ceph.com/issues/53108#change-205427
https://tracker.ceph.com/issues/51282#change-205397
https://tracker.ceph.com/issues/52503#change-205400
https://tracker.ceph.com/issues/53108#change-205427

majianpeng · 2021-11-04T01:01:09Z

@majianpeng mind taking a look into https://tracker.ceph.com/issues/53108 seems not related but still new failure seen recently

@ideepika remembered a question about objectmap. When write after discard, objectmap maybe omit discard command:
https://github.com/ceph/ceph/blob/master/src/librbd/io/ObjectRequest.cc#L426.
For example: write(0,64K) and later discard(0,32k). Because currently write still can't create object, so discard will be omitted.

github-actions bot added the rbd label Oct 26, 2021

majianpeng changed the title ~~librbd: send LUSH_SOURCE_INTERNAL when do copy/deep_copy.~~ librbd: send FLUSH_SOURCE_INTERNAL when do copy/deep_copy. Oct 26, 2021

majianpeng force-pushed the send-internal-flush-for-rbd-copy branch from c61dbc1 to 8331cc5 Compare October 27, 2021 03:29

ideepika requested review from trociny, sunnyku and ideepika October 27, 2021 14:32

trociny approved these changes Oct 28, 2021

View reviewed changes

src/librbd/api/Image.cc Outdated Show resolved Hide resolved

majianpeng force-pushed the send-internal-flush-for-rbd-copy branch 2 times, most recently from 1acb2b5 to 2c38f39 Compare October 29, 2021 05:35

majianpeng force-pushed the send-internal-flush-for-rbd-copy branch from 2c38f39 to a2ae83f Compare November 1, 2021 00:43

ideepika added wip-deepika-testing and removed wip-deepika-testing labels Nov 1, 2021

ideepika added the wip-deepika2-testing label Nov 1, 2021

ideepika changed the title ~~librbd: send FLUSH_SOURCE_INTERNAL when do copy/deep_copy.~~ librbd: flush to OSD using FLUSH_SOURCE_INTERNAL while doing copy/deep_copy Nov 1, 2021

ideepika approved these changes Nov 1, 2021

View reviewed changes

sunnyku approved these changes Nov 1, 2021

View reviewed changes

ideepika added wip-deepika-testing and removed wip-deepika2-testing labels Nov 2, 2021

majianpeng changed the title ~~librbd: flush to OSD using FLUSH_SOURCE_INTERNAL while doing copy/deep_copy~~ librbd: send FLUSH_SOURCE_INTERNAL when do copy/deep_copy. Nov 2, 2021

ideepika merged commit 41d3c83 into ceph:master Nov 3, 2021

ideepika mentioned this pull request Dec 9, 2021

pacific: librbd/cache/pwl: misc backports #44199

Merged

14 tasks

ideepika added the bug-fix label Dec 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

librbd: send FLUSH_SOURCE_INTERNAL when do copy/deep_copy. #43659

librbd: send FLUSH_SOURCE_INTERNAL when do copy/deep_copy. #43659

majianpeng commented Oct 26, 2021 •

edited by ideepika

majianpeng commented Oct 26, 2021

majianpeng commented Oct 26, 2021

majianpeng commented Oct 26, 2021

majianpeng commented Oct 26, 2021

tchaikov commented Oct 27, 2021

majianpeng commented Oct 28, 2021

majianpeng commented Oct 28, 2021

trociny left a comment

majianpeng commented Oct 29, 2021 •

edited

majianpeng commented Oct 29, 2021

trociny commented Oct 29, 2021

trociny commented Oct 29, 2021

majianpeng commented Oct 29, 2021

ideepika commented Oct 31, 2021

majianpeng commented Nov 1, 2021

majianpeng commented Nov 1, 2021

ideepika commented Nov 1, 2021

majianpeng commented Nov 1, 2021

ideepika left a comment

sunnyku left a comment

ideepika commented Nov 2, 2021

majianpeng commented Nov 2, 2021

ideepika commented Nov 3, 2021

ideepika commented Nov 3, 2021

majianpeng commented Nov 4, 2021 •

edited

librbd: send FLUSH_SOURCE_INTERNAL when do copy/deep_copy. #43659

librbd: send FLUSH_SOURCE_INTERNAL when do copy/deep_copy. #43659

Conversation

majianpeng commented Oct 26, 2021 • edited by ideepika

Checklist

majianpeng commented Oct 26, 2021

majianpeng commented Oct 26, 2021

majianpeng commented Oct 26, 2021

majianpeng commented Oct 26, 2021

tchaikov commented Oct 27, 2021

majianpeng commented Oct 28, 2021

majianpeng commented Oct 28, 2021

trociny left a comment

Choose a reason for hiding this comment

majianpeng commented Oct 29, 2021 • edited

majianpeng commented Oct 29, 2021

trociny commented Oct 29, 2021

trociny commented Oct 29, 2021

majianpeng commented Oct 29, 2021

ideepika commented Oct 31, 2021

majianpeng commented Nov 1, 2021

majianpeng commented Nov 1, 2021

ideepika commented Nov 1, 2021

majianpeng commented Nov 1, 2021

ideepika left a comment

Choose a reason for hiding this comment

sunnyku left a comment

Choose a reason for hiding this comment

ideepika commented Nov 2, 2021

majianpeng commented Nov 2, 2021

ideepika commented Nov 3, 2021

ideepika commented Nov 3, 2021

majianpeng commented Nov 4, 2021 • edited

majianpeng commented Oct 26, 2021 •

edited by ideepika

majianpeng commented Oct 29, 2021 •

edited

majianpeng commented Nov 4, 2021 •

edited