Re: [PATCH 12/15] virtio_blk: pass queue_limits to blk_mq_alloc_disk

[John Garry <john.g.garry@oracle.com>]

On 29/06/2024 06:19, Christoph Hellwig wrote:
> On Fri, Jun 28, 2024 at 03:25:02PM +0100, John Garry wrote:
>> I think that we might need a change like the following change after this:
>>
>> ----8<----
>> [PATCH] virtio_blk: Fix default logical block size
>>
>> If we fail to read a logical block size in virtblk_read_limits() ->
>> virtio_cread_feature(), then we default to what is in
>> lim->logical_block_size, but that would be 0.
>>
>> We can deal with lim->logical_block_size = 0 later in the
>> blk_mq_alloc_disk(), but the subsequent code in virtblk_read_limits()
>> cannot, so give a default of SECTOR_SIZE.
> 
> I think the right fix would be to initialize it where the virtio code
> currently uses the uninitialized lim->logical_block_size.  That assumes
> that we really want to handle this case.  If reading the block size
> fails, can we really trust reading other less basic attributes?  Or
> should we just fail the probe?

According to the comment on virtio_cread_feature, it is conditional 
(which I read as optional) and that function can only fail with -ENOENT. 
So I don't think that the probe should fail. virtio people?

 From my qemu testing, it is always supplied (obvs).

> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 6c64a67ab9c901..5bde41505818f9 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -1303,7 +1303,7 @@ static int virtblk_read_limits(struct virtio_blk *vblk,
>   
>   		lim->logical_block_size = blk_size;
>   	} else
> -		blk_size = lim->logical_block_size;
> +		blk_size = SECTOR_SIZE;

I think that it would need to be:
		blk_size = lim->logical_block_size = SECTOR_SIZE;

Which is a big ugly, so maybe:

	if (err)
		blk_size = SECTOR_SIZE;
	lim->logical_block_size = SECTOR_SIZE;

or, alternatively, set bsize to SECTOR_SIZE when declared.

>   
>   	/* Use topology information if available */
>   	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,