nvme: block ioctls if controller not in a live state
PATCH 1/1 nvme: block ioctls if controller not in a live state
Rather than allow ioctl-based admin cmds to get intermixed on the admin queue with commands being used to initialize a controller or io commands to go to a controller in reconnect thus possibly hanging, reject them if the controller isn't in the LIVE state. Reject with an -EAGAIN status so that the app knows it could retry.
--- drivers/nvme/host/core.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/nvme/host/core.c +4 -0
@@ -1041,6 +1041,8 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)return -EFAULT ;if (io.flags)return -EINVAL ; +if (ns->ctrl->state !=NVME_CTRL_LIVE ) +return -EAGAIN ;switch (io.opcode) {case nvme_cmd_write:
@@ -1174,6 +1176,8 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,return -EFAULT ;if (cmd.flags)return -EINVAL ; +if (ctrl->state !=NVME_CTRL_LIVE ) +return -EAGAIN ;memset (&c,0 ,sizeof (c)); c.common.opcode = cmd.opcode;
Signature · 1 line
2.13.1
PATCH 1/1 nvme: block ioctls if controller not in a live state
Quoted 5 lines · James Smart
Rather than allow ioctl-based admin cmds to get intermixed on the admin queue with commands being used to initialize a controller or io commands to go to a controller in reconnect thus possibly hanging, reject them if the controller isn't in the LIVE state. Reject with an
-EAGAINstatus so that the app knows it could retry.
Shouldn't the nvmf_check_if_read() calls in the transport drivers' ->queue_rq() callouts handle this?
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 1/1 nvme: block ioctls if controller not in a live state
Quoted 10 lines · Johannes Thumshirn
Quoted 5 lines · James Smart
Rather than allow ioctl-based admin cmds to get intermixed on the admin queue with commands being used to initialize a controller or io commands to go to a controller in reconnect thus possibly hanging, reject them if the controller isn't in the LIVE state. Reject with an
-EAGAINstatus so that the app knows it could retry.Shouldn't the
nvmf_check_if_read()calls in the transport drivers' ->queue_rq() callouts handle this?
yes - but if things reach that layer, the return status for the ioctl can be quite different. (if I read things right) The core routines are coded so that if there's a hard error, a -Exxx status will be returned. Otherwise, it returns the nvme_req(req)->status value (assumed positive big value). Which means, if the if_ready checks reject it, it'll go back with an NVME_SC_ABORT_REQ status. Which means the cli gets a convoluted error response. For the cases where it would be blocked by controller state, I want an -EAGAIN status so that the app knows it can retry and it should succeed sometime in the future. Any other status - the app gives up and accepts the status value.
-- james
PATCH 1/1 nvme: block ioctls if controller not in a live state
Quoted 10 lines · James Smart
yes - but if things reach that layer, the return status for the ioctl can be quite different. (if I read things right) The core routines are coded so that if there's a hard error, a -Exxx status will be returned. Otherwise, it returns the nvme_req(req)->status value (assumed positive big value). Which means, if the
if_readychecks reject it, it'll go back with anNVME_SC_ABORT_REQstatus. Which means the cli gets a convoluted error response. For the cases where it would be blocked by controller state, I want an-EAGAINstatus so that the app knows it can retry and it should succeed sometime in the future. Any other status - the app gives up and accepts the status value.
Ah OK, thanks for the clarification.
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 1/1 nvme: block ioctls if controller not in a live state
In reply to James SmartQuoted 5 lines · James Smart
Rather than allow ioctl-based admin cmds to get intermixed on the admin queue with commands being used to initialize a controller or io commands to go to a controller in reconnect thus possibly hanging, reject them if the controller isn't in the LIVE state. Reject with an
-EAGAINstatus so that the app knows it could retry.
-EAGAIN implies that you can poll for the fd to be ready, so I think this is the wrong error code here.
PATCH 1/1 nvme: block ioctls if controller not in a live state
Quoted 8 lines · Christoph Hellwig
Quoted 5 lines · James Smart
Rather than allow ioctl-based admin cmds to get intermixed on the admin queue with commands being used to initialize a controller or io commands to go to a controller in reconnect thus possibly hanging, reject them if the controller isn't in the LIVE state. Reject with an
-EAGAINstatus so that the app knows it could retry.
-EAGAINimplies that you can poll for the fd to be ready, so I think this is the wrong error code here.
That is the exact intent.? I do want the app to trigger off -EAGAIN in those two states and try again after a delay./? Perhaps I should change from !LIVE to NEW || RESETTING || CONNECTING to be more explicit.? I assumed !LIVE was sufficient as the other states should transition to LIVE or the delete path is taken which would remove the fd all together.
-- james
PATCH 1/1 nvme: block ioctls if controller not in a live state
Quoted 6 lines · James Smart
That is the exact intent.? I do want the app to trigger off
-EAGAINin those two states and try again after a delay./? Perhaps I should change from !LIVE to NEW || RESETTING || CONNECTING to be more explicit.? I assumed !LIVE was sufficient as the other states should transition to LIVE or the delete path is taken which would remove the fd all together.
The problem is that userspace reasonably expects to be able to call poll/select when used on a character device and it gets -EAGAIN.
Maybe the right solution here is to just do a wait_event_interruptible to wait for the controller being live.
PATCH 1/1 nvme: block ioctls if controller not in a live state
Quoted 8 lines · Christoph Hellwig
Quoted 5 lines · James Smart
That is the exact intent.? I do want the app to trigger off
-EAGAINin those two states and try again after a delay./? Perhaps I should change from !LIVE to NEW || RESETTING || CONNECTING to be more explicit.? I assumed !LIVE was sufficient as the other states should transition to LIVE or the delete path is taken which would remove the fd all together.The problem is that userspace reasonably expects to be able to call poll/select when used on a character device and it gets
-EAGAIN.
What's stopping us from adding poll support to the dev node ?
Quoted 3 lines
Maybe the right solution here is to just do a
wait_event_interruptibleto wait for the controller being live.
That's doable. I don't like hanging the app up for what could be 60s or so though (controller loss timeout).
--james
PATCH 1/1 nvme: block ioctls if controller not in a live state
Quoted 9 lines · James Smart
Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · James Smart
That is the exact intent.? I do want the app to trigger off
-EAGAINin those two states and try again after a delay./? Perhaps I should change from !LIVE to NEW || RESETTING || CONNECTING to be more explicit.? I assumed !LIVE was sufficient as the other states should transition to LIVE or the delete path is taken which would remove the fd all together.
Quoted 4 lines
Quoted 2 lines
The problem is that userspace reasonably expects to be able to call poll/select when used on a character device and it gets
-EAGAIN.What's stopping us from adding poll support to the dev node ?
A volunteer to write the code.
Quoted 7 lines
Quoted 4 lines
Maybe the right solution here is to just do a
wait_event_interruptibleto wait for the controller being live.That's doable. I don't like hanging the app up for what could be 60s or so though (controller loss timeout).
Then you need to poll. We should only do that if opened with O_NONBLOCK and otherwise wait for the event, similar to the pattern used in a lot of character devices.
PATCH 1/1 nvme: block ioctls if controller not in a live state
Quoted 14 lines · Christoph Hellwig
Quoted 11 lines · James Smart
Quoted 8 lines · Christoph Hellwig
Quoted 5 lines · James Smart
That is the exact intent.? I do want the app to trigger off
-EAGAINin those two states and try again after a delay./? Perhaps I should change from !LIVE to NEW || RESETTING || CONNECTING to be more explicit.? I assumed !LIVE was sufficient as the other states should transition to LIVE or the delete path is taken which would remove the fd all together.The problem is that userspace reasonably expects to be able to call poll/select when used on a character device and it gets
-EAGAIN.What's stopping us from adding poll support to the dev node ?
A volunteer to write the code.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Maybe the right solution here is to just do a
wait_event_interruptibleto wait for the controller being live.That's doable. I don't like hanging the app up for what could be 60s or so though (controller loss timeout).
Then you need to poll. We should only do that if opened with
O_NONBLOCKand otherwise wait for the event, similar to the pattern used in a lot of character devices.
Christoph,
I was going to address this by adding the poll support.?? However, after reviewing the if_ready changes, they will fall into the USERCMD cases which will return the BLK_STST_IOERR and NVME_SC_ABORT_REQ.
In the nvme cli app, rather than keying retry off the EAGAIN, we could key off the ABORT_REQ status. I didn't like that initially as it may correspond to a real scenario that wasn't one of the reject-as-not-in-a-state-for-an-ioctl cases.
What do you think - continue with the poll interface and the EAGAIN block status, or revise the app EAGAIN check to NVME_SC_ABORT_REQ ?
-- james
PATCH 1/1 nvme: block ioctls if controller not in a live state
Quoted 11 lines · James Smart
I was going to address this by adding the poll support.?? However, after reviewing the
if_readychanges, they will fall into the USERCMD cases which will return theBLK_STST_IOERRandNVME_SC_ABORT_REQ.In the nvme cli app, rather than keying retry off the EAGAIN, we could key off the
ABORT_REQstatus. I didn't like that initially as it may correspond to a real scenario that wasn't one of the reject-as-not-in-a-state-for-an-ioctl cases.What do you think - continue with the poll interface and the EAGAIN block status, or revise the app EAGAIN check to
NVME_SC_ABORT_REQ?
How about just having an option in the ioctl interface to not mark the I/O failfast?
PATCH 1/1 nvme: block ioctls if controller not in a live state
Quoted 10 lines · Christoph Hellwig
Quoted 9 lines · James Smart
I was going to address this by adding the poll support.?? However, after reviewing the
if_readychanges, they will fall into the USERCMD cases which will return theBLK_STST_IOERRandNVME_SC_ABORT_REQ.In the nvme cli app, rather than keying retry off the EAGAIN, we could key off the
ABORT_REQstatus. I didn't like that initially as it may correspond to a real scenario that wasn't one of the reject-as-not-in-a-state-for-an-ioctl cases.
Quoted 6 lines
Quoted 2 lines
What do you think - continue with the poll interface and the EAGAIN block status, or revise the app EAGAIN check to
NVME_SC_ABORT_REQ?How about just having an option in the ioctl interface to not mark the I/O failfast?
with the gist being you want to suspend the ioctl ?
I've already seen enough user feedback to say the ioctl shouldn't hang. So I agree with them being marked failfast.
-- james
PATCH 1/1 nvme: block ioctls if controller not in a live state
In reply to James SmartQuoted 5 lines · James Smart
Rather than allow ioctl-based admin cmds to get intermixed on the admin queue with commands being used to initialize a controller or io commands to go to a controller in reconnect thus possibly hanging, reject them if the controller isn't in the LIVE state. Reject with an
-EAGAINstatus so that the app knows it could retry.
The admin ioctl is fine to use in ADMIN_ONLY state as well.
PATCH 1/1 nvme: block ioctls if controller not in a live state
Quoted 9 lines · Keith Busch
Quoted 5 lines · James Smart
Rather than allow ioctl-based admin cmds to get intermixed on the admin queue with commands being used to initialize a controller or io commands to go to a controller in reconnect thus possibly hanging, reject them if the controller isn't in the LIVE state. Reject with an
-EAGAINstatus so that the app knows it could retry.The admin ioctl is fine to use in
ADMIN_ONLYstate as well.
Yep, I would need to take that state into account. For now, looks like I'll be adding poll support and reposting.
-- james