* Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
[not found] ` <0916865d50c640e3aa95dc542f3986b9@CAMSVWEXC01.scsc.local>
@ 2020-11-02 16:30 ` Niklas Cassel
2020-11-02 21:25 ` Javier Gonzalez
2020-11-02 18:08 ` [PATCH V2] " hch
1 sibling, 1 reply; 17+ messages in thread
From: Niklas Cassel @ 2020-11-02 16:30 UTC (permalink / raw)
To: Javier Gonzalez
Cc: kbusch, javier, linux-nvme, linux-block, hch, sagi, axboe,
joshi.k, Klaus B. Jensen
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.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvme: report capacity 0 for non supported ZNS SSDs
2020-11-02 16:30 ` [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs Niklas Cassel
@ 2020-11-02 21:25 ` Javier Gonzalez
0 siblings, 0 replies; 17+ messages in thread
From: Javier Gonzalez @ 2020-11-02 21:25 UTC (permalink / raw)
To: Niklas Cassel
Cc: kbusch, linux-nvme, linux-block, hch, sagi, axboe, joshi.k,
Klaus B. Jensen
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 :)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
[not found] ` <0916865d50c640e3aa95dc542f3986b9@CAMSVWEXC01.scsc.local>
2020-11-02 16:30 ` [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs Niklas Cassel
@ 2020-11-02 18:08 ` hch
2020-11-02 18:33 ` Keith Busch
1 sibling, 1 reply; 17+ messages in thread
From: hch @ 2020-11-02 18:08 UTC (permalink / raw)
To: Javier Gonzalez
Cc: kbusch, javier, linux-nvme, linux-block, hch, sagi, axboe,
joshi.k, Klaus B. Jensen, Niklas.Cassel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
2020-11-02 18:08 ` [PATCH V2] " hch
@ 2020-11-02 18:33 ` Keith Busch
2020-11-02 18:58 ` hch
0 siblings, 1 reply; 17+ messages in thread
From: Keith Busch @ 2020-11-02 18:33 UTC (permalink / raw)
To: hch
Cc: Javier Gonzalez, javier, linux-nvme, linux-block, sagi, axboe,
joshi.k, Klaus B. Jensen, Niklas.Cassel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
2020-11-02 18:33 ` Keith Busch
@ 2020-11-02 18:58 ` hch
2020-11-02 21:24 ` Javier Gonzalez
2020-11-04 14:26 ` [PATCH V2] " Hannes Reinecke
0 siblings, 2 replies; 17+ messages in thread
From: hch @ 2020-11-02 18:58 UTC (permalink / raw)
To: Keith Busch
Cc: hch, Javier Gonzalez, javier, linux-nvme, linux-block, sagi,
axboe, joshi.k, Klaus B. Jensen, Niklas.Cassel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvme: report capacity 0 for non supported ZNS SSDs
2020-11-02 18:58 ` hch
@ 2020-11-02 21:24 ` Javier Gonzalez
2020-11-03 9:06 ` hch
2020-11-04 14:26 ` [PATCH V2] " Hannes Reinecke
1 sibling, 1 reply; 17+ messages in thread
From: Javier Gonzalez @ 2020-11-02 21:24 UTC (permalink / raw)
To: hch
Cc: Keith Busch, linux-nvme, linux-block, sagi, axboe, joshi.k,
Klaus B. Jensen, Niklas.Cassel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvme: report capacity 0 for non supported ZNS SSDs
2020-11-02 21:24 ` Javier Gonzalez
@ 2020-11-03 9:06 ` hch
2020-11-03 14:10 ` Javier Gonzalez
0 siblings, 1 reply; 17+ messages in thread
From: hch @ 2020-11-03 9:06 UTC (permalink / raw)
To: Javier Gonzalez
Cc: hch, Keith Busch, linux-nvme, linux-block, sagi, axboe, joshi.k,
Klaus B. Jensen, Niklas.Cassel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvme: report capacity 0 for non supported ZNS SSDs
2020-11-03 9:06 ` hch
@ 2020-11-03 14:10 ` Javier Gonzalez
2020-11-03 15:26 ` hch
0 siblings, 1 reply; 17+ messages in thread
From: Javier Gonzalez @ 2020-11-03 14:10 UTC (permalink / raw)
To: hch
Cc: Keith Busch, linux-nvme, linux-block, sagi, axboe, joshi.k,
Klaus B. Jensen, Niklas.Cassel
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?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvme: report capacity 0 for non supported ZNS SSDs
2020-11-03 14:10 ` Javier Gonzalez
@ 2020-11-03 15:26 ` hch
2020-11-03 15:54 ` Javier Gonzalez
0 siblings, 1 reply; 17+ messages in thread
From: hch @ 2020-11-03 15:26 UTC (permalink / raw)
To: Javier Gonzalez
Cc: hch, Keith Busch, linux-nvme, linux-block, sagi, axboe, joshi.k,
Klaus B. Jensen, Niklas.Cassel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvme: report capacity 0 for non supported ZNS SSDs
2020-11-03 15:26 ` hch
@ 2020-11-03 15:54 ` Javier Gonzalez
0 siblings, 0 replies; 17+ messages in thread
From: Javier Gonzalez @ 2020-11-03 15:54 UTC (permalink / raw)
To: hch
Cc: Keith Busch, linux-nvme, linux-block, sagi, axboe, joshi.k,
Klaus B. Jensen, Niklas.Cassel
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!
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
2020-11-02 18:58 ` hch
2020-11-02 21:24 ` Javier Gonzalez
@ 2020-11-04 14:26 ` Hannes Reinecke
2020-11-04 14:29 ` hch
1 sibling, 1 reply; 17+ messages in thread
From: Hannes Reinecke @ 2020-11-04 14:26 UTC (permalink / raw)
To: hch, Keith Busch
Cc: Javier Gonzalez, javier, linux-nvme, linux-block, sagi, axboe,
joshi.k, Klaus B. Jensen, Niklas.Cassel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
2020-11-04 14:26 ` [PATCH V2] " Hannes Reinecke
@ 2020-11-04 14:29 ` hch
2020-11-04 14:46 ` Hannes Reinecke
0 siblings, 1 reply; 17+ messages in thread
From: hch @ 2020-11-04 14:29 UTC (permalink / raw)
To: Hannes Reinecke
Cc: hch, Keith Busch, Javier Gonzalez, javier, linux-nvme,
linux-block, sagi, axboe, joshi.k, Klaus B. Jensen,
Niklas.Cassel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
2020-11-04 14:29 ` hch
@ 2020-11-04 14:46 ` Hannes Reinecke
2020-11-05 7:37 ` hch
0 siblings, 1 reply; 17+ messages in thread
From: Hannes Reinecke @ 2020-11-04 14:46 UTC (permalink / raw)
To: hch
Cc: Keith Busch, Javier Gonzalez, javier, linux-nvme, linux-block,
sagi, axboe, joshi.k, Klaus B. Jensen, Niklas.Cassel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
2020-11-04 14:46 ` Hannes Reinecke
@ 2020-11-05 7:37 ` hch
2020-11-05 7:42 ` Hannes Reinecke
0 siblings, 1 reply; 17+ messages in thread
From: hch @ 2020-11-05 7:37 UTC (permalink / raw)
To: Hannes Reinecke
Cc: hch, Keith Busch, Javier Gonzalez, javier, linux-nvme,
linux-block, sagi, axboe, joshi.k, Klaus B. Jensen,
Niklas.Cassel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
2020-11-05 7:37 ` hch
@ 2020-11-05 7:42 ` Hannes Reinecke
0 siblings, 0 replies; 17+ messages in thread
From: Hannes Reinecke @ 2020-11-05 7:42 UTC (permalink / raw)
To: hch
Cc: Keith Busch, Javier Gonzalez, javier, linux-nvme, linux-block,
sagi, axboe, joshi.k, Klaus B. Jensen, Niklas.Cassel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
@ 2020-11-02 13:22 Javier González
2020-11-02 15:44 ` Keith Busch
0 siblings, 1 reply; 17+ messages in thread
From: Javier González @ 2020-11-02 13:22 UTC (permalink / raw)
To: linux-nvme
Cc: linux-block, hch, kbusch, sagi, axboe, joshi.k, k.jensen,
Niklas.Cassel, Javier González
Allow ZNS SSDs to be presented to the host even when they implement
features that are not supported by the kernel zoned block device.
Instead of rejecting the SSD at the NVMe driver level, deal with this in
the block layer by setting capacity to 0, as we do with other things
such as unsupported PI configurations. This allows to use standard
management tools such as nvme-cli to choose a different format or
firmware slot that is compatible with the Linux zoned block device.
Changes since V1:
- Apply feedback from Niklas:
- Use IS_ENABLED() for checking config option
- Use local variable
- Use different variable names
Signed-off-by: Javier González <javier.gonz@samsung.com>
---
drivers/nvme/host/core.c | 3 +++
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/host/zns.c | 26 ++++++++++++++------------
3 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c190c56bf702..638997b6f5cd 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2026,6 +2026,9 @@ static void nvme_update_disk_info(struct gendisk *disk,
capacity = 0;
}
+ if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && !ns->zoned_ns_supp)
+ capacity = 0;
+
set_capacity_revalidate_and_notify(disk, capacity, false);
nvme_config_discard(disk, ns);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 87737fa32360..ff4fe645ab9b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -443,6 +443,7 @@ struct nvme_ns {
u8 pi_type;
#ifdef CONFIG_BLK_DEV_ZONED
u64 zsze;
+ bool zoned_ns_supp;
#endif
unsigned long features;
unsigned long flags;
diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
index 57cfd78731fb..1ae039f9c20c 100644
--- a/drivers/nvme/host/zns.c
+++ b/drivers/nvme/host/zns.c
@@ -42,22 +42,25 @@ int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
struct request_queue *q = disk->queue;
struct nvme_command c = { };
struct nvme_id_ns_zns *id;
+ bool zoned_ns_supp = true;
int status;
/* Driver requires zone append support */
if (!(le32_to_cpu(log->iocs[nvme_cmd_zone_append]) &
NVME_CMD_EFFECTS_CSUPP)) {
+ zoned_ns_supp = false;
dev_warn(ns->ctrl->device,
"append not supported for zoned namespace:%d\n",
ns->head->ns_id);
- return -EINVAL;
- }
-
- /* Lazily query controller append limit for the first zoned namespace */
- if (!ns->ctrl->max_zone_append) {
- status = nvme_set_max_append(ns->ctrl);
- if (status)
- return status;
+ } else {
+ /* Lazily query controller append limit for the first
+ * zoned namespace
+ */
+ if (!ns->ctrl->max_zone_append) {
+ status = nvme_set_max_append(ns->ctrl);
+ if (status)
+ return status;
+ }
}
id = kzalloc(sizeof(*id), GFP_KERNEL);
@@ -78,23 +81,22 @@ int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
* operation characteristics.
*/
if (id->zoc) {
+ zoned_ns_supp = false;
dev_warn(ns->ctrl->device,
"zone operations:%x not supported for namespace:%u\n",
le16_to_cpu(id->zoc), ns->head->ns_id);
- status = -EINVAL;
- goto free_data;
}
ns->zsze = nvme_lba_to_sect(ns, le64_to_cpu(id->lbafe[lbaf].zsze));
if (!is_power_of_2(ns->zsze)) {
+ zoned_ns_supp = false;
dev_warn(ns->ctrl->device,
"invalid zone size:%llu for namespace:%u\n",
ns->zsze, ns->head->ns_id);
- status = -EINVAL;
- goto free_data;
}
q->limits.zoned = BLK_ZONED_HM;
+ ns->zoned_ns_supp = zoned_ns_supp;
blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
blk_queue_max_open_zones(q, le32_to_cpu(id->mor) + 1);
blk_queue_max_active_zones(q, le32_to_cpu(id->mar) + 1);
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs
2020-11-02 13:22 Javier González
@ 2020-11-02 15:44 ` Keith Busch
0 siblings, 0 replies; 17+ messages in thread
From: Keith Busch @ 2020-11-02 15:44 UTC (permalink / raw)
To: Javier González
Cc: linux-nvme, linux-block, hch, sagi, axboe, joshi.k, k.jensen,
Niklas.Cassel, Javier González
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:
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-11-05 7:42 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20201102161505eucas1p19415e34eb0b14c7eca5a2c648569cf1e@eucas1p1.samsung.com>
[not found] ` <0916865d50c640e3aa95dc542f3986b9@CAMSVWEXC01.scsc.local>
2020-11-02 16:30 ` [PATCH V2] nvme: report capacity 0 for non supported ZNS SSDs Niklas Cassel
2020-11-02 21:25 ` Javier Gonzalez
2020-11-02 18:08 ` [PATCH V2] " hch
2020-11-02 18:33 ` Keith Busch
2020-11-02 18:58 ` hch
2020-11-02 21:24 ` Javier Gonzalez
2020-11-03 9:06 ` hch
2020-11-03 14:10 ` Javier Gonzalez
2020-11-03 15:26 ` hch
2020-11-03 15:54 ` Javier Gonzalez
2020-11-04 14:26 ` [PATCH V2] " Hannes Reinecke
2020-11-04 14:29 ` hch
2020-11-04 14:46 ` Hannes Reinecke
2020-11-05 7:37 ` hch
2020-11-05 7:42 ` Hannes Reinecke
2020-11-02 13:22 Javier González
2020-11-02 15:44 ` Keith Busch
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).