Skip to contentSkip to conversation

virtio_blk: fix defaults for max_hw_sectors and max_segment_size

PATCH Superseded Mike Snitzer 5 participants 15 messages Nov 20, 2014 – Nov 26, 2014

Subsystems (4)
Block layer virtio core Device mapper virtio block and SCSI drivers
lore
Share & tools
lore

Mike Snitzer

author
#0 lore raw

PATCH 1/1 virtio_blk: fix defaults for max_hw_sectors and max_segment_size

virtio_blk incorrectly established -1U as the default for these queue_limits. Set these limits to sane default values to avoid crashing the kernel. But the virtio-blk protocol should probably be extended to allow proper stacking of the disk's limits from the host.

This change fixes a crash that was reported when virtio-blk was used to test linux-dm.git commit 604ea90641b4 ("dm thin: adjust max_sectors_kb based on thinp blocksize") that will initially set max_sectors to max_hw_sectors and then rounddown to the first power-of-2 factor of the DM thin-pool's blocksize. Basically that commit assumes drivers don't suck when establishing max_hw_sectors so it acted like a canary in the coal mine.

In the case of a DM thin-pool built ontop of virtio-blk data device these are the insane limits that were established for the DM thin-pool:

  # cat /sys/block/dm-6/queue/max_sectors_kb
  1073741824
  # cat /sys/block/dm-6/queue/max_hw_sectors_kb
  2147483647

by stacking the virtio-blk device's limits:

  # cat /sys/block/vdb/queue/max_sectors_kb
  512
  # cat /sys/block/vdb/queue/max_hw_sectors_kb
  2147483647

Attempting to mkfs.xfs against a thin device from this thin-pool quickly resulted in fs/direct-io.c:dio_send_cur_page()'s BUG_ON.

Signed-off-by Mike Snitzer <[email protected]>
Cc [email protected]
---
 drivers/block/virtio_blk.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
drivers/block/virtio_blk.c +6 -3
index c6a27d5..68efbdc 100644
@@ -674,8 +674,11 @@ static int virtblk_probe(struct virtio_device *vdev)
 	/* No need to bounce any requests */
 	blk_queue_bounce_limit(q, BLK_BOUNCE_ANY);
 
-	/* No real sector limit. */
-	blk_queue_max_hw_sectors(q, -1U);
+	/*
+	 * Limited by disk's max_hw_sectors in host, but
+	 * without that info establish a sane default.
+	 */
+	blk_queue_max_hw_sectors(q, BLK_DEF_MAX_SECTORS);
 
 	/* Host can optionally specify maximum segment size and number of
 	 * segments. */

@@ -684,7 +687,7 @@ static int virtblk_probe(struct virtio_device *vdev)
 	if (!err)
 		blk_queue_max_segment_size(q, v);
 	else
-		blk_queue_max_segment_size(q, -1U);
+		blk_queue_max_segment_size(q, BLK_MAX_SEGMENT_SIZE);
 
 	/* Host can optionally specify the block size of the device */
 	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE,
Signature · 1 line
1.7.4.4
maintainer
#1 lore raw
Quoted 23 lines · Mike Snitzer

virtio_blk incorrectly established -1U as the default for these queue_limits. Set these limits to sane default values to avoid crashing the kernel. But the virtio-blk protocol should probably be extended to allow proper stacking of the disk's limits from the host.

This change fixes a crash that was reported when virtio-blk was used to test linux-dm.git commit 604ea90641b4 ("dm thin: adjust max_sectors_kb based on thinp blocksize") that will initially set max_sectors to max_hw_sectors and then rounddown to the first power-of-2 factor of the DM thin-pool's blocksize. Basically that commit assumes drivers don't suck when establishing max_hw_sectors so it acted like a canary in the coal mine.

In the case of a DM thin-pool built ontop of virtio-blk data device these are the insane limits that were established for the DM thin-pool:

  # cat /sys/block/dm-6/queue/max_sectors_kb
  1073741824
  # cat /sys/block/dm-6/queue/max_hw_sectors_kb
  2147483647

by stacking the virtio-blk device's limits:

Quoted 7 lines
  # cat /sys/block/vdb/queue/max_sectors_kb
  512
  # cat /sys/block/vdb/queue/max_hw_sectors_kb
  2147483647

Attempting to mkfs.xfs against a thin device from this thin-pool quickly resulted in fs/direct-io.c:dio_send_cur_page()'s BUG_ON.

Why exactly does it BUG_ON?
Did some memory allocation fail?

Will it still BUG_ON if host gives us high values?

If linux makes assumptions about hardware limits, won't it be better to put them in blk core and not in individual drivers?

Quoted 11 lines
Signed-off-by Mike Snitzer <[email protected]>
Cc [email protected]
---
 drivers/block/virtio_blk.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
drivers/block/virtio_blk.c +0 -0
index c6a27d5..68efbdc 100644

drivers/block/virtio_blk.c static int virtblk_probe(struct virtio_device *vdev) · line 681

Quoted 10 lines
drivers/block/virtio_blk.c +5 -2
@@ -674,8 +674,11 @@ static int virtblk_probe(struct virtio_device *vdev)
 	/* No need to bounce any requests */
 	blk_queue_bounce_limit(q, BLK_BOUNCE_ANY);
 
-	/* No real sector limit. */
-	blk_queue_max_hw_sectors(q, -1U);
+	/*
+	 * Limited by disk's max_hw_sectors in host, but
+	 * without that info establish a sane default.
+	 */
+	blk_queue_max_hw_sectors(q, BLK_DEF_MAX_SECTORS);

I see
drivers/usb/storage/scsiglue.c: blk_queue_max_hw_sectors(sdev->request_queue, 0x7FFFFF);

so maybe we should go higher, and use INT_MAX too?

drivers/block/virtio_blk.c static int virtblk_probe(struct virtio_device *vdev) · line 693

Quoted 12 lines
drivers/block/virtio_blk.c +1 -1
 	/* Host can optionally specify maximum segment size and number of
 	 * segments. */

@@ -684,7 +687,7 @@ static int virtblk_probe(struct virtio_device *vdev)
 	if (!err)
 		blk_queue_max_segment_size(q, v);
 	else
-		blk_queue_max_segment_size(q, -1U);
+		blk_queue_max_segment_size(q, BLK_MAX_SEGMENT_SIZE);
 
 	/* Host can optionally specify the block size of the device */
 	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE,

Here too, I see some drivers asking for more:
drivers/block/mtip32xx/mtip32xx.c: blk_queue_max_segment_size(dd->queue, 0x400000);

Quoted 2 lines
Signature · 1 line
1.7.4.4
author
#2 lore raw
Quoted 4 lines · Michael S. Tsirkin
Quoted 3 lines · Mike Snitzer

virtio_blk incorrectly established -1U as the default for these queue_limits. Set these limits to sane default values to avoid crashing the kernel.

...

Quoted 5 lines
Quoted 2 lines

Attempting to mkfs.xfs against a thin device from this thin-pool quickly resulted in fs/direct-io.c:dio_send_cur_page()'s BUG_ON.

Why exactly does it BUG_ON?
Did some memory allocation fail?

No idea, kernel log doesn't say.. all it has is "kernel BUG" pointing to fs/direct-io.c:dio_send_cur_page()'s BUG_ON.

I could dig deeper on _why_ but honestly, there really isn't much point. virtio-blk doesn't get to live in fantasy-land just because it happens to think it is limitless.

Quoted 1 line

Will it still BUG_ON if host gives us high values?

Maybe, if/when virtio-blk allows the host to inject a value for max_hw_sectors. But my fix doesn't stack the host's limits up, it sets a value that isn't prone to make the block/fs layers BUG.

Quoted 3 lines

If linux makes assumptions about hardware limits, won't it be better to put them in blk core and not in individual drivers?

The individual block driver is meant to establish sane values for these limits.

Block core _does_ have some sane wrappers for stacking these limits (e.g. blk_stack_limits, etc). All of those wrappers are meant to allow for virtual drivers to build up limits that respect the underlying hardware's limits.

But virtio-blk doesn't use any of them due to the virtio-blk driver relying on the virtio-blk protocol to encapsulate each and every one of them.

Quoted 25 lines
Quoted 11 lines
Signed-off-by Mike Snitzer <[email protected]>
Cc [email protected]
---
 drivers/block/virtio_blk.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
drivers/block/virtio_blk.c +0 -0
index c6a27d5..68efbdc 100644

drivers/block/virtio_blk.c static int virtblk_probe(struct virtio_device *vdev) · line 681

Quoted 10 lines
drivers/block/virtio_blk.c +5 -2
@@ -674,8 +674,11 @@ static int virtblk_probe(struct virtio_device *vdev)
 	/* No need to bounce any requests */
 	blk_queue_bounce_limit(q, BLK_BOUNCE_ANY);
 
-	/* No real sector limit. */
-	blk_queue_max_hw_sectors(q, -1U);
+	/*
+	 * Limited by disk's max_hw_sectors in host, but
+	 * without that info establish a sane default.
+	 */
+	blk_queue_max_hw_sectors(q, BLK_DEF_MAX_SECTORS);

I see
drivers/usb/storage/scsiglue.c: blk_queue_max_hw_sectors(sdev->request_queue, 0x7FFFFF);

Quoted 1 line

so maybe we should go higher, and use INT_MAX too?

No, higher doesn't help _at all_ if the driver itself doesn't actually take care to stack the underlying driver's limits. Without limits stacking (which virtio-blk doesn't really have) it is the lack of reality-based default values that is _the_ problem that enduced this BUG.

blk_stack_limits() does a lot of min_t(top, bottom), etc. So you want the default "top" of a stacking driver to be high enough so as not to artificially limit the resulting stacked limit. Which is why we have things like blk_set_stacking_limits(). You'll note that blk_set_stacking_limits() properly establishes UINT_MAX, etc. BUT it is "proper" purely because drivers that call it (e.g. DM) also make use of the block layer's limits stacking functions (again,
e.g. blk_stack_limits).

Quoted 10 lines
Quoted 10 lines
drivers/block/virtio_blk.c +1 -1
 	/* Host can optionally specify maximum segment size and number of
 	 * segments. */

@@ -684,7 +687,7 @@ static int virtblk_probe(struct virtio_device *vdev)
 	if (!err)
 		blk_queue_max_segment_size(q, v);
 	else
-		blk_queue_max_segment_size(q, -1U);
+		blk_queue_max_segment_size(q, BLK_MAX_SEGMENT_SIZE);
Quoted 5 lines

drivers/block/virtio_blk.c static int virtblk_probe(struct virtio_device *vdev) · line 693

Quoted 2 lines
drivers/block/virtio_blk.c +0 -0
 	/* Host can optionally specify the block size of the device */
 	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE,

Here too, I see some drivers asking for more:
drivers/block/mtip32xx/mtip32xx.c: blk_queue_max_segment_size(dd->queue, 0x400000);

Those drivers you listed could be equally broken..

For virtio-blk the issue is that the limits it establishes don't reflect the underlying host's hardware capabilties. This was a virtio-blk time bomb waiting to go off.

And to be clear, I just fixed the blk_queue_max_segment_size(q, -1U); because it is blatantly wrong when we've established BLK_MAX_SEGMENT_SIZE.

The bug that was reported is purely due to max_hw_sectors being 2TB and the established max_sectors being 1TB.

author
#5 lore raw
In reply to Mike Snitzer
Quoted 5 lines · Mike Snitzer

virtio_blk incorrectly established -1U as the default for these queue_limits. Set these limits to sane default values to avoid crashing the kernel. But the virtio-blk protocol should probably be extended to allow proper stacking of the disk's limits from the host.

Quoted 7 lines

This change fixes a crash that was reported when virtio-blk was used to test linux-dm.git commit 604ea90641b4 ("dm thin: adjust max_sectors_kb based on thinp blocksize") that will initially set max_sectors to max_hw_sectors and then rounddown to the first power-of-2 factor of the DM thin-pool's blocksize. Basically that commit assumes drivers don't suck when establishing max_hw_sectors so it acted like a canary in the coal mine.

I have changed that DM thinp code to not be so fragile with this follow-on fix:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-for-3.19&id=971ab7029b61ec10e0765bfb96331448ce5c3094

Quoted 10 lines

In the case of a DM thin-pool built ontop of virtio-blk data device these are the insane limits that were established for the DM thin-pool:

  # cat /sys/block/dm-6/queue/max_sectors_kb
  1073741824
  # cat /sys/block/dm-6/queue/max_hw_sectors_kb
  2147483647

by stacking the virtio-blk device's limits:

Quoted 7 lines
  # cat /sys/block/vdb/queue/max_sectors_kb
  512
  # cat /sys/block/vdb/queue/max_hw_sectors_kb
  2147483647

Attempting to mkfs.xfs against a thin device from this thin-pool quickly resulted in fs/direct-io.c:dio_send_cur_page()'s BUG_ON.

But virtio_blk really must be fixed. I'll post v2 of this patch with a revised header that skips all the references to DM thinp, etc.

author
#6 lore raw

PATCH v2 1/1 virtio_blk: fix defaults for max_hw_sectors and max_segment_size

In reply to Mike Snitzer

virtio_blk incorrectly established -1U as the default for these queue_limits. Set these limits to sane default values to avoid crashing the kernel. But the virtio-blk protocol should probably be extended to allow proper stacking of the disk's limits from the host.

This change fixes a crash that can occur if the max_sectors_kb is modified to even be half of virtio_blk's advertised max_hw_sectors_kb:

  # cat /sys/block/vdb/queue/max_sectors_kb
  512
  # cat /sys/block/vdb/queue/max_hw_sectors_kb
  2147483647
  # echo 1073741824 > /sys/block/vdb/queue/max_sectors_kb

Attempting to mkfs.xfs against /dev/vdb will result in hitting fs/direct-io.c:dio_send_cur_page()'s BUG_ON.

While fixing the blk_queue_max_hw_sectors(q, -1U) call it was immediately apparent that the blk_queue_max_segment_size(q, -1U) also should be fixed.

Signed-off-by Mike Snitzer <[email protected]>
Cc [email protected]
---
 drivers/block/virtio_blk.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

v2: revise header to simplify the scope of the problem explanation

drivers/block/virtio_blk.c +6 -3
index c6a27d5..68efbdc 100644
@@ -674,8 +674,11 @@ static int virtblk_probe(struct virtio_device *vdev)
 	/* No need to bounce any requests */
 	blk_queue_bounce_limit(q, BLK_BOUNCE_ANY);
 
-	/* No real sector limit. */
-	blk_queue_max_hw_sectors(q, -1U);
+	/*
+	 * Limited by disk's max_hw_sectors in host, but
+	 * without that info establish a sane default.
+	 */
+	blk_queue_max_hw_sectors(q, BLK_DEF_MAX_SECTORS);
 
 	/* Host can optionally specify maximum segment size and number of
 	 * segments. */

@@ -684,7 +687,7 @@ static int virtblk_probe(struct virtio_device *vdev)
 	if (!err)
 		blk_queue_max_segment_size(q, v);
 	else
-		blk_queue_max_segment_size(q, -1U);
+		blk_queue_max_segment_size(q, BLK_MAX_SEGMENT_SIZE);
 
 	/* Host can optionally specify the block size of the device */
 	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE,
Signature · 1 line
1.7.4.4
#7 lore raw
In reply to Mike Snitzer
Quoted 5 lines · Mike Snitzer

virtio_blk incorrectly established -1U as the default for these queue_limits. Set these limits to sane default values to avoid crashing the kernel. But the virtio-blk protocol should probably be extended to allow proper stacking of the disk's limits from the host.

Quoted 7 lines

This change fixes a crash that was reported when virtio-blk was used to test linux-dm.git commit 604ea90641b4 ("dm thin: adjust max_sectors_kb based on thinp blocksize") that will initially set max_sectors to max_hw_sectors and then rounddown to the first power-of-2 factor of the DM thin-pool's blocksize. Basically that commit assumes drivers don't suck when establishing max_hw_sectors so it acted like a canary in the coal mine.

Is that a crash in the host or guest? What kind of mishandling did you see? Unless the recent virtio standard changed anything the host should be able to handle our arbitrary limits, and even if it doesn't that something we need to hash out with qemu and the virtio standards folks.

author
#8 lore raw
Quoted 14 lines · Christoph Hellwig
Quoted 5 lines · Mike Snitzer

virtio_blk incorrectly established -1U as the default for these queue_limits. Set these limits to sane default values to avoid crashing the kernel. But the virtio-blk protocol should probably be extended to allow proper stacking of the disk's limits from the host.

Quoted 7 lines

This change fixes a crash that was reported when virtio-blk was used to test linux-dm.git commit 604ea90641b4 ("dm thin: adjust max_sectors_kb based on thinp blocksize") that will initially set max_sectors to max_hw_sectors and then rounddown to the first power-of-2 factor of the DM thin-pool's blocksize. Basically that commit assumes drivers don't suck when establishing max_hw_sectors so it acted like a canary in the coal mine.

Quoted 5 lines

Is that a crash in the host or guest? What kind of mishandling did you see? Unless the recent virtio standard changed anything the host should be able to handle our arbitrary limits, and even if it doesn't that something we need to hash out with qemu and the virtio standards folks.

Some good news: this guest crash isn't an issue with recent kernels (so upstream, fedora 20, RHEL7, etc aren't impacted -- Jens feel free to drop my virtio_blk patch; even though some of it's limits are clearly broken I'll defer to the virtio_blk developers on the best way forward -- sorry for the noise!).

The BUG I saw only seems to impact RHEL6 kernels so far (note to self, actually _test_ on upstream before reporting a crash against upstream!)

[root@RHEL-6 ~]# echo 1073741824 > /sys/block/vdc/queue/max_sectors_kb [root@RHEL-6 ~]# lvs

Message from syslogd@RHEL-6 at Nov 21 15:32:15 ...
kernel:Kernel panic - not syncing: Fatal exception

Here is the RHEL6 guest crash, just for full disclosure:

kernel BUG at fs/direct-io.c:696!
invalid opcode: 0000 [#1] SMP
last sysfs file: /sys/devices/virtual/block/dm-4/dev CPU 0

Modules linked in: nfs lockd fscache auth_rpcgss nfs_acl sunrpc ipv6 ext2 dm_thin_pool dm_bio_prison dm_persistent_data dm_bufio libcrc32c dm_mirror dm_region_hash dm_log dm_mod microcode virtio_balloon i2c_piix4 i2c_core virtio_net ext4 jbd2 mbcache virtio_blk virtio_pci virtio_ring virtio pata_acpi ata_generic ata_piix [last unloaded: speedstep_lib]

Pid: 1679, comm: lvs Not tainted 2.6.32 #6 Bochs Bochs

RIP: 0010:[<ffffffff811ce336>]  [<ffffffff811ce336>] __blockdev_direct_IO_newtrunc+0x986/0x1270
RSP: 0018:ffff88011a11ba48  EFLAGS: 00010287
RAX: 0000000000000000 RBX: ffff8801192fbd28 RCX: 0000000000001000
RDX: ffffea0003b3d218 RSI: ffff88011aac4300 RDI: ffff880118572378
RBP: ffff88011a11bbe8 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801192fbd00
R13: 0000000000000000 R14: ffff880118c3cac0 R15: 0000000000000000
FS:  00007fde78bc37a0(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000012706f0 CR3: 000000011a432000 CR4: 00000000000407f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process lvs (pid: 1679, threadinfo ffff88011a11a000, task ffff8801185a4aa0)
Stack:
 ffff88011a11bb48 ffff88011a11baa8 ffff88010000000c ffff88011a11bb18
<d> 0000000000000000 0000000000000000 ffff88011a11bdc8 ffff88011a11beb8
<d> 0000000c1a11baa8 ffff880118c3cb98 0000000000000000 0000000018c3ccb8
Call Trace:
 [<ffffffff811c9e90>] ? blkdev_get_block+0x0/0x20
 [<ffffffff811cec97>] __blockdev_direct_IO+0x77/0xe0
 [<ffffffff811c9e90>] ? blkdev_get_block+0x0/0x20
 [<ffffffff811caf17>] blkdev_direct_IO+0x57/0x60
 [<ffffffff811c9e90>] ? blkdev_get_block+0x0/0x20
 [<ffffffff8112619b>] generic_file_aio_read+0x6bb/0x700
 [<ffffffff811cba60>] ? blkdev_get+0x10/0x20
 [<ffffffff811cba70>] ? blkdev_open+0x0/0xc0
 [<ffffffff8118af4f>] ? __dentry_open+0x23f/0x360
 [<ffffffff811ca2d1>] blkdev_aio_read+0x51/0x80
 [<ffffffff8118dc6a>] do_sync_read+0xfa/0x140
 [<ffffffff8109eaf0>] ? autoremove_wake_function+0x0/0x40
 [<ffffffff811ca22c>] ? block_ioctl+0x3c/0x40
 [<ffffffff811a34c2>] ? vfs_ioctl+0x22/0xa0
 [<ffffffff811a3664>] ? do_vfs_ioctl+0x84/0x580
 [<ffffffff8122cee6>] ? security_file_permission+0x16/0x20
 [<ffffffff8118e625>] vfs_read+0xb5/0x1a0
 [<ffffffff8118e761>] sys_read+0x51/0x90
 [<ffffffff810e5aae>] ? __audit_syscall_exit+0x25e/0x290
 [<ffffffff8100b072>] system_call_fastpath+0x16/0x1b
Code: fe ff ff c7 85 fc fe ff ff 00 00 00 00 48 89 95 10 ff ff ff 8b 95 34 ff ff ff e8 46 ac ff ff 3b 85 34 ff ff ff 0f 84 fc 02 00 00 <0f> 0b eb fe 8b 9d 34 ff ff ff 8b 85 30 ff ff ff 01 d8 85 c0 0f
RIP  [<ffffffff811ce336>] __blockdev_direct_IO_newtrunc+0x986/0x1270
 RSP <ffff88011a11ba48>
---[ end trace 73be5dcaf8939399 ]---
Kernel panic - not syncing: Fatal exception
#3 lore raw
In reply to Mike Snitzer
Quoted 11 lines · Mike Snitzer
Quoted 4 lines · Michael S. Tsirkin
Quoted 3 lines · Mike Snitzer

virtio_blk incorrectly established -1U as the default for these queue_limits. Set these limits to sane default values to avoid crashing the kernel.

...

Quoted 3 lines
Quoted 2 lines

Attempting to mkfs.xfs against a thin device from this thin-pool quickly resulted in fs/direct-io.c:dio_send_cur_page()'s BUG_ON.

Quoted 7 lines
Quoted 2 lines

Why exactly does it BUG_ON?
Did some memory allocation fail?

No idea, kernel log doesn't say.. all it has is "kernel BUG" pointing to fs/direct-io.c:dio_send_cur_page()'s BUG_ON.

I could dig deeper on _why_ but honestly, there really isn't much point.

There is *always* a point in understanding the code you are modifying.

Quoted 2 lines

virtio-blk doesn't get to live in fantasy-land just because it happens to think it is limitless.

Calm down please.

We don't have a sector limit. We have a segment limit, which is set above this line.

Cheers,
Rusty.

author
#4 lore raw
Quoted 15 lines · Rusty Russell
Quoted 14 lines · Mike Snitzer
Quoted 4 lines · Michael S. Tsirkin
Quoted 3 lines · Mike Snitzer

virtio_blk incorrectly established -1U as the default for these queue_limits. Set these limits to sane default values to avoid crashing the kernel.

...

Quoted 5 lines
Quoted 2 lines

Attempting to mkfs.xfs against a thin device from this thin-pool quickly resulted in fs/direct-io.c:dio_send_cur_page()'s BUG_ON.

Why exactly does it BUG_ON?
Did some memory allocation fail?

Quoted 6 lines
Quoted 4 lines

No idea, kernel log doesn't say.. all it has is "kernel BUG" pointing to fs/direct-io.c:dio_send_cur_page()'s BUG_ON.

I could dig deeper on _why_ but honestly, there really isn't much point.

There is *always* a point in understanding the code you are modifying.

Yes, I agree (and understanding the BUG in question will be pursued). But in the context of the patch I proposed it was irrelevent. virtio-blk still _should_ fix its limits to reflect those of the block device it stacks on. My patch was a stop-gap until proper virtio-blk protocol extensions were added. But you don't seem inclined to care.

Quoted 4 lines
Quoted 2 lines

virtio-blk doesn't get to live in fantasy-land just because it happens to think it is limitless.

Calm down please.

Sure, but it'd have helped if virtio-blk developers demonstrated acknowledgement that a stacking block driver should stack the limits of the underlying device. Instead you decided to trim all related portions of my reply to mst that were measured and helpful.

Quoted 2 lines

We don't have a sector limit. We have a segment limit, which is set above this line.

Then at a minimum max_hw_sectors should reflect that segment limit.

But again, the underlying device has limits that should be stacked up. Why is that irrelevent to virtio-blk? Plus, this is a matter of not allowing a user to shoot themselves in the foot by fiddling with traditional block limits only to find in some kernel (*cough* RHEL6) they result in BUG.

Mike

maintainer
#9 lore raw
In reply to Mike Snitzer
Quoted 82 lines · Mike Snitzer
Quoted 14 lines · Christoph Hellwig
Quoted 5 lines · Mike Snitzer

virtio_blk incorrectly established -1U as the default for these queue_limits. Set these limits to sane default values to avoid crashing the kernel. But the virtio-blk protocol should probably be extended to allow proper stacking of the disk's limits from the host.

Quoted 7 lines

This change fixes a crash that was reported when virtio-blk was used to test linux-dm.git commit 604ea90641b4 ("dm thin: adjust max_sectors_kb based on thinp blocksize") that will initially set max_sectors to max_hw_sectors and then rounddown to the first power-of-2 factor of the DM thin-pool's blocksize. Basically that commit assumes drivers don't suck when establishing max_hw_sectors so it acted like a canary in the coal mine.

Quoted 5 lines

Is that a crash in the host or guest? What kind of mishandling did you see? Unless the recent virtio standard changed anything the host should be able to handle our arbitrary limits, and even if it doesn't that something we need to hash out with qemu and the virtio standards folks.

Some good news: this guest crash isn't an issue with recent kernels (so upstream, fedora 20, RHEL7, etc aren't impacted -- Jens feel free to drop my virtio_blk patch; even though some of it's limits are clearly broken I'll defer to the virtio_blk developers on the best way forward -- sorry for the noise!).

The BUG I saw only seems to impact RHEL6 kernels so far (note to self, actually _test_ on upstream before reporting a crash against upstream!)

[root@RHEL-6 ~]# echo 1073741824 > /sys/block/vdc/queue/max_sectors_kb [root@RHEL-6 ~]# lvs

Message from syslogd@RHEL-6 at Nov 21 15:32:15 ...
kernel:Kernel panic - not syncing: Fatal exception

Here is the RHEL6 guest crash, just for full disclosure:

kernel BUG at fs/direct-io.c:696!
invalid opcode: 0000 [#1] SMP
last sysfs file: /sys/devices/virtual/block/dm-4/dev CPU 0

Modules linked in: nfs lockd fscache auth_rpcgss nfs_acl sunrpc ipv6 ext2 dm_thin_pool dm_bio_prison dm_persistent_data dm_bufio libcrc32c dm_mirror dm_region_hash dm_log dm_mod microcode virtio_balloon i2c_piix4 i2c_core virtio_net ext4 jbd2 mbcache virtio_blk virtio_pci virtio_ring virtio pata_acpi ata_generic ata_piix [last unloaded: speedstep_lib]

Pid: 1679, comm: lvs Not tainted 2.6.32 #6 Bochs Bochs

RIP: 0010:[<ffffffff811ce336>]  [<ffffffff811ce336>] __blockdev_direct_IO_newtrunc+0x986/0x1270
RSP: 0018:ffff88011a11ba48  EFLAGS: 00010287
RAX: 0000000000000000 RBX: ffff8801192fbd28 RCX: 0000000000001000
RDX: ffffea0003b3d218 RSI: ffff88011aac4300 RDI: ffff880118572378
RBP: ffff88011a11bbe8 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801192fbd00
R13: 0000000000000000 R14: ffff880118c3cac0 R15: 0000000000000000
FS:  00007fde78bc37a0(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000012706f0 CR3: 000000011a432000 CR4: 00000000000407f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process lvs (pid: 1679, threadinfo ffff88011a11a000, task ffff8801185a4aa0)
Stack:
 ffff88011a11bb48 ffff88011a11baa8 ffff88010000000c ffff88011a11bb18
<d> 0000000000000000 0000000000000000 ffff88011a11bdc8 ffff88011a11beb8
<d> 0000000c1a11baa8 ffff880118c3cb98 0000000000000000 0000000018c3ccb8
Call Trace:
 [<ffffffff811c9e90>] ? blkdev_get_block+0x0/0x20
 [<ffffffff811cec97>] __blockdev_direct_IO+0x77/0xe0
 [<ffffffff811c9e90>] ? blkdev_get_block+0x0/0x20
 [<ffffffff811caf17>] blkdev_direct_IO+0x57/0x60
 [<ffffffff811c9e90>] ? blkdev_get_block+0x0/0x20
 [<ffffffff8112619b>] generic_file_aio_read+0x6bb/0x700
 [<ffffffff811cba60>] ? blkdev_get+0x10/0x20
 [<ffffffff811cba70>] ? blkdev_open+0x0/0xc0
 [<ffffffff8118af4f>] ? __dentry_open+0x23f/0x360
 [<ffffffff811ca2d1>] blkdev_aio_read+0x51/0x80
 [<ffffffff8118dc6a>] do_sync_read+0xfa/0x140
 [<ffffffff8109eaf0>] ? autoremove_wake_function+0x0/0x40
 [<ffffffff811ca22c>] ? block_ioctl+0x3c/0x40
 [<ffffffff811a34c2>] ? vfs_ioctl+0x22/0xa0
 [<ffffffff811a3664>] ? do_vfs_ioctl+0x84/0x580
 [<ffffffff8122cee6>] ? security_file_permission+0x16/0x20
 [<ffffffff8118e625>] vfs_read+0xb5/0x1a0
Quoted 8 lines
 [<ffffffff8118e761>] sys_read+0x51/0x90
 [<ffffffff810e5aae>] ? __audit_syscall_exit+0x25e/0x290
 [<ffffffff8100b072>] system_call_fastpath+0x16/0x1b
Code: fe ff ff c7 85 fc fe ff ff 00 00 00 00 48 89 95 10 ff ff ff 8b 95 34 ff ff ff e8 46 ac ff ff 3b 85 34 ff ff ff 0f 84 fc 02 00 00 <0f> 0b eb fe 8b 9d 34 ff ff ff 8b 85 30 ff ff ff 01 d8 85 c0 0f
RIP  [<ffffffff811ce336>] __blockdev_direct_IO_newtrunc+0x986/0x1270
 RSP <ffff88011a11ba48>
---[ end trace 73be5dcaf8939399 ]---
Kernel panic - not syncing: Fatal exception

That code isn't even in mainline, as far as I can tell...

Signature · 1 line
Jens Axboe
author
#10 lore raw
Quoted 92 lines · Jens Axboe
Quoted 82 lines · Mike Snitzer
Quoted 14 lines · Christoph Hellwig
Quoted 5 lines · Mike Snitzer

virtio_blk incorrectly established -1U as the default for these queue_limits. Set these limits to sane default values to avoid crashing the kernel. But the virtio-blk protocol should probably be extended to allow proper stacking of the disk's limits from the host.

Quoted 7 lines

This change fixes a crash that was reported when virtio-blk was used to test linux-dm.git commit 604ea90641b4 ("dm thin: adjust max_sectors_kb based on thinp blocksize") that will initially set max_sectors to max_hw_sectors and then rounddown to the first power-of-2 factor of the DM thin-pool's blocksize. Basically that commit assumes drivers don't suck when establishing max_hw_sectors so it acted like a canary in the coal mine.

Quoted 5 lines

Is that a crash in the host or guest? What kind of mishandling did you see? Unless the recent virtio standard changed anything the host should be able to handle our arbitrary limits, and even if it doesn't that something we need to hash out with qemu and the virtio standards folks.

Some good news: this guest crash isn't an issue with recent kernels (so upstream, fedora 20, RHEL7, etc aren't impacted -- Jens feel free to drop my virtio_blk patch; even though some of it's limits are clearly broken I'll defer to the virtio_blk developers on the best way forward -- sorry for the noise!).

The BUG I saw only seems to impact RHEL6 kernels so far (note to self, actually _test_ on upstream before reporting a crash against upstream!)

[root@RHEL-6 ~]# echo 1073741824 > /sys/block/vdc/queue/max_sectors_kb [root@RHEL-6 ~]# lvs

Message from syslogd@RHEL-6 at Nov 21 15:32:15 ...
kernel:Kernel panic - not syncing: Fatal exception

Here is the RHEL6 guest crash, just for full disclosure:

kernel BUG at fs/direct-io.c:696!
invalid opcode: 0000 [#1] SMP
last sysfs file: /sys/devices/virtual/block/dm-4/dev CPU 0

Modules linked in: nfs lockd fscache auth_rpcgss nfs_acl sunrpc ipv6 ext2 dm_thin_pool dm_bio_prison dm_persistent_data dm_bufio libcrc32c dm_mirror dm_region_hash dm_log dm_mod microcode virtio_balloon i2c_piix4 i2c_core virtio_net ext4 jbd2 mbcache virtio_blk virtio_pci virtio_ring virtio pata_acpi ata_generic ata_piix [last unloaded: speedstep_lib]

Pid: 1679, comm: lvs Not tainted 2.6.32 #6 Bochs Bochs

RIP: 0010:[<ffffffff811ce336>]  [<ffffffff811ce336>] __blockdev_direct_IO_newtrunc+0x986/0x1270
RSP: 0018:ffff88011a11ba48  EFLAGS: 00010287
RAX: 0000000000000000 RBX: ffff8801192fbd28 RCX: 0000000000001000
RDX: ffffea0003b3d218 RSI: ffff88011aac4300 RDI: ffff880118572378
RBP: ffff88011a11bbe8 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801192fbd00
R13: 0000000000000000 R14: ffff880118c3cac0 R15: 0000000000000000
FS:  00007fde78bc37a0(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000012706f0 CR3: 000000011a432000 CR4: 00000000000407f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process lvs (pid: 1679, threadinfo ffff88011a11a000, task ffff8801185a4aa0)
Stack:
 ffff88011a11bb48 ffff88011a11baa8 ffff88010000000c ffff88011a11bb18
<d> 0000000000000000 0000000000000000 ffff88011a11bdc8 ffff88011a11beb8
<d> 0000000c1a11baa8 ffff880118c3cb98 0000000000000000 0000000018c3ccb8
Call Trace:
 [<ffffffff811c9e90>] ? blkdev_get_block+0x0/0x20
 [<ffffffff811cec97>] __blockdev_direct_IO+0x77/0xe0
 [<ffffffff811c9e90>] ? blkdev_get_block+0x0/0x20
 [<ffffffff811caf17>] blkdev_direct_IO+0x57/0x60
 [<ffffffff811c9e90>] ? blkdev_get_block+0x0/0x20
 [<ffffffff8112619b>] generic_file_aio_read+0x6bb/0x700
 [<ffffffff811cba60>] ? blkdev_get+0x10/0x20
 [<ffffffff811cba70>] ? blkdev_open+0x0/0xc0
 [<ffffffff8118af4f>] ? __dentry_open+0x23f/0x360
 [<ffffffff811ca2d1>] blkdev_aio_read+0x51/0x80
 [<ffffffff8118dc6a>] do_sync_read+0xfa/0x140
 [<ffffffff8109eaf0>] ? autoremove_wake_function+0x0/0x40
 [<ffffffff811ca22c>] ? block_ioctl+0x3c/0x40
 [<ffffffff811a34c2>] ? vfs_ioctl+0x22/0xa0
 [<ffffffff811a3664>] ? do_vfs_ioctl+0x84/0x580
 [<ffffffff8122cee6>] ? security_file_permission+0x16/0x20
 [<ffffffff8118e625>] vfs_read+0xb5/0x1a0
Quoted 8 lines
 [<ffffffff8118e761>] sys_read+0x51/0x90
 [<ffffffff810e5aae>] ? __audit_syscall_exit+0x25e/0x290
 [<ffffffff8100b072>] system_call_fastpath+0x16/0x1b
Code: fe ff ff c7 85 fc fe ff ff 00 00 00 00 48 89 95 10 ff ff ff 8b 95 34 ff ff ff e8 46 ac ff ff 3b 85 34 ff ff ff 0f 84 fc 02 00 00 <0f> 0b eb fe 8b 9d 34 ff ff ff 8b 85 30 ff ff ff 01 d8 85 c0 0f
RIP  [<ffffffff811ce336>] __blockdev_direct_IO_newtrunc+0x986/0x1270
 RSP <ffff88011a11ba48>
---[ end trace 73be5dcaf8939399 ]---
Kernel panic - not syncing: Fatal exception
Quoted 1 line

That code isn't even in mainline, as far as I can tell...

Right, it is old RHEL6 code.

But I've yet to determine what changed upstream that enables this to "just work" with a really large max_sectors (I haven't been looking either).

maintainer
#11 lore raw
Quoted 95 lines · Mike Snitzer
Quoted 92 lines · Jens Axboe
Quoted 90 lines · Mike Snitzer
Quoted 14 lines · Christoph Hellwig
Quoted 5 lines · Mike Snitzer

virtio_blk incorrectly established -1U as the default for these queue_limits. Set these limits to sane default values to avoid crashing the kernel. But the virtio-blk protocol should probably be extended to allow proper stacking of the disk's limits from the host.

Quoted 7 lines

This change fixes a crash that was reported when virtio-blk was used to test linux-dm.git commit 604ea90641b4 ("dm thin: adjust max_sectors_kb based on thinp blocksize") that will initially set max_sectors to max_hw_sectors and then rounddown to the first power-of-2 factor of the DM thin-pool's blocksize. Basically that commit assumes drivers don't suck when establishing max_hw_sectors so it acted like a canary in the coal mine.

Quoted 5 lines

Is that a crash in the host or guest? What kind of mishandling did you see? Unless the recent virtio standard changed anything the host should be able to handle our arbitrary limits, and even if it doesn't that something we need to hash out with qemu and the virtio standards folks.

Some good news: this guest crash isn't an issue with recent kernels (so upstream, fedora 20, RHEL7, etc aren't impacted -- Jens feel free to drop my virtio_blk patch; even though some of it's limits are clearly broken I'll defer to the virtio_blk developers on the best way forward -- sorry for the noise!).

The BUG I saw only seems to impact RHEL6 kernels so far (note to self, actually _test_ on upstream before reporting a crash against upstream!)

[root@RHEL-6 ~]# echo 1073741824 > /sys/block/vdc/queue/max_sectors_kb [root@RHEL-6 ~]# lvs

Message from syslogd@RHEL-6 at Nov 21 15:32:15 ...
kernel:Kernel panic - not syncing: Fatal exception

Here is the RHEL6 guest crash, just for full disclosure:

kernel BUG at fs/direct-io.c:696!
invalid opcode: 0000 [#1] SMP
last sysfs file: /sys/devices/virtual/block/dm-4/dev CPU 0

Modules linked in: nfs lockd fscache auth_rpcgss nfs_acl sunrpc ipv6 ext2 dm_thin_pool dm_bio_prison dm_persistent_data dm_bufio libcrc32c dm_mirror dm_region_hash dm_log dm_mod microcode virtio_balloon i2c_piix4 i2c_core virtio_net ext4 jbd2 mbcache virtio_blk virtio_pci virtio_ring virtio pata_acpi ata_generic ata_piix [last unloaded: speedstep_lib]

Pid: 1679, comm: lvs Not tainted 2.6.32 #6 Bochs Bochs

RIP: 0010:[<ffffffff811ce336>]  [<ffffffff811ce336>] __blockdev_direct_IO_newtrunc+0x986/0x1270
RSP: 0018:ffff88011a11ba48  EFLAGS: 00010287
RAX: 0000000000000000 RBX: ffff8801192fbd28 RCX: 0000000000001000
RDX: ffffea0003b3d218 RSI: ffff88011aac4300 RDI: ffff880118572378
RBP: ffff88011a11bbe8 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801192fbd00
R13: 0000000000000000 R14: ffff880118c3cac0 R15: 0000000000000000
FS:  00007fde78bc37a0(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000012706f0 CR3: 000000011a432000 CR4: 00000000000407f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process lvs (pid: 1679, threadinfo ffff88011a11a000, task ffff8801185a4aa0)
Stack:
 ffff88011a11bb48 ffff88011a11baa8 ffff88010000000c ffff88011a11bb18
<d> 0000000000000000 0000000000000000 ffff88011a11bdc8 ffff88011a11beb8
<d> 0000000c1a11baa8 ffff880118c3cb98 0000000000000000 0000000018c3ccb8
Call Trace:
 [<ffffffff811c9e90>] ? blkdev_get_block+0x0/0x20
 [<ffffffff811cec97>] __blockdev_direct_IO+0x77/0xe0
 [<ffffffff811c9e90>] ? blkdev_get_block+0x0/0x20
 [<ffffffff811caf17>] blkdev_direct_IO+0x57/0x60
 [<ffffffff811c9e90>] ? blkdev_get_block+0x0/0x20
 [<ffffffff8112619b>] generic_file_aio_read+0x6bb/0x700
 [<ffffffff811cba60>] ? blkdev_get+0x10/0x20
 [<ffffffff811cba70>] ? blkdev_open+0x0/0xc0
 [<ffffffff8118af4f>] ? __dentry_open+0x23f/0x360
 [<ffffffff811ca2d1>] blkdev_aio_read+0x51/0x80
 [<ffffffff8118dc6a>] do_sync_read+0xfa/0x140
 [<ffffffff8109eaf0>] ? autoremove_wake_function+0x0/0x40
 [<ffffffff811ca22c>] ? block_ioctl+0x3c/0x40
 [<ffffffff811a34c2>] ? vfs_ioctl+0x22/0xa0
 [<ffffffff811a3664>] ? do_vfs_ioctl+0x84/0x580
 [<ffffffff8122cee6>] ? security_file_permission+0x16/0x20
 [<ffffffff8118e625>] vfs_read+0xb5/0x1a0
 [<ffffffff8118e761>] sys_read+0x51/0x90
 [<ffffffff810e5aae>] ? __audit_syscall_exit+0x25e/0x290
 [<ffffffff8100b072>] system_call_fastpath+0x16/0x1b
Code: fe ff ff c7 85 fc fe ff ff 00 00 00 00 48 89 95 10 ff ff ff 8b 95 34 ff ff ff e8 46 ac ff ff 3b 85 34 ff ff ff 0f 84 fc 02 00 00 <0f> 0b eb fe 8b 9d 34 ff ff ff 8b 85 30 ff ff ff 01 d8 85 c0 0f
RIP  [<ffffffff811ce336>] __blockdev_direct_IO_newtrunc+0x986/0x1270
 RSP <ffff88011a11ba48>
---[ end trace 73be5dcaf8939399 ]---
Kernel panic - not syncing: Fatal exception
Quoted 7 lines
Quoted 1 line

That code isn't even in mainline, as far as I can tell...

Right, it is old RHEL6 code.

But I've yet to determine what changed upstream that enables this to "just work" with a really large max_sectors (I haven't been looking either).

Kind of hard for the rest of us to say, since it's triggering a BUG in code we don't have :-)

Signature · 1 line
Jens Axboe
author
#12 lore raw
Quoted 7 lines · Jens Axboe
Quoted 6 lines · Mike Snitzer
Quoted 2 lines · Jens Axboe

That code isn't even in mainline, as far as I can tell...

Quoted 8 lines
Quoted 5 lines

Right, it is old RHEL6 code.

But I've yet to determine what changed upstream that enables this to "just work" with a really large max_sectors (I haven't been looking either).

Kind of hard for the rest of us to say, since it's triggering a BUG in code we don't have :-)

I never asked you or others to weigh in on old RHEL6 code. Once I realized upstream worked even if max_sectors is _really_ high I said "sorry for the noise".

But while you're here, I wouldn't mind getting your take on virtio-blk setting max_hw_sectors to -1U.

As I said in my original reply to mst: it only makes sense to set a really high initial upper bound like that in a driver if that driver goes on to stack an underlying device's limit.

maintainer
#13 lore raw
Quoted 23 lines · Mike Snitzer
Quoted 7 lines · Jens Axboe
Quoted 6 lines · Mike Snitzer
Quoted 2 lines · Jens Axboe

That code isn't even in mainline, as far as I can tell...

Quoted 8 lines
Quoted 5 lines

Right, it is old RHEL6 code.

But I've yet to determine what changed upstream that enables this to "just work" with a really large max_sectors (I haven't been looking either).

Kind of hard for the rest of us to say, since it's triggering a BUG in code we don't have :-)

I never asked you or others to weigh in on old RHEL6 code. Once I realized upstream worked even if max_sectors is _really_ high I said "sorry for the noise".

Quoted 6 lines

But while you're here, I wouldn't mind getting your take on virtio-blk setting max_hw_sectors to -1U.

As I said in my original reply to mst: it only makes sense to set a really high initial upper bound like that in a driver if that driver goes on to stack an underlying device's limit.

-1U should just work, IMHO, there's no reason we should need to cap it at some synthetic value. That said, it seems it should be one of those parameters that should be negotiated up and set appropriately.

Signature · 1 line
Jens Axboe
author
#14 lore raw
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Mike Snitzer

But while you're here, I wouldn't mind getting your take on virtio-blk setting max_hw_sectors to -1U.

Quoted 7 lines
Quoted 3 lines

As I said in my original reply to mst: it only makes sense to set a really high initial upper bound like that in a driver if that driver goes on to stack an underlying device's limit.

-1U should just work, IMHO, there's no reason we should need to cap it at some synthetic value. That said, it seems it should be one of those parameters that should be negotiated up and set appropriately.

I'm saying set it to the underlying device's value for max_hw_sectors -- not some synthetic value. So I think we're saying the same thing.

But it isn't immediately clear (to me) how that benefits virtio-blk users (obviously they are getting by today). So until that is pinned down I imagine nobody will care to extend the virtio-blk protocol to allow stacking max_hw_sectors and max_sectors up.