librbd: compare and write against a clone can result in failure #18887

dillaman · 2017-11-11T02:47:09Z

No description provided.

dillaman · 2017-11-13T15:30:08Z

Test run (pre-shrink object map fix): http://pulpito.ceph.com/jdillaman-2017-11-12_12:43:25-rbd-wip-jd-testing-distro-basic-smithi/

dillaman · 2017-11-13T17:24:43Z

... fsx tests now passing after fixing shrink issue: http://pulpito.ceph.com/jdillaman-2017-11-13_11:22:59-rbd-wip-jd-testing-distro-basic-smithi/

trociny · 2017-11-14T21:15:10Z

src/librbd/io/CopyupRequest.cc

    for (size_t i=0; i<m_pending_requests.size(); ++i) {
-      ObjectRequest<> *req = m_pending_requests[i];
+      auto req = m_pending_requests[i];


nit: probably for (auto req : m_pending_requests) instead?

trociny · 2017-11-14T21:18:30Z

src/librbd/io/ObjectRequest.cc

+    librados::ObjectWriteOperation *wr) {
+  I *image_ctx = this->m_ictx;
+  RWLock::RLocker snap_locker(image_ctx->snap_lock);
+  if (image_ctx->enable_alloc_hint &&


nit: may be no need to check for image_ctx->enable_alloc_hint here as it will be checked by ObjectRequest<I>::add_write_hint() anyway?

trociny · 2017-11-15T11:58:36Z

@dillaman fsx failure:

http://qa-proxy.ceph.com/teuthology/trociny-2017-11-14_21:38:44-rbd-wip-mgolub-testing-distro-basic-smithi/1849099/teuthology.log

Reproduced locally with:

CEPH_ARGS='--rbd_skip_partial_discard=true' ceph_test_librbd_fsx -d -W -R -p 100 -P /tmp/fsx -r 1 -w 1 -t 1 -h 1 -l 250000000 -S 6423 -N 20000 rbd image_client.0

trociny · 2017-11-15T12:45:59Z

Another failure case:

http://pulpito.ceph.com/trociny-2017-11-14_21:42:50-rbd-wip-mgolub-testing-distro-basic-smithi/

Reproduced locally with:

ceph_test_librbd_fsx -d -W -R -p 100 -P /tmp/fsx -r 512 -w 512 -t 512 -h 512 -l 250000000 -S 9072 -N 6000 -M rbd image_client.5

(It looks like rbd-nbd is important as I can't reproduce without -M)

dillaman · 2017-11-15T13:13:01Z

@trociny The skip_partial_discard failure looks unrelated since compare and write would never have worked given how it's handled by fsx. It might just be better to just disable the compare and write operator when skip_partial_discard is enabled. I'll take a look at the rbd-nbd failure and see if I can reproduce).

dillaman · 2017-11-15T13:22:28Z

... of course rbd-nbd does not function at all on F26.

trociny · 2017-11-15T14:28:15Z

... of course rbd-nbd does not function at all on F26.

Hm, is this something F26 specific? Just in case, didn't you forget to set PATH properly in root environment?

dillaman · 2017-11-15T14:30:34Z

@trociny No, it starts rbd-nbd daemon and maps the device just fine. The first time I ran it it immediately failed on the first write call with an "out of space" error. The second time I ran it (after cleaning up my environment), it failed with "no extend on truncate! not posix" -- so it doesn't seem to be picking up the resize operations correctly.

dillaman · 2017-11-15T14:31:48Z

... I'm playing around w/ it to see if I can get it working while an Ubuntu build is in-progress.

trociny · 2017-11-15T14:51:18Z

src/test/librbd/fsx.cc

@@ -2069,6 +2069,13 @@ docompareandwrite(unsigned offset, unsigned size)
        if (o_direct)
                size -= size % writebdy;

+        if (skip_partial_discard) {
+                if (!quiet && testcalls > simulatedopcount && !o_direct)


I suppose !o_direct check is not needed here. I assume it is if (size == 0) specific because we may truncate size in o_direct case.

Probably it is a good idea to move this if (skip_partial_discard) block above if (o_direct) block or below if (size == 0) block as they look related.

dillaman · 2017-11-15T15:19:18Z

@trociny I had to add the following to get F26 to rescan the partition table and pick up the size change on the /dev/nbdX device:

diff --git a/src/tools/rbd_nbd/rbd-nbd.cc b/src/tools/rbd_nbd/rbd-nbd.cc
index 371f5301ba..669b986641 100644
--- a/src/tools/rbd_nbd/rbd-nbd.cc
+++ b/src/tools/rbd_nbd/rbd-nbd.cc
@@ -476,6 +476,10 @@ public:
         } else {
           size = new_size;
         }
+        if (ioctl(fd, BLKRRPART, NULL) < 0) {
+            derr << "rescan of partition table failed: " << cpp_strerror(errno)
+                 << dendl;
+        }
         if (image.invalidate_cache() < 0)
             derr << "invalidate rbd cache failed" << dendl;
       }

dillaman · 2017-11-15T16:53:49Z

@trociny Fixed the issue -- the ObjectReadRequest was skipping the read immediately if the object map said the object didn't exist. I moved that check to ObjectReadRequest::read_object so that it properly checks the writeback cache first.

trociny · 2017-11-15T19:39:19Z

@dillaman TestMockIoObjectRequest need tweaking. See jenkins logs.

dillaman · 2017-11-15T20:07:10Z

@trociny Sorry about that -- should be all better now.

trociny · 2017-11-16T10:55:32Z

src/librbd/io/CopyupRequest.cc

@@ -264,7 +256,11 @@ bool CopyupRequest<I>::should_complete(int r)

  case STATE_COPYUP:
    // invoked via a finisher in librados, so thread safe


@dillaman I assume this comment is not relevant now?

…ent file Signed-off-by: Jason Dillaman <[email protected]>

Signed-off-by: Jason Dillaman <[email protected]>

The ReadResult wrapper requires the buffer extents to know how to properly reconstruct the out buffer. Signed-off-by: Jason Dillaman <[email protected]>

Signed-off-by: Jason Dillaman <[email protected]>

This adds support for sparse-reads and ensures all object reads utilize a single, tested code path. Signed-off-by: Jason Dillaman <[email protected]>

The compare-and-write object operation cannot be executed concurrently within a copyup operation since the object might not exist yet (if not performing a deep-copy). Signed-off-by: Jason Dillaman <[email protected]>

The initial copyup was not receiving a write hint and the code for hints was duplicated multiple times. Additionally, the object map state should match the last executed write op. Signed-off-by: Jason Dillaman <[email protected]>

…e machine Signed-off-by: Jason Dillaman <[email protected]>

Fixes: http://tracker.ceph.com/issues/20789 Signed-off-by: Jason Dillaman <[email protected]>

Signed-off-by: Jason Dillaman <[email protected]>

trociny · 2017-11-16T14:27:27Z

@ceph-jenkins retest this please

trociny

LGTM

dillaman added bug-fix cleanup rbd labels Nov 11, 2017

dillaman force-pushed the wip-20789 branch 3 times, most recently from 4ce1871 to bfac405 Compare November 13, 2017 15:26

dillaman changed the title ~~[DNM] librbd: compare and write against a clone can result in failure~~ librbd: compare and write against a clone can result in failure Nov 13, 2017

trociny added the wip-mgolub-testing label Nov 14, 2017

dillaman mentioned this pull request Nov 14, 2017

librbd: add missing space between op_type and m_oid in debug msg #18915

Closed

trociny reviewed Nov 14, 2017

View reviewed changes

dillaman force-pushed the wip-20789 branch from bfac405 to bf54969 Compare November 15, 2017 13:26

trociny reviewed Nov 15, 2017

View reviewed changes

dillaman force-pushed the wip-20789 branch from bf54969 to 9b88c4f Compare November 15, 2017 16:52

dillaman force-pushed the wip-20789 branch from 9b88c4f to 42072b0 Compare November 15, 2017 17:38

dillaman force-pushed the wip-20789 branch from 42072b0 to 3faaf9e Compare November 15, 2017 20:06

trociny reviewed Nov 16, 2017

View reviewed changes

test/librados_test_stub: simulate set_alloc_hint creating a non-exist…

4b851ce

…ent file Signed-off-by: Jason Dillaman <[email protected]>

Jason Dillaman added 12 commits November 16, 2017 07:31

test/librados_test_stub: mocked remainder of IO ops used by librbd

c4e5613

Signed-off-by: Jason Dillaman <[email protected]>

librbd: templatize IO object request state machines

654a78f

Signed-off-by: Jason Dillaman <[email protected]>

librbd: ObjectReadRequest does not require buffer extents

35ce1c2

The ReadResult wrapper requires the buffer extents to know how to properly reconstruct the out buffer. Signed-off-by: Jason Dillaman <[email protected]>

librbd: reduce lock scope when assembling read results

284feb6

Signed-off-by: Jason Dillaman <[email protected]>

librbd: default template parameter for C_SparseReadRequest

ab0e558

Signed-off-by: Jason Dillaman <[email protected]>

librbd: refactor io::ObjectReadRequest

62c3282

Signed-off-by: Jason Dillaman <[email protected]>

librbd: object cacher should re-use read state machine

e79c0cf

This adds support for sparse-reads and ensures all object reads utilize a single, tested code path. Signed-off-by: Jason Dillaman <[email protected]>

librbd: copyup state machine needs to handle empty write ops

f6db9b8

The compare-and-write object operation cannot be executed concurrently within a copyup operation since the object might not exist yet (if not performing a deep-copy). Signed-off-by: Jason Dillaman <[email protected]>

librbd: consolidate all object discard-related logic into single stat…

318797f

…e machine Signed-off-by: Jason Dillaman <[email protected]>

librbd: refactor object write request state machines

0e643fb

Fixes: http://tracker.ceph.com/issues/20789 Signed-off-by: Jason Dillaman <[email protected]>

test/librbd: disable compare and write if skipping partial discard

4e8b0b6

Signed-off-by: Jason Dillaman <[email protected]>

dillaman force-pushed the wip-20789 branch from 3faaf9e to 4e8b0b6 Compare November 16, 2017 12:32

trociny approved these changes Nov 16, 2017

View reviewed changes

trociny merged commit 8defef4 into ceph:master Nov 16, 2017

dillaman deleted the wip-20789 branch November 16, 2017 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

librbd: compare and write against a clone can result in failure #18887

librbd: compare and write against a clone can result in failure #18887

dillaman commented Nov 11, 2017

dillaman commented Nov 13, 2017

dillaman commented Nov 13, 2017

trociny Nov 14, 2017

trociny Nov 14, 2017

trociny commented Nov 15, 2017

trociny commented Nov 15, 2017

dillaman commented Nov 15, 2017

dillaman commented Nov 15, 2017

trociny commented Nov 15, 2017

dillaman commented Nov 15, 2017

dillaman commented Nov 15, 2017

trociny Nov 15, 2017

dillaman commented Nov 15, 2017

dillaman commented Nov 15, 2017

trociny commented Nov 15, 2017

dillaman commented Nov 15, 2017

trociny Nov 16, 2017

dillaman Nov 16, 2017

trociny commented Nov 16, 2017

trociny left a comment

		@@ -264,7 +256,11 @@ bool CopyupRequest<I>::should_complete(int r)

		case STATE_COPYUP:
		// invoked via a finisher in librados, so thread safe

librbd: compare and write against a clone can result in failure #18887

librbd: compare and write against a clone can result in failure #18887

Conversation

dillaman commented Nov 11, 2017

dillaman commented Nov 13, 2017

dillaman commented Nov 13, 2017

trociny Nov 14, 2017

Choose a reason for hiding this comment

trociny Nov 14, 2017

Choose a reason for hiding this comment

trociny commented Nov 15, 2017

trociny commented Nov 15, 2017

dillaman commented Nov 15, 2017

dillaman commented Nov 15, 2017

trociny commented Nov 15, 2017

dillaman commented Nov 15, 2017

dillaman commented Nov 15, 2017

trociny Nov 15, 2017

Choose a reason for hiding this comment

dillaman commented Nov 15, 2017

dillaman commented Nov 15, 2017

trociny commented Nov 15, 2017

dillaman commented Nov 15, 2017

trociny Nov 16, 2017

Choose a reason for hiding this comment

dillaman Nov 16, 2017

Choose a reason for hiding this comment

trociny commented Nov 16, 2017

trociny left a comment

Choose a reason for hiding this comment