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

John Garry
2024-06-30T09:55:45Z

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,