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: minor fixes for image trash move #14834

Merged
merged 2 commits into from May 5, 2017

Conversation

Projects
None yet
2 participants
@runsisi
Contributor

runsisi commented Apr 27, 2017

  1. do not free ictx a second time if the sync open failed, otherwise it will assert in perf counter code
  2. do not try to move the image to trash can if the image does not exist

Signed-off-by: runsisi runsisi@zte.com.cn

@@ -1351,9 +1351,7 @@ int validate_pool(IoCtx &io_ctx, CephContext *cct) {
int r = ictx->state->open(true);
if (r < 0) {
ldout(cct, 2) << "error opening image: " << cpp_strerror(-r) << dendl;
if (r != -ENOENT) {

This comment has been minimized.

@dillaman

dillaman Apr 27, 2017

Contributor

I believe the intent of this was the handle the case where you had a crash between adding the image to the trash and removing the image's id object and removing the image from the directory (i.e. run it a second time to get to a known good state). However, since the image id won't be populated, the current implementation is not correct. I think on -ENOENT, you need to do an additional check to retrieve the image id from the directory.

This comment has been minimized.

@runsisi

runsisi Apr 27, 2017

Contributor

@dillaman do you mean on -ENOENT i have to check if we can get the image id from RBD_DIRECTOY by image name, and then try to open it by id again?

This comment has been minimized.

@dillaman

dillaman Apr 27, 2017

Contributor

@runsisi On -ENOENT from image open, just try to retrieve the image id from the directory. If successful, continue -- otherwise, fail with the error code from the directory lookup.

This comment has been minimized.

@runsisi

runsisi Apr 28, 2017

Contributor

@dillaman updated, thanks.

@dillaman

@runsisi I'd rather just have trash_remove perform the directory lookup instead of changing the open state machine for all use-cases. If we have some sort of corruption that resulted in the loss of the image id object, I wouldn't want to silently open the image.

runsisi added some commits Apr 27, 2017

runsisi
librbd: do not delete ictx twice if open image failed
Signed-off-by: runsisi <runsisi@zte.com.cn>
runsisi
librbd: get image id from directory if open image failed
Signed-off-by: runsisi <runsisi@zte.com.cn>
@runsisi

This comment has been minimized.

Contributor

runsisi commented May 2, 2017

@dillaman updated, thanks!

@dillaman

lgtm

@dillaman dillaman merged commit 965cfb5 into ceph:master May 5, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@runsisi runsisi deleted the runsisi:wip-fix-double-free branch May 6, 2017

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