Skip to content
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

librbd: bug fixes for optional data pool support #11960

Merged
merged 8 commits into from Nov 21, 2016

Conversation

vshankar
Copy link
Contributor

This PR enables running librbd with an optional data pool. Relevant support was already added a while ago in the form of an additional "--datapool" option while creating RBD images, but there were places in librbd that were not fully functional to fully support using optional data pool. Furthermore, running teuthology tests revealed a bunch of failed tests which this PR aims to resolve.

NOTE: teuthology runs are being scheduled with this PR on top of Sam's ec-partial-overwrite branch.

@@ -146,7 +146,7 @@ bool CopyupRequest::send_copyup() {
ldout(m_ictx->cct, 20) << __func__ << " " << this << " copyup with "
<< "empty snapshot context" << dendl;
librados::AioCompletion *comp = util::create_rados_safe_callback(this);
r = m_ictx->md_ctx.aio_operate(m_oid, comp, &copyup_op, 0, snaps);
r = m_ictx->data_ctx.aio_operate(m_oid, comp, &copyup_op, 0, snaps);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, but not 100% correct. You will need to use a new "data_ctx" here -- it needs to be pointing to the correct pool but its snapshot context needs to be empty (like md_ctx).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dillaman I understand what you mean and see why it's required. I tried to see what's happens in the OSD when m_ictx->data_ctx is used and it seems that the SnapContext is empty in this case too (empty snaps during rbd.copyup and list of SnapContext for the actual (write) call):

2016-11-15 14:26:44.078037 7fc8807cc700 10 osd.2 pg_epoch: 44 pg[3.e( v 32'11 (0'0,32'11] local-les=35 n=5 ec=11 les/c/f 35/35/0 34/34/34) [2,0,1] r=0 lpr=34 crt=32'11 lcod 0'0 mlcod 0'0 active+clean] execute_ctx 0x560cf27f5c00
2016-11-15 14:26:44.078049 7fc8807cc700 10 osd.2 pg_epoch: 44 pg[3.e( v 32'11 (0'0,32'11] local-les=35 n=5 ec=11 les/c/f 35/35/0 34/34/34) [2,0,1] r=0 lpr=34 crt=32'11 lcod 0'0 mlcod 0'0 active+clean] do_op 3:71a2fc68:::rbd_data.85382ae8944a.0000000000000000:head [call rbd.copyup] ov 0'0 av 44'12 snapc 0=[] snapset 0=[]:[]

2016-11-15 14:26:44.220713 7fc8807cc700 10 osd.2 pg_epoch: 44 pg[3.e( v 44'12 (0'0,44'12] local-les=35 n=6 ec=11 les/c/f 35/35/0 34/34/34) [2,0,1] r=0 lpr=34 crt=32'11 lcod 32'11 mlcod 32'11 active+clean] execute_ctx 0x560cf2a8f500
2016-11-15 14:26:44.220726 7fc8807cc700 10 osd.2 pg_epoch: 44 pg[3.e( v 44'12 (0'0,44'12] local-les=35 n=6 ec=11 les/c/f 35/35/0 34/34/34) [2,0,1] r=0 lpr=34 crt=32'11 lcod 32'11 mlcod 32'11 active+clean] do_op 3:71a2fc68:::rbd_data.85382ae8944a.0000000000000000:head [set-alloc-hint object_size 4194304 write_size 4194304,write 0~0] ov 44'12 av 44'13 snapc 14=[14,13,12,11,10,f,e,d] snapset 0=[]:[]+head

Note that I've made the changes as suggested though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dillaman , i know we have to set snapc to empty for this osd op, as @vshankar said, why we need to create a new data_ctx instead of the original m_ictx->data_ctx ? thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@runsisi As @vshankar mentioned, it actually does work the other way because it's the form of aio_operate that allows you to override the current snap context associated with the IoCtx. That was my mistake.

@dillaman dillaman added this to the kraken milestone Nov 14, 2016
@vshankar vshankar force-pushed the wip-librbd-ec-support branch 2 times, most recently from 6820fbc to 9d1b605 Compare November 15, 2016 10:11
Copy link

@dillaman dillaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dillaman
Copy link

Related ceph-qa-suite changes: ceph/ceph-qa-suite#1235

@dillaman
Copy link

retest this please

@vshankar vshankar changed the title [DNM]: librbd optional data pool support librbd optional data pool support Nov 15, 2016
@dillaman dillaman changed the title librbd optional data pool support librbd: bug fixes for optional data pool support Nov 15, 2016
@ghost
Copy link

ghost commented Nov 17, 2016

jenkins test this please (eio ignored in master)

@ghost
Copy link

ghost commented Nov 17, 2016

jenkins test this please (osd-scrub-snaps.sh)

@ghost
Copy link

ghost commented Nov 18, 2016

jenkins test this please (eio now ignored in master)

close() was never called for the passed in IoCtx which
could probably result in an IoCtx leak if the original
IoCtx was a valid pool context allocated earlier.

Its kind of better to do it here rather than to leave
the destruction on the caller for better (or cleaner)
common case handling.

Signed-off-by: Venky Shankar <vshankar@redhat.com>
Signed-off-by: Venky Shankar <vshankar@redhat.com>
Signed-off-by: Venky Shankar <vshankar@redhat.com>
Signed-off-by: Venky Shankar <vshankar@redhat.com>
Signed-off-by: Venky Shankar <vshankar@redhat.com>
Signed-off-by: Venky Shankar <vshankar@redhat.com>
Signed-off-by: Venky Shankar <vshankar@redhat.com>
Signed-off-by: Venky Shankar <vshankar@redhat.com>
@vshankar
Copy link
Contributor Author

Changes in this patchset:

  • fix import_export.sh and test_rbd.py to support optional image data pool

@vshankar
Copy link
Contributor Author

@dillaman
Copy link

Re-running subset on smithi nodes to remove all the false failures due to VPS resource constraints:

http://pulpito.ceph.com/jdillaman-2016-11-21_08:55:50-rbd-wip-ec-partial-overwrites---basic-smithi/

@vshankar
Copy link
Contributor Author

Re-running subset on smithi nodes to remove all the false failures due to VPS resource constraints:

http://pulpito.ceph.com/jdillaman-2016-11-21_08:55:50-rbd-wip-ec-partial-overwrites---basic-smithi/

Thanks @dillaman

@dillaman dillaman merged commit 5cd929a into ceph:master Nov 21, 2016
@vshankar vshankar deleted the wip-librbd-ec-support branch November 22, 2016 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants