librbd/cache/pwl/ssd: solve competition between read and retire #42580

hualongfeng · 2021-08-02T02:31:46Z

SSD read is not like rwl's. SSD need aio read. Therefore,
we cannot guarantee that the data will not be retire
during the period from sending the read request to the SSD
and receiving the data to the memory, which may cause
the corresponding data on the SSD to be overwritten.

Signed-off-by: Feng Hualong hualong.feng@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 api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

hualongfeng · 2021-08-02T02:33:05Z

@MahatiC @idryomov @majianpeng please review it.

MahatiC

Shouldn't we check the reader count before releasing data from RAM?[0]

[0]https://github.com/ceph/ceph/blob/master/src/librbd/cache/pwl/AbstractWriteLog.cc#L1293

MahatiC · 2021-08-03T04:27:43Z

src/librbd/cache/pwl/ssd/WriteLog.h

@@ -136,6 +136,8 @@ class WriteLog : public AbstractWriteLog<ImageCtxT> {
                           Context *ctx);
  void aio_read_data_blocks(std::vector<WriteLogCacheEntry*> &log_entries,


Is this function still needed? I don't think so. The only other place it's used is[0][1]. In both places it WriteLogCacheEntry can be changed.

[0]https://github.com/ceph/ceph/blob/master/src/librbd/cache/pwl/ssd/WriteLog.cc#L960
[1]https://github.com/ceph/ceph/blob/master/src/librbd/cache/pwl/ssd/WriteLog.cc#L513

@MahatiC yes, the function should be removed.

@MahatiC this function shouldn't be removed, aio_read_data_block() keeps raw pointer.

https://github.com/ceph/ceph/blob/master/src/librbd/cache/pwl/ssd/WriteLog.cc#L960 this line don't changed.

Does it have to be a raw pointer? Having two different rather long functions do exactly the same thing just on different types of pointers is really undesirable.

Is there a way to refactor this?

hualongfeng · 2021-08-03T06:36:56Z

Shouldn't we check the reader count before releasing data from RAM?[0]

[0]https://github.com/ceph/ceph/blob/master/src/librbd/cache/pwl/AbstractWriteLog.cc#L1293

hi, @MahatiC About releasing data from RAM before check the reader count, if we don't release it, when will it be release next time? And I don't think this impact the correctness, it might impact the performance. It only release data in RAM, but we can read data from SSD.

MahatiC · 2021-08-03T10:47:09Z

hi, @MahatiC About releasing data from RAM before check the reader count, if we don't release it, when will it be release next time? And I don't think this impact the correctness, it might impact the performance. It only release data in RAM, but we can read data from SSD.

you're right. I don't think this needs to be changed.

src/librbd/cache/pwl/AbstractWriteLog.cc

src/librbd/cache/pwl/ssd/WriteLog.cc

src/librbd/cache/pwl/LogEntry.h

hualongfeng · 2021-08-05T03:59:07Z

jenkins test make check

src/librbd/cache/pwl/ssd/WriteLog.cc

src/librbd/cache/pwl/rwl/LogEntry.cc

idryomov

Looks fine to me, just a couple of nits.

src/librbd/cache/pwl/ssd/WriteLog.cc

idryomov · 2021-08-05T13:01:20Z

@MahatiC Are you OK with this PR?

MahatiC · 2021-08-06T03:17:07Z

@hualongfeng I assume ssd cache unit tests are run with this PR? And also if there's a specific crash for this, that is also verified by applying this PR?

@idryomov pending above check, looks good to me.

hualongfeng · 2021-08-06T06:30:50Z

@hualongfeng I assume ssd cache unit tests are run with this PR? And also if there's a specific crash for this, that is also verified by applying this PR?

@MahatiC I don't understand what you mean? Do I need to write a new unit test to test the PR? Or I need to run the existed unit tests to test the PR?

MahatiC · 2021-08-06T06:42:49Z

@hualongfeng I assume ssd cache unit tests are run with this PR? And also if there's a specific crash for this, that is also verified by applying this PR?

@MahatiC I don't understand what you mean? Do I need to write a new unit test to test the PR? Or I need to run the existed unit tests to test the PR?

run the existing ones

hualongfeng · 2021-08-06T10:16:32Z

run the existing ones

@MahatiC run this?

root@ceph-client1:/mnt/ceph/build# ./bin/unittest_librbd --gtest_filter="TestMockCacheSSDWriteLog.*"
seed 2238984
Note: Google Test filter = TestMockCacheSSDWriteLog.*
[==========] Running 16 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 16 tests from TestMockCacheSSDWriteLog
[ RUN      ] TestMockCacheSSDWriteLog.init_state_write
Failed to load class: cas (lib/libcls_cas.so): lib/libcls_cas.so: undefined symbol: _Z26cls_get_manifest_ref_countPvNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
Failed to load class: cmpomap (lib/libcls_cmpomap.so): lib/libcls_cmpomap.so: undefined symbol: _Z28cls_cxx_map_get_vals_by_keysPvRKSt3setINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESt4lessIS6_ESaIS6_EEPSt3mapIS6_N4ceph6buffer7v15_2_04listES8_SaISt4pairIKS6_SH_EEE
Failed to load class: fifo (lib/libcls_fifo.so): lib/libcls_fifo.so: undefined symbol: _Z20cls_gen_random_bytesPci
Failed to load class: log (lib/libcls_log.so): lib/libcls_log.so: undefined symbol: _Z24cls_cxx_map_write_headerPvPN4ceph6buffer7v15_2_04listE
Failed to load class: lua (lib/libcls_lua.so): lib/libcls_lua.so: undefined symbol: _Z19cls_current_versionPv
Failed to load class: rgw (lib/libcls_rgw.so): lib/libcls_rgw.so: undefined symbol: _Z19cls_current_versionPv
Failed to load class: test_remote_reads (lib/libcls_test_remote_reads.so): lib/libcls_test_remote_reads.so: undefined symbol: _Z14cls_cxx_gatherPvRKSt3setINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESt4lessIS6_ESaIS6_EERKS6_PKcSG_RN4ceph6buffer7v15_2_04listE
Failed to load class: user (lib/libcls_user.so): lib/libcls_user.so: undefined symbol: _Z24cls_cxx_map_write_headerPvPN4ceph6buffer7v15_2_04listE
[       OK ] TestMockCacheSSDWriteLog.init_state_write (34 ms)
[ RUN      ] TestMockCacheSSDWriteLog.init_state_json_write
[       OK ] TestMockCacheSSDWriteLog.init_state_json_write (23 ms)
[ RUN      ] TestMockCacheSSDWriteLog.init_shutdown
[       OK ] TestMockCacheSSDWriteLog.init_shutdown (303 ms)
[ RUN      ] TestMockCacheSSDWriteLog.write
[       OK ] TestMockCacheSSDWriteLog.write (312 ms)
[ RUN      ] TestMockCacheSSDWriteLog.read_hit_ssd_cache
[       OK ] TestMockCacheSSDWriteLog.read_hit_ssd_cache (317 ms)
[ RUN      ] TestMockCacheSSDWriteLog.read_hit_part_ssd_cache
[       OK ] TestMockCacheSSDWriteLog.read_hit_part_ssd_cache (312 ms)
[ RUN      ] TestMockCacheSSDWriteLog.read_miss_ssd_cache
[       OK ] TestMockCacheSSDWriteLog.read_miss_ssd_cache (312 ms)
[ RUN      ] TestMockCacheSSDWriteLog.compare_and_write_compare_matched
[       OK ] TestMockCacheSSDWriteLog.compare_and_write_compare_matched (312 ms)
[ RUN      ] TestMockCacheSSDWriteLog.compare_and_write_compare_failed
[       OK ] TestMockCacheSSDWriteLog.compare_and_write_compare_failed (311 ms)
[ RUN      ] TestMockCacheSSDWriteLog.writesame
[       OK ] TestMockCacheSSDWriteLog.writesame (312 ms)
[ RUN      ] TestMockCacheSSDWriteLog.discard
[       OK ] TestMockCacheSSDWriteLog.discard (317 ms)
[ RUN      ] TestMockCacheSSDWriteLog.invalidate
[       OK ] TestMockCacheSSDWriteLog.invalidate (312 ms)
[ RUN      ] TestMockCacheSSDWriteLog.flush
[       OK ] TestMockCacheSSDWriteLog.flush (308 ms)
[ RUN      ] TestMockCacheSSDWriteLog.flush_source_shutdown
[       OK ] TestMockCacheSSDWriteLog.flush_source_shutdown (308 ms)
[ RUN      ] TestMockCacheSSDWriteLog.flush_source_internal
[       OK ] TestMockCacheSSDWriteLog.flush_source_internal (316 ms)
[ RUN      ] TestMockCacheSSDWriteLog.flush_source_user
[       OK ] TestMockCacheSSDWriteLog.flush_source_user (331 ms)
[----------] 16 tests from TestMockCacheSSDWriteLog (4440 ms total)

[----------] Global test environment tear-down
[==========] 16 tests from 1 test suite ran. (4492 ms total)
[  PASSED  ] 16 tests.

Dismissing pending aio_read_data_block() solution.

src/librbd/cache/pwl/ssd/WriteLog.cc

idryomov · 2021-08-11T17:22:30Z

jenkins test make check

SSD read is not like rwl's. SSD need aio read. Therefore, we cannot guarantee that the data will not be retire during the period from sending the read request to the SSD and receiving the data to the memory, which may cause the corresponding data on the SSD to be overwritten. Fixes: https://tracker.ceph.com/issues/52236 Signed-off-by: Feng Hualong <hualong.feng@intel.com>

hualongfeng · 2021-08-12T05:20:06Z

jenkins test make check arm64

idryomov · 2021-08-16T09:28:26Z

https://pulpito.ceph.com/dis-2021-08-12_20:19:57-rbd-wip-dis-testing-distro-basic-smithi/

github-actions bot added the rbd label Aug 2, 2021

MahatiC reviewed Aug 3, 2021

View reviewed changes

hualongfeng force-pushed the solve_competition_between_read_and_retire branch from db3ae82 to 949ca0c Compare August 3, 2021 06:43

idryomov added the bug-fix label Aug 4, 2021

idryomov requested changes Aug 4, 2021

View reviewed changes

hualongfeng force-pushed the solve_competition_between_read_and_retire branch from 949ca0c to 0a263fa Compare August 5, 2021 02:33

idryomov requested changes Aug 5, 2021

View reviewed changes

src/librbd/cache/pwl/ssd/WriteLog.cc Outdated Show resolved Hide resolved

src/librbd/cache/pwl/ssd/WriteLog.cc Outdated Show resolved Hide resolved

src/librbd/cache/pwl/rwl/LogEntry.cc Show resolved Hide resolved

hualongfeng force-pushed the solve_competition_between_read_and_retire branch from 0a263fa to b248917 Compare August 5, 2021 12:24

idryomov reviewed Aug 5, 2021

View reviewed changes

src/librbd/cache/pwl/ssd/WriteLog.cc Outdated Show resolved Hide resolved

src/librbd/cache/pwl/ssd/WriteLog.cc Outdated Show resolved Hide resolved

src/librbd/cache/pwl/ssd/WriteLog.cc Show resolved Hide resolved

hualongfeng force-pushed the solve_competition_between_read_and_retire branch 3 times, most recently from 3d0e54a to b5b592c Compare August 6, 2021 01:21

idryomov previously approved these changes Aug 6, 2021

View reviewed changes

idryomov added the wip-dis-testing label Aug 6, 2021

hualongfeng force-pushed the solve_competition_between_read_and_retire branch from b5b592c to d17a643 Compare August 6, 2021 10:08

idryomov removed the wip-dis-testing label Aug 6, 2021

idryomov self-requested a review August 6, 2021 10:37

hualongfeng force-pushed the solve_competition_between_read_and_retire branch from d17a643 to 364a32e Compare August 7, 2021 13:12

idryomov reviewed Aug 9, 2021

View reviewed changes

src/librbd/cache/pwl/ssd/WriteLog.cc Outdated Show resolved Hide resolved

hualongfeng force-pushed the solve_competition_between_read_and_retire branch from 364a32e to d4baa53 Compare August 10, 2021 02:22

idryomov reviewed Aug 10, 2021

View reviewed changes

src/librbd/cache/pwl/ssd/WriteLog.cc Outdated Show resolved Hide resolved

src/librbd/cache/pwl/ssd/WriteLog.cc Outdated Show resolved Hide resolved

hualongfeng force-pushed the solve_competition_between_read_and_retire branch from d4baa53 to 5bb990a Compare August 11, 2021 02:10

idryomov force-pushed the solve_competition_between_read_and_retire branch from 5bb990a to dfdb452 Compare August 11, 2021 20:15

idryomov approved these changes Aug 11, 2021

View reviewed changes

idryomov added the wip-dis-testing label Aug 11, 2021

idryomov merged commit 076a39c into ceph:master Aug 16, 2021

ideepika mentioned this pull request Nov 2, 2021

pacific: librbd/cache/pwl: persistant cache backports #43772

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

librbd/cache/pwl/ssd: solve competition between read and retire #42580

librbd/cache/pwl/ssd: solve competition between read and retire #42580

hualongfeng commented Aug 2, 2021

hualongfeng commented Aug 2, 2021

MahatiC left a comment •

edited

MahatiC Aug 3, 2021 •

edited

hualongfeng Aug 3, 2021

hualongfeng Aug 6, 2021

idryomov Aug 6, 2021

hualongfeng commented Aug 3, 2021

MahatiC commented Aug 3, 2021

hualongfeng commented Aug 5, 2021

idryomov left a comment

idryomov commented Aug 5, 2021

MahatiC commented Aug 6, 2021

hualongfeng commented Aug 6, 2021

MahatiC commented Aug 6, 2021

hualongfeng commented Aug 6, 2021

idryomov commented Aug 11, 2021

hualongfeng commented Aug 12, 2021

idryomov commented Aug 16, 2021

		@@ -136,6 +136,8 @@ class WriteLog : public AbstractWriteLog<ImageCtxT> {
		Context *ctx);
		void aio_read_data_blocks(std::vector<WriteLogCacheEntry*> &log_entries,

librbd/cache/pwl/ssd: solve competition between read and retire #42580

librbd/cache/pwl/ssd: solve competition between read and retire #42580

Conversation

hualongfeng commented Aug 2, 2021

Checklist

hualongfeng commented Aug 2, 2021

MahatiC left a comment • edited

Choose a reason for hiding this comment

MahatiC Aug 3, 2021 • edited

Choose a reason for hiding this comment

hualongfeng Aug 3, 2021

Choose a reason for hiding this comment

hualongfeng Aug 6, 2021

Choose a reason for hiding this comment

idryomov Aug 6, 2021

Choose a reason for hiding this comment

hualongfeng commented Aug 3, 2021

MahatiC commented Aug 3, 2021

hualongfeng commented Aug 5, 2021

idryomov left a comment

Choose a reason for hiding this comment

idryomov commented Aug 5, 2021

MahatiC commented Aug 6, 2021

hualongfeng commented Aug 6, 2021

MahatiC commented Aug 6, 2021

hualongfeng commented Aug 6, 2021

idryomov commented Aug 11, 2021

hualongfeng commented Aug 12, 2021

idryomov commented Aug 16, 2021

MahatiC left a comment •

edited

MahatiC Aug 3, 2021 •

edited