[PATCH] nvmet-file: clamp-down file namespace lba_shift

[PATCH] 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>
---
This issue was observed in I/O errors in the log running test nvme/009.

 drivers/nvme/target/io-cmd-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 517522305e5c..5008826cbd6a 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -49,7 +49,7 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 		goto err;
 
 	ns->size = stat.size;
-	ns->blksize_shift = file_inode(ns->file)->i_blkbits;
+	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] nvmet-file: clamp-down file namespace lba_shift

[Chaitanya Kulkarni]

On 3/5/19 1:55 AM, 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>
> ---
> This issue was observed in I/O errors in the log running test nvme/009.
>
We are not doing any I/O in nvme/009 explicitly using dd/fio, is this 
due to internal commands we send as a part of connect and disconnect ?
Or test number is different ?

>   drivers/nvme/target/io-cmd-file.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
> index 517522305e5c..5008826cbd6a 100644
> --- a/drivers/nvme/target/io-cmd-file.c
> +++ b/drivers/nvme/target/io-cmd-file.c
> @@ -49,7 +49,7 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
>   		goto err;
>   
>   	ns->size = stat.size;
> -	ns->blksize_shift = file_inode(ns->file)->i_blkbits;
> +	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),
> 

[PATCH] 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>
>> ---
>> This issue was observed in I/O errors in the log running test nvme/009.
>>
> We are not doing any I/O in nvme/009 explicitly using dd/fio, is this
> due to internal commands we send as a part of connect and disconnect ?
> Or test number is different ?

The former.

Also, I just now hit a divide by zero BUG when running test 009 with
multipath enabled.

Quick look shows that this is coming from blk_queue_stack_limits for the
ns_head.

Looks like nvme_update_disk_info() is reading a bs=0:
--
         unsigned short bs = 1 << ns->lba_shift;
--

bs is 16 bits and lba_shift is 20
(coming directly from the tempfile file_inode(ns->file)->i_blkbits)

So this fixes that one as well. Regardless, I think bs should be
unsigned int?

[PATCH] nvmet-file: clamp-down file namespace lba_shift

[Chaitanya Kulkarni]

On 3/6/19 12:19 PM, 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>
>>> ---
>>> This issue was observed in I/O errors in the log running test nvme/009.
>>>
>> We are not doing any I/O in nvme/009 explicitly using dd/fio, is this
>> due to internal commands we send as a part of connect and disconnect ?
>> Or test number is different ?
> 
> The former.
> 
> Also, I just now hit a divide by zero BUG when running test 009 with
> multipath enabled.
> 
> Quick look shows that this is coming from blk_queue_stack_limits for the
> ns_head.
> 
> Looks like nvme_update_disk_info() is reading a bs=0:
> --
>           unsigned short bs = 1 << ns->lba_shift;
> --
> 
> bs is 16 bits and lba_shift is 20
> (coming directly from the tempfile file_inode(ns->file)->i_blkbits)
> 
> So this fixes that one as well. Regardless, I think bs should be
> unsigned int?

Yes.

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

[PATCH] nvmet-file: clamp-down file namespace lba_shift

[Keith Busch]

On Wed, Mar 06, 2019@08:38:21PM +0000, Chaitanya Kulkarni wrote:
> On 3/6/19 12:19 PM, Sagi Grimberg wrote:
> > Quick look shows that this is coming from blk_queue_stack_limits for the
> > ns_head.
> > 
> > Looks like nvme_update_disk_info() is reading a bs=0:
> > --
> >           unsigned short bs = 1 << ns->lba_shift;
> > --
> > 
> > bs is 16 bits and lba_shift is 20
> > (coming directly from the tempfile file_inode(ns->file)->i_blkbits)
> > 
> > So this fixes that one as well. Regardless, I think bs should be
> > unsigned int?
> 
> Yes.

blk_queue_logical_block_size() takes an 'unsigned short', so be sure to
check 'bs' is less than USHRT_MAX.

[PATCH] nvmet-file: clamp-down file namespace lba_shift

[Sagi Grimberg]

>>> Quick look shows that this is coming from blk_queue_stack_limits for the
>>> ns_head.
>>>
>>> Looks like nvme_update_disk_info() is reading a bs=0:
>>> --
>>>            unsigned short bs = 1 << ns->lba_shift;
>>> --
>>>
>>> bs is 16 bits and lba_shift is 20
>>> (coming directly from the tempfile file_inode(ns->file)->i_blkbits)
>>>
>>> So this fixes that one as well. Regardless, I think bs should be
>>> unsigned int?
>>
>> Yes.
> 
> blk_queue_logical_block_size() takes an 'unsigned short', so be sure to
> check 'bs' is less than USHRT_MAX.

So this means that we should fail ns allocation if lba_shift is more
than fits in a short.

[PATCH] nvmet-file: clamp-down file namespace lba_shift

[Christoph Hellwig]

> -	ns->blksize_shift = file_inode(ns->file)->i_blkbits;
> +	ns->blksize_shift = min_t(u8, file_inode(ns->file)->i_blkbits, PAGE_SHIFT);

I think we want to limit to 4k as the universally acceptable upper
bound.  Also this should not use u8 for the case but probably an
unsigned int.

Last but not least this introduced a > 80 char line.