[PATCH] nvme: honour O_NONBLOCK during resetting

[PATCH] nvme: honour O_NONBLOCK during resetting

[Hannes Reinecke]

When opening a controller device node we should honour the O_NONBLOCK
flag to allow the device to be openend even if it's in state 'resetting'
or 'connecting'. This allows user-space applications to use a call to 'open'
to figure out if the controller is present, even if it's currently
undergoing a reset.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 74980a1cf89e..67cde3eaf5b0 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3136,6 +3136,11 @@ static int nvme_dev_open(struct inode *inode, struct file *file)
 	switch (ctrl->state) {
 	case NVME_CTRL_LIVE:
 		break;
+	case NVME_CTRL_RESETTING:
+	case NVME_CTRL_CONNECTING:
+		if (file->f_flags & O_NONBLOCK)
+			break;
+		fallthrough;
 	default:
 		return -EWOULDBLOCK;
 	}
-- 
2.29.2

Re: [PATCH] nvme: honour O_NONBLOCK during resetting

[Chaitanya Kulkarni]

On 11/11/2021 2:59 AM, Hannes Reinecke wrote:
> When opening a controller device node we should honour the O_NONBLOCK
> flag to allow the device to be openend even if it's in state 'resetting'
> or 'connecting'. This allows user-space applications to use a call to 'open'
> to figure out if the controller is present, even if it's currently
> undergoing a reset.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Will resetting and connecting ever result is deleting the controller due
to error cases present in that path ?

If yes then application will have handle for something that might
go away in the future, should allow such a semantic ?

-ck

Re: [PATCH] nvme: honour O_NONBLOCK during resetting

[Keith Busch]

On Tue, Nov 16, 2021 at 01:40:04AM +0000, Chaitanya Kulkarni wrote:
> On 11/11/2021 2:59 AM, Hannes Reinecke wrote:
> > When opening a controller device node we should honour the O_NONBLOCK
> > flag to allow the device to be openend even if it's in state 'resetting'
> > or 'connecting'. This allows user-space applications to use a call to 'open'
> > to figure out if the controller is present, even if it's currently
> > undergoing a reset.
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> 
> Will resetting and connecting ever result is deleting the controller due
> to error cases present in that path ?
> 
> If yes then application will have handle for something that might
> go away in the future, should allow such a semantic ?

Resets can happen at any time, so they already have to handle it even if the
controller was live when they opened it. Perhaps the open should succeed for
any non-terminal state.

Re: [PATCH] nvme: honour O_NONBLOCK during resetting

[Hannes Reinecke]

On 11/16/21 2:40 AM, Chaitanya Kulkarni wrote:
> On 11/11/2021 2:59 AM, Hannes Reinecke wrote:
>> When opening a controller device node we should honour the O_NONBLOCK
>> flag to allow the device to be openend even if it's in state 'resetting'
>> or 'connecting'. This allows user-space applications to use a call to 'open'
>> to figure out if the controller is present, even if it's currently
>> undergoing a reset.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
> 
> Will resetting and connecting ever result is deleting the controller due
> to error cases present in that path ?
> 
Yes, of course this might happen.

> If yes then application will have handle for something that might
> go away in the future, should allow such a semantic ?
> I am not sure I follow.
Even today an application can open the device, and even now error 
recovery can result in the device now going away.
And even today it'll be possible that error recovery sets the device
to 'RESETTING' after the open call.

So I don't think that we need to treat things special because of this patch.

Whether or not the call to nvme_dev_ioctl() should be protected by an 
explicit state check is a different point, though; that does have some 
benefits seeing that the timing between open() and ioctl() is pretty 
much up to the application, and hence we shouldn't assume that the state 
doesn't change in between.
But really it should be done in a different patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH] nvme: honour O_NONBLOCK during resetting

[Hannes Reinecke]

On 11/16/21 3:18 AM, Keith Busch wrote:
> On Tue, Nov 16, 2021 at 01:40:04AM +0000, Chaitanya Kulkarni wrote:
>> On 11/11/2021 2:59 AM, Hannes Reinecke wrote:
>>> When opening a controller device node we should honour the O_NONBLOCK
>>> flag to allow the device to be openend even if it's in state 'resetting'
>>> or 'connecting'. This allows user-space applications to use a call to 'open'
>>> to figure out if the controller is present, even if it's currently
>>> undergoing a reset.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>
>> Will resetting and connecting ever result is deleting the controller due
>> to error cases present in that path ?
>>
>> If yes then application will have handle for something that might
>> go away in the future, should allow such a semantic ?
> 
> Resets can happen at any time, so they already have to handle it even if the
> controller was live when they opened it. Perhaps the open should succeed for
> any non-terminal state.
> 
Indeed. And we should add a state check for the ioctl itself, returning 
an error when the device is not running.

I'll be sending an updated patchset.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer