osd: full-object read CRC mismatch due to 'truncate' modifying oi.size w/o clearing 'data_digest' by NitzanMordhai · Pull Request #55008 · ceph/ceph

NitzanMordhai · 2023-12-26T12:54:40Z

This is continue of PR:
#43902
the commits cherry-picked and i added some tests that recreate the issue without the fix

when write before truncate, need trim length, if truncate is to 0,
write is [0128k], write change to [00], do nothing, oi.size is 0, x1 = set_data_digest(crc32(-1)).
write is [128k128k], write change to [128k0], truncate oi.size to offset 128k, x2 = set_data_digest(crc32(x1)).
write is [256k128k], write change to [256k0], truncate oi.size to offset 256k, x3 = set_data_digest(crc32(x2)).
...
write is [4063232128k], write change to [40632320], truncate oi.size to offset 4063232, xn = set_data_digest(crs32(xn-1))
Now, we can see oi.size is 4063232, and data_digest is 0xffffffff, because thelength of in_data of crc is 0 every time.
when read verify crc will reply EIO. (EC pool).

so, when truncate in write, need clear data_digest and DIGEST flag,
when write before truncate, need to trim length, when offset over than oi.size, don't truncate oi.size to offset.

Fixes: https://tracker.ceph.com/issues/53240
Signed-off-by: jiawd jiawendong@xtaotech.com

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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
jenkins test windows
jenkins test rook e2e

src/test/librados/testcase_cxx.cc

ronen-fr · 2023-12-26T13:08:29Z

@NitzanMordhai - commit test/pybind: check crc fail after append zero
isn't signed

athanatos · 2024-01-02T15:50:39Z

src/osd/PrimaryLogPG.cc

                                             op.extent.truncate_size);
 	    }
+	    //truncate modify oi.size, need clear old data_digest and DIGEST flag
+	    oi.clear_data_digest();


Seems like this should be in the immediately above if block? If so, perhaps we should just move it into truncate_update_size_and_usage?

@athanatos Jia, who wrote the original fix added that note: https://tracker.ceph.com/issues/53240#note-1
this is case 2, write before truncating

Hmm, having read that it's still not clear to me why we clear_data_digest() here even if truncate_size == oi.size.

@athanatos agree, going to move it into the if

src/osd/PrimaryLogPG.cc

rzarzynski · 2024-02-07T00:25:31Z

src/test/librados/io_cxx.cc

 }

+TEST_F(LibRadosIoPP, CrcZeroWrite) {
+  char buf[128];


crimson lacks the fix, so I expect this test to fail there. Might we worth adding at a comment at least.

It's a simple fix -- can we just go ahead and fix it in crimson rather than letting the test fail?

actually, i tested it on crimson and it pass

This test fails sporadically with ENOTSUP:

$ ./ceph_test_rados_api_io_pp # ... /mnt/data/workspace/ceph_linux/src/test/librados/io_cxx.cc:888: Failure Expected equality of these values: 0 ioctx.write("foo", bl, 0, sizeof(buf)) Which is: -95 [ FAILED ] LibRadosIoECPP.CrcZeroWrite (767 ms)

I'm thinking about skipping it on Windows for the time being: ceph/ceph-win32-tests#19

rzarzynski · 2024-02-07T00:26:56Z

src/test/librados/io_cxx.cc

+  bufferlist bl;
+
+  ASSERT_EQ(0, ioctx.write("foo", bl, 0, 0));
+  ASSERT_EQ(0, ioctx.write("foo", bl, 0, sizeof(buf)));


Hmm, the only usage of the 0xcc-filled buf seems to be the sizeof(). Is that truly the intention?

@rzarzynski yes, i had in mind to check that we didn't had any override above zero, but i didn't added it. i can remove the memset.

@rzarzynski please review

rzarzynski · 2024-03-04T13:08:48Z

src/osd/PrimaryLogPG.cc

              truncate_update_size_and_usage(ctx->delta_stats,
                                             oi,
                                             op.extent.truncate_size);
+	      //truncate modify oi.size, need clear old data_digest and DIGEST flag


In this case the logic is a little bit different than in CEPH_OSD_OP_TRUNCATE:

if (op.extent.truncate_seq) { ceph_assert(op.extent.offset == op.extent.truncate_size); // <------------------ if (op.extent.truncate_seq <= oi.truncate_seq) { dout(10) << " truncate seq " << op.extent.truncate_seq << " <= current " << oi.truncate_seq << ", no-op" << dendl; break; // old } dout(10) << " truncate seq " << op.extent.truncate_seq << " > current " << oi.truncate_seq << ", truncating" << dendl; oi.truncate_seq = op.extent.truncate_seq; oi.truncate_size = op.extent.truncate_size; } // ... if (op.extent.offset != oi.size) { // <---------- truncate_update_size_and_usage(ctx->delta_stats, oi, op.extent.offset); } ctx->delta_stats.num_wr++; // do no set exists, or we will break above DELETE -> TRUNCATE munging. oi.clear_data_digest(); // <-------

The difference is what happens in the op.extent.truncate_size == oi.size case: CEPH_OSD_OP_TRUNCATE clears the digest; CEPH_OSD_OP_WRITE – does not.

I think the reason is that the trunc-like write differs from regular trunc. The former puts truncate into the transaction when expanding the object; for shrinking (or keeping same) only nop + update of object_info happens – the data is untouched. Therefore preserving the digest for op.extent.truncate_size == oi.size should be fine.

if (op.extent.length == 0) { if (op.extent.offset > oi.size) { t->truncate( soid, op.extent.offset); truncate_update_size_and_usage(ctx->delta_stats, oi, op.extent.offset); } else { t->nop(soid); } } else { t->write( soid, op.extent.offset, op.extent.length, osd_op.indata, op.flags); }

rzarzynski · 2024-03-04T14:07:12Z

src/osd/PrimaryLogPG.cc

+	        soid, op.extent.offset);
+              truncate_update_size_and_usage(ctx->delta_stats, oi,
+                                             op.extent.offset);
+              oi.clear_data_digest();


ljflores · 2024-03-25T20:38:30Z

This PR had some unit test problems. Ref: https://trello.com/c/SFxA3U2X/1976-wip-yuri3-testing-2024-03-25-0744-old-wip-yuri3-testing-2024-03-24-1519-old-wip-yuri3-testing-2024-03-12-0851

NitzanMordhai · 2024-04-01T04:57:54Z

This PR had some unit test problems. Ref: https://trello.com/c/SFxA3U2X/1976-wip-yuri3-testing-2024-03-25-0744-old-wip-yuri3-testing-2024-03-24-1519-old-wip-yuri3-testing-2024-03-12-0851

the unit-test with EC needed allow_ec_overwrites, but since those test unit doesn't refresh the pool (delete\create) for each test, we can't remove the overwrites setting which causes the other unittest to fail.

rzarzynski · 2024-04-29T17:38:32Z

jenkins test windows

rzarzynski · 2024-04-29T17:38:59Z

@yuriw: may we retake this one into QA?

rzarzynski · 2024-05-06T17:40:53Z

jenkins test windows

rzarzynski · 2024-05-06T17:41:39Z

QA in progress: https://tracker.ceph.com/issues/65349.

yuriw · 2024-05-06T19:18:47Z

jenkins test windows

yuriw · 2024-05-06T19:19:01Z

By @kamoltat "RADOS APPROVED https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues65349"

yuriw · 2024-05-06T19:20:00Z

@NitzanMordhai pls merge when all checks passed

ref: https://tracker.ceph.com/issues/65349

NitzanMordhai · 2024-05-09T08:18:08Z

jenkins test windows

NitzanMordhai · 2024-05-12T05:16:53Z

jenkins test windows

NitzanMordhai · 2024-05-12T07:09:53Z

jenkins retest this please

ljflores · 2024-05-13T17:35:34Z

jenkins test api

ljflores · 2024-05-13T17:35:44Z

jenkins test windows

NitzanMordhai · 2024-05-15T10:36:50Z

jenkins test api

NitzanMordhai · 2024-05-16T12:41:10Z

jenkins test api

…e and forget to clear data_digest when write before truncate, need trim length, if truncate is to 0, write is [0~128k], write change to [0~0], do nothing, oi.size is 0, x1 = set_data_digest(crc32(-1)). write is [128k~128k], write change to [128k~0], truncate oi.size to offset 128k, x2 = set_data_digest(crc32(x1)). write is [256k~128k], write change to [256k~0], truncate oi.size to offset 256k, x3 = set_data_digest(crc32(x2)). ... write is [4063232~128k], write change to [4063232~0], truncate oi.size to offset 4063232, xn = set_data_digest(crs32(xn-1)) Now, we can see oi.size is 4063232, and data_digest is 0xffffffff, because thelength of in_data of crc is 0 every time. when read verify crc will reply EIO. (EC pool). so, when truncate in write, need clear data_digest and DIGEST flag, when write before truncate, need to trim length, when offset over than oi.size, don't truncate oi.size to offset. Fixes: https://tracker.ceph.com/issues/53240 Signed-off-by: jiawd <jiawendong@xtaotech.com>

Fixes: https://tracker.ceph.com/issues/53240 Signed-off-by: jiawd <jiawendong@xtaotech.com>

Add test for zero crc check failed. Fixes: https://tracker.ceph.com/issues/53240 Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

1. adding allow_ec_overwrite option for cxx test 2. adding new test for crc failuer check with append zero length Fixes: https://tracker.ceph.com/issues/53240 Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

petrutlucian94 · 2024-05-24T11:55:35Z

I think the Windows failure was actually legit, one of the tests fails randomly with ENOTSUP.

The make check job doesn't actually run this test.

NitzanMordhai requested a review from a team as a code owner December 26, 2023 12:54

NitzanMordhai requested review from idryomov and removed request for a team December 26, 2023 12:54

github-actions bot added core tests labels Dec 26, 2023

NitzanMordhai requested review from a team and rzarzynski December 26, 2023 12:55

ronen-fr reviewed Dec 26, 2023

View reviewed changes

src/test/librados/testcase_cxx.cc Outdated Show resolved Hide resolved

NitzanMordhai force-pushed the wip-nitzan-clear-data-digest-for-crc-check branch 2 times, most recently from e12c5c1 to 6c1aec3 Compare December 27, 2023 05:59

NitzanMordhai requested a review from ronen-fr December 27, 2023 06:02

athanatos reviewed Jan 2, 2024

View reviewed changes

NitzanMordhai requested a review from athanatos January 3, 2024 13:45

NitzanMordhai force-pushed the wip-nitzan-clear-data-digest-for-crc-check branch 2 times, most recently from 381433a to 01d1f5b Compare January 4, 2024 14:00

rzarzynski reviewed Feb 7, 2024

View reviewed changes

NitzanMordhai force-pushed the wip-nitzan-clear-data-digest-for-crc-check branch from 01d1f5b to e83ce0c Compare February 7, 2024 06:22

NitzanMordhai requested a review from rzarzynski February 7, 2024 06:23

rzarzynski approved these changes Mar 4, 2024

View reviewed changes

rzarzynski added the needs-qa label Mar 4, 2024

ljflores added the wip-yuri3-testing label Mar 12, 2024

yuriw removed the wip-yuri3-testing label Mar 25, 2024

ljflores added TESTED and removed needs-qa labels Mar 25, 2024

NitzanMordhai force-pushed the wip-nitzan-clear-data-digest-for-crc-check branch from e83ce0c to a6e427f Compare April 1, 2024 04:53

NitzanMordhai added the needs-qa label Apr 1, 2024

ronen-fr changed the title ~~osd: full-object read crc is mismatch, because truncate modify oi.size and forget to clear data_digest~~ osd: full-object read CRC mismatch due to 'truncate' modifying oi.size w/o clearing 'data_digest' Apr 1, 2024

yuriw added TESTED ready-to-merge and removed wip-yuri3-testing labels May 6, 2024

jiawd and others added 5 commits May 19, 2024 05:03

osd: Remove the duplicate oi.clear_data_digest

83d3523

Fixes: https://tracker.ceph.com/issues/53240 Signed-off-by: jiawd <jiawendong@xtaotech.com>

osd: Adjust code style

afd4266

Fixes: https://tracker.ceph.com/issues/53240 Signed-off-by: jiawd <jiawendong@xtaotech.com>

test/pybind: check crc fail after append zero

6662e6b

Add test for zero crc check failed. Fixes: https://tracker.ceph.com/issues/53240 Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

ceph_test_rados_io_pp: Add cxx test for append zero test

06e4c6f

1. adding allow_ec_overwrite option for cxx test 2. adding new test for crc failuer check with append zero length Fixes: https://tracker.ceph.com/issues/53240 Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

NitzanMordhai force-pushed the wip-nitzan-clear-data-digest-for-crc-check branch from 415a1b8 to 06e4c6f Compare May 19, 2024 05:05

NitzanMordhai merged commit fd7b38b into ceph:main May 19, 2024

NitzanMordhai deleted the wip-nitzan-clear-data-digest-for-crc-check branch May 19, 2024 07:29

Matan-B mentioned this pull request May 22, 2024

test/pybind/test_rados.py: fix test_list_objects #57616

Merged

14 tasks

Conversation

NitzanMordhai commented Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

ronen-fr commented Dec 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrutlucian94 May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljflores commented Mar 25, 2024

Uh oh!

NitzanMordhai commented Apr 1, 2024

Uh oh!

rzarzynski commented Apr 29, 2024

Uh oh!

rzarzynski commented Apr 29, 2024

Uh oh!

rzarzynski commented May 6, 2024

Uh oh!

rzarzynski commented May 6, 2024

Uh oh!

yuriw commented May 6, 2024

Uh oh!

yuriw commented May 6, 2024

Uh oh!

yuriw commented May 6, 2024

Uh oh!

NitzanMordhai commented May 9, 2024

Uh oh!

NitzanMordhai commented May 12, 2024

Uh oh!

NitzanMordhai commented May 12, 2024

Uh oh!

ljflores commented May 13, 2024

Uh oh!

ljflores commented May 13, 2024

Uh oh!

NitzanMordhai commented May 15, 2024

Uh oh!

NitzanMordhai commented May 16, 2024

Uh oh!

petrutlucian94 commented May 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

NitzanMordhai commented Dec 26, 2023 •

edited

Loading

petrutlucian94 May 24, 2024 •

edited

Loading