Regression in xen-blkfront regarding sector sizes

Regression in xen-blkfront regarding sector sizes

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]

Hi,

Some Qubes users report a regression in xen-blkfront regarding block
size reporting. It works fine on 6.8.8, but appears broken on 6.9.2.

The specific problem is that blkfront reports block size of 512, even for
backend devices of 4096. This, for example, fails 512-bytes reads with
O_DIRECT, and appears to break mounting a filesystem on such a device
(at least xfs one).

For example it looks like this:

    [user@dom0 ~]$ head /sys/block/loop12/queue/*_block_size
    ==> /sys/block/loop12/queue/logical_block_size <==
    4096

    ==> /sys/block/loop12/queue/physical_block_size <==
    4096

    [user@dom0 bin]$ qvm-run -p the-vm 'head /sys/block/xvdi/queue/*_block_size'
    ==> /sys/block/xvdi/queue/logical_block_size <==
    512

    ==> /sys/block/xvdi/queue/physical_block_size <==
    512

and then:

    $ sudo dd if=/dev/xvdi of=/dev/null count=1 status=progress iflag=direct
    /usr/bin/dd: error reading '/dev/xvdi': Input/output error
    0+0 records in
    0+0 records out
    0 bytes copied, 0.000170858 s, 0.0 kB/s

and mounting fails like this:

    [   68.055045] SGI XFS with ACLs, security attributes, realtime, scrub, quota, no debug enabled
    [   68.057308] I/O error, dev xvdi, sector 0 op 0x0:(READ) flags 0x1000 phys_seg 1 prio class 0
    [   68.057333] XFS (xvdi): SB validate failed with error -5.

More details at https://github.com/QubesOS/qubes-issues/issues/9293

Rusty suspects it's related to
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/block/xen-blkfront.c?id=ba3f67c1163812b5d7ec33705c31edaa30ce6c51,
so I'm cc-ing people mentioned there too.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Regression in xen-blkfront regarding sector sizes

[Jürgen Groß]

On 24.06.24 15:48, Marek Marczykowski-Górecki wrote:
> Hi,
> 
> Some Qubes users report a regression in xen-blkfront regarding block
> size reporting. It works fine on 6.8.8, but appears broken on 6.9.2.
> 
> The specific problem is that blkfront reports block size of 512, even for
> backend devices of 4096. This, for example, fails 512-bytes reads with
> O_DIRECT, and appears to break mounting a filesystem on such a device
> (at least xfs one).
> 
> For example it looks like this:
> 
>      [user@dom0 ~]$ head /sys/block/loop12/queue/*_block_size
>      ==> /sys/block/loop12/queue/logical_block_size <==
>      4096
> 
>      ==> /sys/block/loop12/queue/physical_block_size <==
>      4096
> 
>      [user@dom0 bin]$ qvm-run -p the-vm 'head /sys/block/xvdi/queue/*_block_size'
>      ==> /sys/block/xvdi/queue/logical_block_size <==
>      512
> 
>      ==> /sys/block/xvdi/queue/physical_block_size <==
>      512
> 
> and then:
> 
>      $ sudo dd if=/dev/xvdi of=/dev/null count=1 status=progress iflag=direct
>      /usr/bin/dd: error reading '/dev/xvdi': Input/output error
>      0+0 records in
>      0+0 records out
>      0 bytes copied, 0.000170858 s, 0.0 kB/s
> 
> and mounting fails like this:
> 
>      [   68.055045] SGI XFS with ACLs, security attributes, realtime, scrub, quota, no debug enabled
>      [   68.057308] I/O error, dev xvdi, sector 0 op 0x0:(READ) flags 0x1000 phys_seg 1 prio class 0
>      [   68.057333] XFS (xvdi): SB validate failed with error -5.
> 
> More details at https://github.com/QubesOS/qubes-issues/issues/9293
> 
> Rusty suspects it's related to
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/block/xen-blkfront.c?id=ba3f67c1163812b5d7ec33705c31edaa30ce6c51,
> so I'm cc-ing people mentioned there too.

I think the call of blkif_set_queue_limits() in this patch should NOT precede
setting of info->sector_size and info->physical_sector_size, as those are
needed by blkif_set_queue_limits().


Juergen


> 

Re: Regression in xen-blkfront regarding sector sizes

[Christoph Hellwig]

On Mon, Jun 24, 2024 at 04:29:15PM +0200, Jürgen Groß wrote:
>> Rusty suspects it's related to
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/block/xen-blkfront.c?id=ba3f67c1163812b5d7ec33705c31edaa30ce6c51,
>> so I'm cc-ing people mentioned there too.
>
> I think the call of blkif_set_queue_limits() in this patch should NOT precede
> setting of info->sector_size and info->physical_sector_size, as those are
> needed by blkif_set_queue_limits().

Yes.  Something like the patch below should fix it.  We could also stop
passing sector_size and physіcal_sector_size to xlvbd_alloc_gendisk to
clean things up a bit more.

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index fd7c0ff2139cee..9f3d68044f8882 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1133,6 +1133,8 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
 	if (err)
 		goto out_release_minors;
 
+	info->sector_size = sector_size;
+	info->physical_sector_size = physical_sector_size;
 	blkif_set_queue_limits(info, &lim);
 	gd = blk_mq_alloc_disk(&info->tag_set, &lim, info);
 	if (IS_ERR(gd)) {
@@ -1159,8 +1161,6 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
 
 	info->rq = gd->queue;
 	info->gd = gd;
-	info->sector_size = sector_size;
-	info->physical_sector_size = physical_sector_size;
 
 	xlvbd_flush(info);
 

Re: Regression in xen-blkfront regarding sector sizes

[Rusty Bird]

Christoph Hellwig:
> On Mon, Jun 24, 2024 at 04:29:15PM +0200, Jürgen Groß wrote:
> >> Rusty suspects it's related to
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/block/xen-blkfront.c?id=ba3f67c1163812b5d7ec33705c31edaa30ce6c51,
> >> so I'm cc-ing people mentioned there too.
> >
> > I think the call of blkif_set_queue_limits() in this patch should NOT precede
> > setting of info->sector_size and info->physical_sector_size, as those are
> > needed by blkif_set_queue_limits().
> 
> Yes.  Something like the patch below should fix it.  We could also stop
> passing sector_size and physіcal_sector_size to xlvbd_alloc_gendisk to
> clean things up a bit more.
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index fd7c0ff2139cee..9f3d68044f8882 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1133,6 +1133,8 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
>  	if (err)
>  		goto out_release_minors;
>  
> +	info->sector_size = sector_size;
> +	info->physical_sector_size = physical_sector_size;
>  	blkif_set_queue_limits(info, &lim);
>  	gd = blk_mq_alloc_disk(&info->tag_set, &lim, info);
>  	if (IS_ERR(gd)) {
> @@ -1159,8 +1161,6 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
>  
>  	info->rq = gd->queue;
>  	info->gd = gd;
> -	info->sector_size = sector_size;
> -	info->physical_sector_size = physical_sector_size;
>  
>  	xlvbd_flush(info);
>  
> 
> 

With this patch applied on top of v6.9.4, I get the correct
logical/physical block sizes and the issue is fixed. Thank you!

Rusty