[PATCH v4 0/3] Couple of fixes detected with blktests

[PATCH v4 0/3] Couple of fixes detected with blktests

[Sagi Grimberg]

nvme test 009 discovered a few issues with and without multipathing.

1. if a namespace has a larger block size than PAGE_SIZE we either panic
   on a divide by zero with multipathing, or we see I/O errors without it.
   - fix nvmet to always export block size that is leq PAGE_SIZE
   - fix the core to set 0 capacity on such a namespace

2. if a namespace allocation fails after nvme_init_ns_head we leak
   subsystems due to a missing ns_head ref put
   - fix it by putting the ns_head reference on nvme_alloc_ns error flow

Changes from v3:
- don't have the host fail if ns allocation lba_shift exceeds PAGE_SHIFT,
  instead set 0 capacity

Changes from v2:
- add comment to nvmet file ns lba_shift clamping
- add host side protection against such a namespace

Sagi Grimberg (3):
  nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE
  nvme: put ns_head ref if namespace fails allocation
  nvmet-file: clamp-down file namespace lba_shift

 drivers/nvme/host/core.c          | 8 +++++++-
 drivers/nvme/target/io-cmd-file.c | 7 ++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Sagi Grimberg]

If our target exposed a namespace with a block size that is greater
than PAGE_SIZE, set 0 capacity on the namespace as we do not support it.

This issue encountered when the nvmet namespace was backed by a tempfile.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 7321da21f8c9..e08979094eb5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1610,6 +1610,10 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	sector_t capacity = le64_to_cpup(&id->nsze) << (ns->lba_shift - 9);
 	unsigned short bs = 1 << ns->lba_shift;
 
+	if (ns->lba_shift > PAGE_SHIFT) {
+		/* unsupported block size, set capacity to 0 later */
+		bs = (1 << 9);
+	}
 	blk_mq_freeze_queue(disk->queue);
 	blk_integrity_unregister(disk);
 
@@ -1621,7 +1625,8 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	if (ns->ms && !ns->ext &&
 	    (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED))
 		nvme_init_integrity(disk, ns->ms, ns->pi_type);
-	if (ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk))
+	if ((ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk)) ||
+	    ns->lba_shift > PAGE_SHIFT)
 		capacity = 0;
 
 	set_capacity(disk, capacity);
-- 
2.17.1

[PATCH v4 2/3] nvme: put ns_head ref if namespace fails allocation

[Sagi Grimberg]

In case nvme_alloc_ns fails after we initialize ns_head but before we
add the ns to the controller namespaces list we need to explicitly put
the ns_head reference because when we teardown the controller we
won't find it, causing us to leak a dangling subsystem eventually.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e08979094eb5..4530243ef706 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3314,6 +3314,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	mutex_lock(&ctrl->subsys->lock);
 	list_del_rcu(&ns->siblings);
 	mutex_unlock(&ctrl->subsys->lock);
+	nvme_put_ns_head(ns->head);
  out_free_id:
 	kfree(id);
  out_free_queue:
-- 
2.17.1

[PATCH v4 3/3] nvmet-file: clamp-down file namespace lba_shift

[Sagi Grimberg]

When the backing file is a tempfile for example, the inode i_blkbits
can be 1M in size which causes problems for hosts to support as the
disk block size.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/io-cmd-file.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 517522305e5c..d52fe965a5d5 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -49,7 +49,12 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 		goto err;
 
 	ns->size = stat.size;
-	ns->blksize_shift = file_inode(ns->file)->i_blkbits;
+	/*
+	 * inode i_blkbits can be greater than the universally accepted upper
+	 * bound so make sure we export we export a sane namespace lba_shift.
+	 */
+	ns->blksize_shift = min_t(u8,
+			file_inode(ns->file)->i_blkbits, PAGE_SHIFT);
 
 	ns->bvec_cache = kmem_cache_create("nvmet-bvec",
 			NVMET_MAX_MPOOL_BVEC * sizeof(struct bio_vec),
-- 
2.17.1

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Keith Busch]

On Mon, Mar 11, 2019@03:02:25PM -0700, Sagi Grimberg wrote:
> If our target exposed a namespace with a block size that is greater
> than PAGE_SIZE, set 0 capacity on the namespace as we do not support it.
> 
> This issue encountered when the nvmet namespace was backed by a tempfile.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>

I responded to the v3 with the review by mistake. :)

Reviewed-by: Keith Busch <keith.busch at intel.com>

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Christoph Hellwig]

On Mon, Mar 11, 2019@03:02:25PM -0700, Sagi Grimberg wrote:
> If our target exposed a namespace with a block size that is greater
> than PAGE_SIZE, set 0 capacity on the namespace as we do not support it.
> 
> This issue encountered when the nvmet namespace was backed by a tempfile.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>  drivers/nvme/host/core.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 7321da21f8c9..e08979094eb5 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1610,6 +1610,10 @@ static void nvme_update_disk_info(struct gendisk *disk,
>  	sector_t capacity = le64_to_cpup(&id->nsze) << (ns->lba_shift - 9);
>  	unsigned short bs = 1 << ns->lba_shift;
>  
> +	if (ns->lba_shift > PAGE_SHIFT) {
> +		/* unsupported block size, set capacity to 0 later */
> +		bs = (1 << 9);
> +	}
>  	blk_mq_freeze_queue(disk->queue);
>  	blk_integrity_unregister(disk);
>  
> @@ -1621,7 +1625,8 @@ static void nvme_update_disk_info(struct gendisk *disk,
>  	if (ns->ms && !ns->ext &&
>  	    (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED))
>  		nvme_init_integrity(disk, ns->ms, ns->pi_type);
> -	if (ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk))
> +	if ((ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk)) ||
> +	    ns->lba_shift > PAGE_SHIFT)
>  		capacity = 0;

I like the idea behind this, but it looks rather convoluted.  I think
for the unusable namespace case we should warn and have a common label
that just sets the capacity, not touching anything else.

Does something like this work for you?

---
>From ec4ce1708eb4d4f536b93f09dab952067f58ba4b Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Tue, 12 Mar 2019 15:31:09 +0100
Subject: nvme: ignore namespaces with an unusable block size

And also refactor the handling for unusable namespaces to use a common
goto label, and print a warning for invalid metadata formats as well
while we are at it.

Reported-by: Sagi Grimberg <sagi at grimberg.me>
Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1597efae6af8..0cf5741ecb7b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1599,6 +1599,12 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	blk_mq_freeze_queue(disk->queue);
 	blk_integrity_unregister(disk);
 
+	if (bs > PAGE_SIZE) {
+		dev_warn(ns->ctrl->device,
+			"unsupported LBA size %u, ignoring namespace\n", bs);
+		goto ignore;
+	}
+
 	blk_queue_logical_block_size(disk->queue, bs);
 	blk_queue_physical_block_size(disk->queue, bs);
 	blk_queue_io_min(disk->queue, bs);
@@ -1606,8 +1612,11 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	if (ns->ms && !ns->ext &&
 	    (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED))
 		nvme_init_integrity(disk, ns->ms, ns->pi_type);
-	if (ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk))
-		capacity = 0;
+	if (ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk)) {
+		dev_warn(ns->ctrl->device,
+			"unsupported metadata format, ignoring namespace\n");
+		goto ignore;
+	}
 
 	set_capacity(disk, capacity);
 	nvme_ns_config_oncs(ns);
@@ -1618,6 +1627,10 @@ static void nvme_update_disk_info(struct gendisk *disk,
 		set_disk_ro(disk, false);
 
 	blk_mq_unfreeze_queue(disk->queue);
+	return;
+ignore:
+	blk_mq_unfreeze_queue(disk->queue);
+	set_capacity(disk, 0);
 }
 
 static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
-- 
2.20.1

[PATCH v4 3/3] nvmet-file: clamp-down file namespace lba_shift

[Christoph Hellwig]

On Mon, Mar 11, 2019@03:02:27PM -0700, Sagi Grimberg wrote:
> When the backing file is a tempfile for example, the inode i_blkbits
> can be 1M in size which causes problems for hosts to support as the
> disk block size.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>  drivers/nvme/target/io-cmd-file.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
> index 517522305e5c..d52fe965a5d5 100644
> --- a/drivers/nvme/target/io-cmd-file.c
> +++ b/drivers/nvme/target/io-cmd-file.c
> @@ -49,7 +49,12 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
>  		goto err;
>  
>  	ns->size = stat.size;
> -	ns->blksize_shift = file_inode(ns->file)->i_blkbits;
> +	/*
> +	 * inode i_blkbits can be greater than the universally accepted upper
> +	 * bound so make sure we export we export a sane namespace lba_shift.
> +	 */
> +	ns->blksize_shift = min_t(u8,
> +			file_inode(ns->file)->i_blkbits, PAGE_SHIFT);

I still wonder why we clamp this down to PAGE SIZE rather than 4k.

Everything larger than 4k has a huge chance of not working with most
remote hosts, even if our target system has a larger page size.

[PATCH v4 2/3] nvme: put ns_head ref if namespace fails allocation

[Christoph Hellwig]

Thanks,

applied to nvme-5.1.

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Christoph Hellwig]

And while we are at it - Dave Chinner has been actively working on
blocksize > PAGE_SIZE support for XFS.  So we might be able to use
"too large" sectors sizes very soon.

[PATCH v4 3/3] nvmet-file: clamp-down file namespace lba_shift

[Sagi Grimberg]

>> When the backing file is a tempfile for example, the inode i_blkbits
>> can be 1M in size which causes problems for hosts to support as the
>> disk block size.
>>
>> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
>> ---
>>   drivers/nvme/target/io-cmd-file.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
>> index 517522305e5c..d52fe965a5d5 100644
>> --- a/drivers/nvme/target/io-cmd-file.c
>> +++ b/drivers/nvme/target/io-cmd-file.c
>> @@ -49,7 +49,12 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
>>   		goto err;
>>   
>>   	ns->size = stat.size;
>> -	ns->blksize_shift = file_inode(ns->file)->i_blkbits;
>> +	/*
>> +	 * inode i_blkbits can be greater than the universally accepted upper
>> +	 * bound so make sure we export we export a sane namespace lba_shift.
>> +	 */
>> +	ns->blksize_shift = min_t(u8,
>> +			file_inode(ns->file)->i_blkbits, PAGE_SHIFT);
> 
> I still wonder why we clamp this down to PAGE SIZE rather than 4k.
> 
> Everything larger than 4k has a huge chance of not working with most
> remote hosts, even if our target system has a larger page size.

loop does the same thing, didn't ever hear complaints on that..

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Sagi Grimberg]

> I like the idea behind this, but it looks rather convoluted.  I think
> for the unusable namespace case we should warn and have a common label
> that just sets the capacity, not touching anything else.
> 
> Does something like this work for you?

No, this is what I had done originally, but we need to always have the
queue set to a decent block size, otherwise blk_queue_stack_limits()
panics on div by 0..

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Christoph Hellwig]

On Tue, Mar 12, 2019@02:15:26PM -0700, Sagi Grimberg wrote:
>
>> I like the idea behind this, but it looks rather convoluted.  I think
>> for the unusable namespace case we should warn and have a common label
>> that just sets the capacity, not touching anything else.
>>
>> Does something like this work for you?
>
> No, this is what I had done originally, but we need to always have the
> queue set to a decent block size, otherwise blk_queue_stack_limits()
> panics on div by 0..

I actually tested it by manually hacking a 8k block size into nvmet
and and it works just fine for me.  Where do you see a division by
zero with this patch exactly?

[PATCH v4 3/3] nvmet-file: clamp-down file namespace lba_shift

[Christoph Hellwig]

On Tue, Mar 12, 2019@02:07:38PM -0700, Sagi Grimberg wrote:
>> I still wonder why we clamp this down to PAGE SIZE rather than 4k.
>>
>> Everything larger than 4k has a huge chance of not working with most
>> remote hosts, even if our target system has a larger page size.
>
> loop does the same thing, didn't ever hear complaints on that..

loop is always used on the same system exporting things..

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Keith Busch]

On Fri, Mar 15, 2019@05:28:37PM +0100, Christoph Hellwig wrote:
> On Tue, Mar 12, 2019@02:15:26PM -0700, Sagi Grimberg wrote:
> >
> >> I like the idea behind this, but it looks rather convoluted.  I think
> >> for the unusable namespace case we should warn and have a common label
> >> that just sets the capacity, not touching anything else.
> >>
> >> Does something like this work for you?
> >
> > No, this is what I had done originally, but we need to always have the
> > queue set to a decent block size, otherwise blk_queue_stack_limits()
> > panics on div by 0..
> 
> I actually tested it by manually hacking a 8k block size into nvmet
> and and it works just fine for me.  Where do you see a division by
> zero with this patch exactly?

I'm not sure abuot a divide-by-zero, but I just hacked up qemu to report
an 8k block size and get this on boot. Happens because alloc_page_buffers
won't allocate a buffer_head when block size is greater than a page size:

 BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
 #PF error: [normal kernel read fault]
 PGD 0 P4D 0
 Oops: 0000 [#1] SMP
 CPU: 3 PID: 391 Comm: kworker/u18:1 Not tainted 5.0.0+ #42
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.0-0-ga698c8995f-prebuilt.qemu.org 04/01/2014
 Workqueue: nvme-wq nvme_scan_work [nvme_core]
 RIP: 0010:create_empty_buffers+0x24/0x100
 Code: eb cb 0f 1f 40 00 0f 1f 44 00 00 41 54 55 49 89 d4 53 ba 01 00 00 00 48 89 fb e8 87 fe ff ff 48 89 c5 48 89 c2 eb 03 48 89 ca <48> 8b 4a 08 4c 09 22 48 85 c9 75 f1 48 89 6a 08 48 8b 43 18 48 8d
 RSP: 0018:ffffbd9ec05cf880 EFLAGS: 00010286
 RAX: 0000000000000000 RBX: ffffe0fec03a38c0 RCX: ffff99f4c751d000
 RDX: 0000000000000000 RSI: ffff99f4c751d000 RDI: ffffe0fec03a38c0
 RBP: 0000000000000000 R08: dead0000000000ff R09: 0000000000000003
 R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
 R13: 0000000000000000 R14: 0000000000000200 R15: ffffe0fec03a38c0
 FS:  0000000000000000(0000) GS:ffff99f4cfd00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 0000000000000008 CR3: 000000000eb28000 CR4: 00000000000006e0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 Call Trace:
  create_page_buffers+0x4d/0x60
  block_read_full_page+0x47/0x310
  ? __add_to_page_cache_locked+0x288/0x330
  ? check_disk_change+0x60/0x60
  ? count_shadow_nodes+0x130/0x130
  do_read_cache_page+0x31c/0x6b0
  ? blkdev_writepages+0x10/0x10
  read_dev_sector+0x28/0xc0
  read_lba+0x126/0x210
  ? kmem_cache_alloc_trace+0x19b/0x1b0
  efi_partition+0x137/0x780
  ? vsnprintf+0x2ae/0x4a0
  ? vsnprintf+0xec/0x4a0
  ? snprintf+0x45/0x70
  ? is_gpt_valid.part.6+0x400/0x400
  ? check_partition+0x137/0x240
  check_partition+0x137/0x240
  rescan_partitions+0xab/0x350
  __blkdev_get+0x342/0x560
  ? inode_insert5+0x11f/0x1e0
  blkdev_get+0x11f/0x310
  ? unlock_new_inode+0x44/0x60
  ? bdget+0xff/0x110
  __device_add_disk+0x426/0x470
  nvme_validate_ns+0x35e/0x7c0 [nvme_core]
  ? nvme_identify_ctrl.isra.56+0x7e/0xc0 [nvme_core]
  ? update_load_avg+0x89/0x550
  nvme_scan_work+0xe5/0x370 [nvme_core]
  ? __synchronize_srcu.part.18+0x91/0xc0
  ? try_to_wake_up+0x55/0x430
  process_one_work+0x1e9/0x3e0
  worker_thread+0x21a/0x3d0
  ? process_one_work+0x3e0/0x3e0
  kthread+0x111/0x130
  ? kthread_park+0x90/0x90
  ret_from_fork+0x1f/0x30
 Modules linked in: nvme nvme_core serio_raw
 CR2: 0000000000000008
 ---[ end trace b38bdf1b424f36e9 ]---

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Christoph Hellwig]

On Fri, Mar 15, 2019@10:38:43AM -0600, Keith Busch wrote:
> On Fri, Mar 15, 2019@05:28:37PM +0100, Christoph Hellwig wrote:
> > On Tue, Mar 12, 2019@02:15:26PM -0700, Sagi Grimberg wrote:
> > >
> > >> I like the idea behind this, but it looks rather convoluted.  I think
> > >> for the unusable namespace case we should warn and have a common label
> > >> that just sets the capacity, not touching anything else.
> > >>
> > >> Does something like this work for you?
> > >
> > > No, this is what I had done originally, but we need to always have the
> > > queue set to a decent block size, otherwise blk_queue_stack_limits()
> > > panics on div by 0..
> > 
> > I actually tested it by manually hacking a 8k block size into nvmet
> > and and it works just fine for me.  Where do you see a division by
> > zero with this patch exactly?
> 
> I'm not sure abuot a divide-by-zero, but I just hacked up qemu to report
> an 8k block size and get this on boot. Happens because alloc_page_buffers
> won't allocate a buffer_head when block size is greater than a page size:

But that is without either the patch from Sagi or the one from me,
right?

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Keith Busch]

On Fri, Mar 15, 2019@05:40:57PM +0100, Christoph Hellwig wrote:
> On Fri, Mar 15, 2019@10:38:43AM -0600, Keith Busch wrote:
> > I'm not sure abuot a divide-by-zero, but I just hacked up qemu to report
> > an 8k block size and get this on boot. Happens because alloc_page_buffers
> > won't allocate a buffer_head when block size is greater than a page size:
> 
> But that is without either the patch from Sagi or the one from me,
> right?

Sorry, I misunderstood to think you were saying only that Sagi's patch
wasn't necessary. Yes, either patch works.

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Sagi Grimberg]

>>> I like the idea behind this, but it looks rather convoluted.  I think
>>> for the unusable namespace case we should warn and have a common label
>>> that just sets the capacity, not touching anything else.
>>>
>>> Does something like this work for you?
>>
>> No, this is what I had done originally, but we need to always have the
>> queue set to a decent block size, otherwise blk_queue_stack_limits()
>> panics on div by 0..
> 
> I actually tested it by manually hacking a 8k block size into nvmet
> and and it works just fine for me.  Where do you see a division by
> zero with this patch exactly?

You need your block size to exceed u16 bs.

Then run blktest/tests/nvme/009 with multipath on.

[PATCH v4 3/3] nvmet-file: clamp-down file namespace lba_shift

[Sagi Grimberg]

>>> I still wonder why we clamp this down to PAGE SIZE rather than 4k.
>>>
>>> Everything larger than 4k has a huge chance of not working with most
>>> remote hosts, even if our target system has a larger page size.
>>
>> loop does the same thing, didn't ever hear complaints on that..
> 
> loop is always used on the same system exporting things..

Yea, I see what you're saying... I can change it to 4k.

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Christoph Hellwig]

On Fri, Mar 15, 2019@10:17:18AM -0700, Sagi Grimberg wrote:
>
>>>> I like the idea behind this, but it looks rather convoluted.  I think
>>>> for the unusable namespace case we should warn and have a common label
>>>> that just sets the capacity, not touching anything else.
>>>>
>>>> Does something like this work for you?
>>>
>>> No, this is what I had done originally, but we need to always have the
>>> queue set to a decent block size, otherwise blk_queue_stack_limits()
>>> panics on div by 0..
>>
>> I actually tested it by manually hacking a 8k block size into nvmet
>> and and it works just fine for me.  Where do you see a division by
>> zero with this patch exactly?
>
> You need your block size to exceed u16 bs.

I'll need to look into actually testing something that large, but
the whole point of my patch is that we never even use the bs value
for anything. So I'm not sure why that would matter.

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Sagi Grimberg]

>>> I actually tested it by manually hacking a 8k block size into nvmet
>>> and and it works just fine for me.  Where do you see a division by
>>> zero with this patch exactly?
>>
>> You need your block size to exceed u16 bs.
> 
> I'll need to look into actually testing something that large, 

just run blktests nvme/009 test with multipath, nothing fancy (I used
ext4).

 From a quick look I think that ext4 tempfiles are coming from
the transaction subsystem so maybe that's why the inode i_blkbits
is 20? Not sure at all though...

Anyways, its very easy to reproduce.

> but the whole point of my patch is that we never even use the bs value
> for anything. So I'm not sure why that would matter.

But than with multipath, blk_queue_stack_limits assume that we have a
valid bs in the lower device queue.

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Sagi Grimberg]

>>>> I actually tested it by manually hacking a 8k block size into nvmet
>>>> and and it works just fine for me.? Where do you see a division by
>>>> zero with this patch exactly?
>>>
>>> You need your block size to exceed u16 bs.
>>
>> I'll need to look into actually testing something that large, 
> 
> just run blktests nvme/009 test with multipath, nothing fancy (I used
> ext4).
> 
>  From a quick look I think that ext4 tempfiles are coming from
> the transaction subsystem so maybe that's why the inode i_blkbits
> is 20? Not sure at all though...
> 
> Anyways, its very easy to reproduce.
> 
>> but the whole point of my patch is that we never even use the bs value
>> for anything. So I'm not sure why that would matter.
> 
> But than with multipath, blk_queue_stack_limits assume that we have a
> valid bs in the lower device queue.

Reviving this,

This is still failing without this patch.

Reproducer:
- use ext4 filesystem
- set nvme.multipath=1
- run blktests: tests/nvme/009

bang...

Christoph,

The reason why bs is set to 512 in nvme_update_disk_info() is because
blk_queue_stack_limits assumes that q->limits.logical_block_size of the
underlying device is not zero (automatically divides by it).

[PATCH v4 1/3] nvme: set 0 capacity if namespace block size exceeds PAGE_SIZE

[Christoph Hellwig]

Thanks,

applied to nvme-5.2.