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

rbd-nbd: ensure unmap returns error code #15593

Merged
merged 1 commit into from Jun 13, 2017

Conversation

Projects
None yet
3 participants
@chenfangxian
Contributor

chenfangxian commented Jun 9, 2017

fix nbd disconnect error handling, when nbd disconnect failed,
ensure do_unmap return error logic.

Signed-off-by: chenfangxian chenfangxian@cmss.chinamobile.com
Signed-off-by: guojiannan guojiannan@cmss.chinamobile.com

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 12, 2017

retest this please.

@liupan1111 liupan1111 requested a review from trociny Jun 12, 2017

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 12, 2017

Jenkins failure is not related with this change. LGTM.

@trociny

Could you please add "rbd-ndb: " prefix to your commit message?

@@ -776,19 +776,24 @@ static int do_map(int argc, const char *argv[])
static int do_unmap()
{

This comment has been minimized.

@trociny

trociny Jun 12, 2017

Contributor

This empty line looks like not necessary.

r = ioctl(nbd, NBD_DISCONNECT);
if (r < 0) {
r = -errno;
cerr << "rbd-nbd: disconnect failed: " << cpp_strerror(-r) << std::endl;

This comment has been minimized.

@trociny

trociny Jun 12, 2017

Contributor

I am not sure about changing the error message. I can see reasons why it is this way right now. If open_device succeeded, but NBD_DISCONNECT failed, "the device is not used" looks like the most probable (if not the only) explanation. Is your experience different?

This comment has been minimized.

@chenfangxian

chenfangxian Jun 12, 2017

Contributor

@trociny , you are right,I have delete the empty line。and I have modify commit message(add rbd-nbd:), but about the NBD_DISCONNECT error message,I think the logic of disconnect is being executed,when disconnect ioctl happen error,use the error message rbd-nbd: disconnect failed and strerror(errno) can display the problem more obvious, for example,When the nbd device do disconnect by someone, the nbd device may be being used by others(by someone mount and so on)。thank you。

This comment has been minimized.

@trociny

trociny Jun 12, 2017

Contributor

I am not sure I understand your example. If an nbd device is used by someone it should be in connected state. Then NBD_DISCONNECT will succeed. Looking at the current code of the nbd driver [1] it looks like nbd_disconnect will return error (-EINVAL) only when the device is not connected. So for me:

rbd-nbd: the device is not used

looks nicer than

rbd-nbd: disconnect failed: (22) Invalid argument

Though, I don't have strong opinion. I just explain why it could be done this way initially (I am not the author) and why I don't consider this a bug. May be we will hear other people opinion before making a decision.

[1] http://elixir.free-electrons.com/linux/latest/source/drivers/block/nbd.c#L710

This comment has been minimized.

@chenfangxian

chenfangxian Jun 13, 2017

Contributor

@trociny, I considered your suggestion, I also consider the implementation logic of nbd in the kernel, rbd-nbd: the device is not used looks nicer than rbd-nbd: disconnect failed: (22) Invalid argument, I have adjusted the code, thank you!

rbd-nbd: fix nbd do_unmap error handling
when nbd disconnect failed, ensure do_unmap return error logic.

Signed-off-by: chenfangxian <chenfangxian@cmss.chinamobile.com>
Signed-off-by: guojiannan <guojiannan@cmss.chinamobile.com>

@trociny trociny changed the title from tools/rbd_nbd: fix do_unmap error handling to rbd-nbd: ensure unmap returns error code Jun 13, 2017

@trociny

LGTM

@trociny trociny merged commit b7d657d into ceph:master Jun 13, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details

@chenfangxian chenfangxian deleted the chenfangxian:rbd-nbd-fix-unmap branch Jun 13, 2017

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