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,
