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: aio calls may block #4854

Merged
merged 14 commits into from Sep 3, 2015

Conversation

Projects
None yet
6 participants
@dillaman
Contributor

dillaman commented Jun 4, 2015

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jun 8, 2015

Passed RBD teuthology suite

@ghost ghost self-assigned this Jun 12, 2015

@ghost ghost changed the title from librbd: aio calls may block to DNM: librbd: aio calls may block Jun 12, 2015

@ghost

This comment has been minimized.

ghost commented Jun 12, 2015

@dillaman it would be good to have comments in the commit messages regarding conflict resolution (reminder ;-)

@dillaman dillaman changed the title from DNM: librbd: aio calls may block to librbd: aio calls may block Jun 22, 2015

@ghost ghost changed the title from librbd: aio calls may block to [DNM] librbd: aio calls may block Jul 8, 2015

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jul 24, 2015

@dachary @dillaman wrote that this PR has already passed an RBD suite, but in view of its complexity should we put it in the next integration test round so it gets tested some more?

@ghost

This comment has been minimized.

ghost commented Jul 24, 2015

@dillaman would you be so kind as to rebase & re-push this PR so that it triggers the make check bot ? I do not remember why I changed to DNM, I suspect it's just because it failed compilation in the integration branch. But I failed to mention the reason for the change.

yuyuyu101 and others added some commits Dec 1, 2014

CephContext: Add AssociatedSingletonObject to allow CephContext's sin…
…gleton

If some objects associated to CephContext want to create a singleton object,
it can inherit AssociatedSingletonObject and implement destruction to get notified.

Signed-off-by: Haomai Wang <haomaiwang@gmail.com>
(cherry picked from commit 7fed5de)
common/ceph_context: don't import std namespace
This was broken by 7fed5de

Signed-off-by: Sage Weil <sage@redhat.com>
(cherry picked from commit 9029813)
WorkQueue: add new ContextWQ work queue
The queue holds a collection of Context pointers that will
be completed by the thread pool.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 24a33e9)

Conflicts:
	src/common/WorkQueue.h: trivial resolution
librbd: add task pool / work queue for AIO requests
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit afb896d)

Conflicts:
	src/common/config_opts.h: trivial resolution
	src/librbd/ImageCtx.cc: trivial resolution
	src/librbd/ImageCtx.h: trivial resolution
	src/librbd/internal.cc: trivial resolution
librbd: avoid blocking AIO API methods
Enqueue all AIO API methods within the new librbd thread pool to
reduce the possibility of any blocking operations. To maintain
backwards compatibility with the legacy return codes of the API's
AIO methods, it's still possible to block attempting to acquire
the snap_lock.

Fixes: #11056
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 3a7b5e3)

Conflicts:
	src/librbd/librbd.cc: trivial resolution
librbd: add new fail method to AioCompletion
Helper method to handle passing fatal errors generated within
librbd (not from the OSDs) back to the client.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 6d1d0c8)

Conflicts:
	src/librbd/AioCompletion.cc: trivial resolution
	src/librbd/AioCompletion.h: trivial resolution
librbd: internal AIO methods no longer return result
All failures should be returned via the AioCompletion.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 9ab42d6)

Conflicts:
	src/librbd/AioRequest.cc: trivial resolution
	src/librbd/internal.cc: trivial resolution
	src/librbd/internal.h: trivial resolution
Throttle: added pending_error method to SimpleThrottle
Allow the client of SimpleThrottle to detect an async error
so that it can exit early.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit b88b88c)
tests: update librbd AIO tests to remove result code
Signed-off-by: Jason Dillaman <dillaman@redhat.com>

Conflicts:
	src/test/librbd/test_librbd.cc: trivial resolution
librbd: AioRequest::send no longer returns a result
The librados calls used by AioRequest::send should always return
zero unless there is a bug.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit c77bce3)

Conflicts:
	src/librbd/AioRequest.cc: trivial resolution
	src/librbd/AioRequest.h: trivial resolution
	src/librbd/internal.cc: trivial resolution
PendingReleaseNotes: document changes to librbd's aio_read methods
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
WorkQueue: added virtual destructor
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit b3f5a75)
librbd: new rbd_non_blocking_aio config option
Setting this option to false reverts librbd to legacy behavior
where AIO operations could potentially block.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 769cad1)

Conflicts:
	src/common/config_opts.h: trivial resolution
	src/librbd/librbd.cc: trivial resolution
tests: verify librbd blocking aio code path
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 4cf4148)

@dillaman dillaman changed the title from [DNM] librbd: aio calls may block to librbd: aio calls may block Jul 24, 2015

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jul 24, 2015

@dachary Rebased and pushed. @smithfarm This fix is actually already available within the downstream Red Hat Ceph Storage 1.2.x product, so the sooner we can get it in upstream, the better in my humble opinion.

@ghost

This comment has been minimized.

ghost commented Jul 24, 2015

@dillaman is the patch id of all the commits the same ? How much testing did it see ?

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jul 24, 2015

@dillaman: JFYI the tracker issue says "DNM until Josh provides +1"

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jul 24, 2015

@jdurgin Thoughts?

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jul 24, 2015

(We will run integration tests on this before merging it in any case.)

@ghost

This comment has been minimized.

ghost commented Jul 24, 2015

@dillaman @smithfarm my concern here is to avoid having a significant amount of commits that look like they are different in hammer and in another context (redhat or suse product), because of tiny differences that change the patch id. For all intent and purposes such commits are the same but from the point of view of a tool, they are different (git cherry will find them different) and it takes a human being to compare them and decide the difference is not significant.

If the commits that exist elsewhere did not see much testing and have not been released yet, the commits from this pull request could probably just be used to replace them. If the commits that exist elsewhere have been extensively tested, and if their patch-id is different, it is very little effort to update this pull request with them (replace the current ones), for the sake of having a clean history.

Hopefully that clarifies a little the context in which I asked "is the patch id of all the commits the same ? How much testing did it see ? ".

Sorry for being so terse ;-)

P.S. See also http://www.spinics.net/lists/ceph-devel/msg24489.html

@ghost

This comment has been minimized.

ghost commented Jul 24, 2015

All but two commits have the same patch-id in v0.80.8.2. The reason for these divergences are explained in the "reconciliation between firefly and v0.80.8.2" mail on ceph-devel.

@ghost

This comment has been minimized.

ghost commented Jul 24, 2015

@smithfarm no further comment, all is well :-)

@jdurgin

This comment has been minimized.

Member

jdurgin commented Jul 24, 2015

No problems from this in master. Looks good to me once it passes through the rbd suite. Generally you can assume that for trivial backports, or ones with just trivial conflicts, I'm fine with merging them after an rbd suite run.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Sep 2, 2015

@jdurgin: it passed the rbd suite. OK to merge? ( see http://tracker.ceph.com/issues/11644#rbd )

@jdurgin

This comment has been minimized.

Member

jdurgin commented Sep 3, 2015

@smithfarm yup, lgtm

smithfarm added a commit that referenced this pull request Sep 3, 2015

Merge pull request #4854 from ceph/wip-11769-firefly
librbd: aio calls may block

Reviewed-by: Josh Durgin <jdurgin@redhat.com>

@smithfarm smithfarm merged commit 99612e7 into firefly Sep 3, 2015

@smithfarm smithfarm deleted the wip-11769-firefly branch Sep 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment