Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs

Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs

[Niklas Cassel]

On Mon, Nov 02, 2020 at 04:15:01PM +0000, Javier Gonzalez wrote:
> 
> From: Keith Busch <kbusch@kernel.org>
> Sent: Nov 2, 2020 16:45
> To: Javier González <javier@javigon.com>
> Cc: linux-nvme@lists.infradead.org; linux-block@vger.kernel.org; hch@lst.de; sagi@grimberg.me; axboe@kernel.dk; joshi.k@samsung.com; "Klaus B. Jensen" <k.jensen@samsung.com>; Niklas.Cassel@wdc.com; Javier Gonzalez <javier.gonz@samsung.com>
> Subject: Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
> 
> On Mon, Nov 02, 2020 at 02:22:14PM +0100, Javier González wrote:
> > Changes since V1:
> >    - Apply feedback from Niklas:
> >        - Use IS_ENABLED() for checking config option
> 
> Your v1 was correct. Using IS_ENBALED() attempts to use an undefined
> symbol when the CONFIG is not enabled:
> 
> Oh. Ok. Will return to that.

Keith is correct, sorry for that.

https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation

"Thus, you still have to use an #ifdef if the code inside the block
references symbols that will not exist if the condition is not met."

Kind regards,
Niklas

> 
>   drivers/nvme/host/core.c: In function ‘nvme_update_disk_info’:
>   drivers/nvme/host/core.c:2056:45: error: ‘struct nvme_ns’ has no member named ‘zoned_ns_supp’
>    2056 |  if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && !ns->zoned_ns_supp)
>         |                                             ^~
> 
> That said, I don't mind the concept, though I recall Christoph had
> concerns about the existing 0-capacity namespace used for invalid
> formats, so I'd like to hear more on that if he has some spare time to
> comment.
> 
> Sounds good. I'll respin a V3 in the meantime. Assuming not supported in the beginning gives problems with non zone namespaces either way. So I'll fix that too.
> 
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs

[hch]

On Mon, Nov 02, 2020 at 04:15:01PM +0000, Javier Gonzalez wrote:
> That said, I don't mind the concept, though I recall Christoph had
> concerns about the existing 0-capacity namespace used for invalid
> formats, so I'd like to hear more on that if he has some spare time to
> comment.

Yes, I really don't think 0 sized block devices are the right interface
for namespaces we can't access.  I think the proper fix is to ensure that
we have a character device that allows submitting I/O commands for each
namespaces including those where we don't understand the I/O command set
or parts of it.  That should also really help to have a proper access
model for the KV command set.  Initially we only need NVME_IOCTL_IO64_CMD,
but eventually we'll need some support for async submissions, be
that through io_uring or other ways.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs

[Keith Busch]

On Mon, Nov 02, 2020 at 07:08:37PM +0100, hch@lst.de wrote:
> On Mon, Nov 02, 2020 at 04:15:01PM +0000, Javier Gonzalez wrote:
> > That said, I don't mind the concept, though I recall Christoph had
> > concerns about the existing 0-capacity namespace used for invalid
> > formats, so I'd like to hear more on that if he has some spare time to
> > comment.
> 
> Yes, I really don't think 0 sized block devices are the right interface
> for namespaces we can't access.  I think the proper fix is to ensure that
> we have a character device that allows submitting I/O commands for each
> namespaces including those where we don't understand the I/O command set
> or parts of it.  That should also really help to have a proper access
> model for the KV command set.  Initially we only need NVME_IOCTL_IO64_CMD,
> but eventually we'll need some support for async submissions, be
> that through io_uring or other ways.

I can see this going one of two ways:

 a) Set up the existing controller character device with a generic
    disk-less request_queue to the IO queues accepting IO commands to
    arbitrary NSIDs.

 b) Each namespace that can't be supported gets their own character
    device.

I'm leaning toward option "a". While it doesn't create handles to unique
namespaces, it has more resilience to potentially future changes. And I
recall the target side had a potential use for that, too.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs

[hch]

On Mon, Nov 02, 2020 at 10:33:55AM -0800, Keith Busch wrote:
> I can see this going one of two ways:
> 
>  a) Set up the existing controller character device with a generic
>     disk-less request_queue to the IO queues accepting IO commands to
>     arbitrary NSIDs.
> 
>  b) Each namespace that can't be supported gets their own character
>     device.
> 
> I'm leaning toward option "a". While it doesn't create handles to unique
> namespaces, it has more resilience to potentially future changes. And I
> recall the target side had a potential use for that, too.

The problem with a) is that it can't be used to give users or groups
access to just one namespaces, so it causes a real access control
nightmare.  The problem with b) is that now applications will break
when we add support for new command sets or features.  I think

  c) Each namespace gets its own character device, period.

is the only sensible option.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: nvme: report capacity 0 for non supported ZNS SSDs

[Javier Gonzalez]

On 02.11.2020 19:58, hch@lst.de wrote:
>On Mon, Nov 02, 2020 at 10:33:55AM -0800, Keith Busch wrote:
>> I can see this going one of two ways:
>>
>>  a) Set up the existing controller character device with a generic
>>     disk-less request_queue to the IO queues accepting IO commands to
>>     arbitrary NSIDs.
>>
>>  b) Each namespace that can't be supported gets their own character
>>     device.
>>
>> I'm leaning toward option "a". While it doesn't create handles to unique
>> namespaces, it has more resilience to potentially future changes. And I
>> recall the target side had a potential use for that, too.
>
>The problem with a) is that it can't be used to give users or groups
>access to just one namespaces, so it causes a real access control
>nightmare.  The problem with b) is that now applications will break
>when we add support for new command sets or features.  I think
>
>  c) Each namespace gets its own character device, period.
>
>is the only sensible option.

This sounds reasonable. We have been back and forth on a patchset with
async passthru using io_uring. I believe this can help with some of the
questions we are preparing for a RFC.

If I understand correctly, the model would be that a namespace will
always have (i) a character device associated where I/O is always allowed
through user formed commands, and if the namespace has full support in
the kernel (ii) a block device where I/O is as it is today. In case of
(ii) both interfaces can be used for I/O.

While we work on iterations for c), do you believe it is reasonable to
merge a version of the current path that follows the PI convention for
unsupported command sets and features? I would assume that we will have
to convert PI to this model too when it is available.

Javier

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: nvme: report capacity 0 for non supported ZNS SSDs

[Javier Gonzalez]

On 02.11.2020 16:30, Niklas Cassel wrote:
>On Mon, Nov 02, 2020 at 04:15:01PM +0000, Javier Gonzalez wrote:
>>
>> From: Keith Busch <kbusch@kernel.org>
>> Sent: Nov 2, 2020 16:45
>> To: Javier González <javier@javigon.com>
>> Cc: linux-nvme@lists.infradead.org; linux-block@vger.kernel.org; hch@lst.de; sagi@grimberg.me; axboe@kernel.dk; joshi.k@samsung.com; "Klaus B. Jensen" <k.jensen@samsung.com>; Niklas.Cassel@wdc.com; Javier Gonzalez <javier.gonz@samsung.com>
>> Subject: Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
>>
>> On Mon, Nov 02, 2020 at 02:22:14PM +0100, Javier González wrote:
>> > Changes since V1:
>> >    - Apply feedback from Niklas:
>> >        - Use IS_ENABLED() for checking config option
>>
>> Your v1 was correct. Using IS_ENBALED() attempts to use an undefined
>> symbol when the CONFIG is not enabled:
>>
>> Oh. Ok. Will return to that.
>
>Keith is correct, sorry for that.
>
>https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation
>
>"Thus, you still have to use an #ifdef if the code inside the block
>references symbols that will not exist if the condition is not met."
>

No worries. Thanks for pointing this out - I had seen code with
IS_ENABLED(), but I had not done the necessary reading to determine if
it was something I should use or not. Now I know :)

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: nvme: report capacity 0 for non supported ZNS SSDs

[hch]

On Mon, Nov 02, 2020 at 10:24:05PM +0100, Javier Gonzalez wrote:
> If I understand correctly, the model would be that a namespace will
> always have (i) a character device associated where I/O is always allowed
> through user formed commands, and if the namespace has full support in
> the kernel (ii) a block device where I/O is as it is today. In case of
> (ii) both interfaces can be used for I/O.

Yes.

> While we work on iterations for c), do you believe it is reasonable to
> merge a version of the current path that follows the PI convention for
> unsupported command sets and features? I would assume that we will have
> to convert PI to this model too when it is available.

I'm rather torn.  I think the model of the zero capacity block device
is a really, really bad one and we should haver never added it.  That
being said, for a ZNS namespace that does not support Zone Append I
can think of a model that actually makes sense:  expose it as a read-only
block device, as we can actually read from it perfectly fine, and that
would also allow ioctl access.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: nvme: report capacity 0 for non supported ZNS SSDs

[Javier Gonzalez]

On 03.11.2020 10:06, hch@lst.de wrote:
>On Mon, Nov 02, 2020 at 10:24:05PM +0100, Javier Gonzalez wrote:
>> If I understand correctly, the model would be that a namespace will
>> always have (i) a character device associated where I/O is always allowed
>> through user formed commands, and if the namespace has full support in
>> the kernel (ii) a block device where I/O is as it is today. In case of
>> (ii) both interfaces can be used for I/O.
>
>Yes.

Thanks for confirming.

One question here is that we are preparing a RFC for a io_uring passthru
using the block device. Based on this discussion, it seems to me that
you see this more suitable through the char device.

Does it make sense that we post this RFC using the block device? It
would be helpful to get early feedback before starting the char device.

>
>> While we work on iterations for c), do you believe it is reasonable to
>> merge a version of the current path that follows the PI convention for
>> unsupported command sets and features? I would assume that we will have
>> to convert PI to this model too when it is available.
>
>I'm rather torn.  I think the model of the zero capacity block device
>is a really, really bad one and we should haver never added it.  That
>being said, for a ZNS namespace that does not support Zone Append I
>can think of a model that actually makes sense:  expose it as a read-only
>block device, as we can actually read from it perfectly fine, and that
>would also allow ioctl access.

This is reasonable. I can re-spin this for append to become read-only
and then we work in parallel for the char device interface.

I see that this does not make much sense for the other non-supported
features in this patch (i.e., !po2 zone size and zoc). Since this is
very much like PI today, is it OK we add these the same way (capacity 0)
and then move to the char device when ready?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: nvme: report capacity 0 for non supported ZNS SSDs

[hch]

On Tue, Nov 03, 2020 at 03:10:19PM +0100, Javier Gonzalez wrote:
> One question here is that we are preparing a RFC for a io_uring passthru
> using the block device. Based on this discussion, it seems to me that
> you see this more suitable through the char device.
>
> Does it make sense that we post this RFC using the block device? It
> would be helpful to get early feedback before starting the char device.

If you wan to send the RFC with the block device that is ok.  But I
think the separate character device is pretty trivial, at least for
NVM command set derived command sets like ZNS (for others we'll need
to finish the Command Set Independent Identify Namespace TP first).

> I see that this does not make much sense for the other non-supported
> features in this patch (i.e., !po2 zone size and zoc). Since this is
> very much like PI today, is it OK we add these the same way (capacity 0)
> and then move to the char device when ready?

I'd rath avoid adding more of that capacity 0 mess if we can.  Especially
as the character device should be really easy to do.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: nvme: report capacity 0 for non supported ZNS SSDs

[Javier Gonzalez]

On 03.11.2020 16:26, hch@lst.de wrote:
>On Tue, Nov 03, 2020 at 03:10:19PM +0100, Javier Gonzalez wrote:
>> One question here is that we are preparing a RFC for a io_uring passthru
>> using the block device. Based on this discussion, it seems to me that
>> you see this more suitable through the char device.
>>
>> Does it make sense that we post this RFC using the block device? It
>> would be helpful to get early feedback before starting the char device.
>
>If you wan to send the RFC with the block device that is ok.  But I
>think the separate character device is pretty trivial, at least for
>NVM command set derived command sets like ZNS (for others we'll need
>to finish the Command Set Independent Identify Namespace TP first).

Ok. Good to hear that we can do this in steps - I was worried that we
would need to cover this use case too in the beginning, which would
delay this work significantly.

>
>> I see that this does not make much sense for the other non-supported
>> features in this patch (i.e., !po2 zone size and zoc). Since this is
>> very much like PI today, is it OK we add these the same way (capacity 0)
>> and then move to the char device when ready?
>
>I'd rath avoid adding more of that capacity 0 mess if we can.  Especially
>as the character device should be really easy to do.

Ok. We will move ahead with the char device and port current capacity 0
there in the series.

Thanks for the guidance Christoph!

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs

[Hannes Reinecke]

On 11/2/20 7:58 PM, hch@lst.de wrote:
> On Mon, Nov 02, 2020 at 10:33:55AM -0800, Keith Busch wrote:
>> I can see this going one of two ways:
>>
>>   a) Set up the existing controller character device with a generic
>>      disk-less request_queue to the IO queues accepting IO commands to
>>      arbitrary NSIDs.
>>
>>   b) Each namespace that can't be supported gets their own character
>>      device.
>>
>> I'm leaning toward option "a". While it doesn't create handles to unique
>> namespaces, it has more resilience to potentially future changes. And I
>> recall the target side had a potential use for that, too.
> 
> The problem with a) is that it can't be used to give users or groups
> access to just one namespaces, so it causes a real access control
> nightmare.  The problem with b) is that now applications will break
> when we add support for new command sets or features.  I think
> 
>    c) Each namespace gets its own character device, period.
> 
> is the only sensible option.
> 
I hardly dare to mention bsg here; but the is pretty similar to what it 
set out to do ...

Or yet another interface?

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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs

[hch]

On Wed, Nov 04, 2020 at 03:26:46PM +0100, Hannes Reinecke wrote:
> I hardly dare to mention bsg here; but the is pretty similar to what it set 
> out to do ...

Except that:

 a) we created a complete mess with bsg by overloading the scsi ioctls
    with some of the transport stuff.  
 b) bsg would not work with existing tools.  A character device that
    accepts the same ioctl will just work.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs

[Hannes Reinecke]

On 11/4/20 3:29 PM, hch@lst.de wrote:
> On Wed, Nov 04, 2020 at 03:26:46PM +0100, Hannes Reinecke wrote:
>> I hardly dare to mention bsg here; but the is pretty similar to what it set
>> out to do ...
> 
> Except that:
> 
>   a) we created a complete mess with bsg by overloading the scsi ioctls
>      with some of the transport stuff.
>   b) bsg would not work with existing tools.  A character device that
>      accepts the same ioctl will just work.
> 
... as would a bsg device which could accept said ioctl ...

Plus it feels a bit weird, having a generic command passthrough 
character device which is then avoided in favour of a protocol-specific 
command passthrough device.
Which we had been arguing for years with IHVs for _not_ doing it.
So what is different here?

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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs

[hch]

On Wed, Nov 04, 2020 at 03:46:02PM +0100, Hannes Reinecke wrote:
> ... as would a bsg device which could accept said ioctl ...

Sure we could.  So we'd have to add more code to almost 1000 lines of
code in bsg that are not useful to the nvme use case to make it useful
for that use case.  Or we could just add about 50 lines of code to NVMe
to do the right thing.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs

[Hannes Reinecke]

On 11/5/20 8:37 AM, hch@lst.de wrote:
> On Wed, Nov 04, 2020 at 03:46:02PM +0100, Hannes Reinecke wrote:
>> ... as would a bsg device which could accept said ioctl ...
> 
> Sure we could.  So we'd have to add more code to almost 1000 lines of
> code in bsg that are not useful to the nvme use case to make it useful
> for that use case.  Or we could just add about 50 lines of code to NVMe
> to do the right thing.
> My point wasn't so much that it'd be easier to code.
Just wanted to point out how we've argued in the past.

But anyway: you are the maintainer, you get to decide.

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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme