Re: [PATCH 05/10] block: do not reassig ->bi_bdev when partition remapping

On 1/25/21 10:57 AM, Jens Axboe wrote:

On 1/25/21 10:55 AM, Christoph Hellwig wrote:

quoted

On Mon, Jan 25, 2021 at 10:53:36AM -0700, Jens Axboe wrote:

quoted

On Sun, Jan 24, 2021 at 3:05 AM Christoph Hellwig [off-list ref] wrote:

quoted

There is no good reason to reassign ->bi_bdev when remapping the
partition-relative block number to the device wide one, as all the
information required by the drivers comes from the gendisk anyway.

Keeping the original ->bi_bdev alive will allow to greatly simplify
the partition-away I/O accounting.

This one causes boot failures for me on my laptop...

Any output to share?  Details of the config?

.config attached - hard to share any output, as it seems to load the
kernel fine, but latter parts of the boot process end up with errors
after the screen blanks. It's a laptop, so nothing fancy in terms of
logging. FWIW, it did boot on my test box.

Let me try and do a video...

Here you go, complaining about EOD access on partition 6.

sudo fdisk -l /dev/nvme0n1
Disk /dev/nvme0n1: 953.89 GiB, 1024209543168 bytes, 2000409264 sectors
Disk model: SAMSUNG MZVLB1T0HBLR-000L7              
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: gpt
Disk identifier: 51484599-D608-465A-A3A9-D36F1D6D126E

Device               Start        End   Sectors   Size Type
/dev/nvme0n1p1        2048     534527    532480   260M EFI System
/dev/nvme0n1p2      534528     567295     32768    16M Microsoft reserved
/dev/nvme0n1p3      567296  257560575 256993280 122.6G Microsoft basic data
/dev/nvme0n1p4  1998360576 2000408575   2048000  1000M Windows recovery environment
/dev/nvme0n1p5   257560576  261718015   4157440     2G Linux filesystem
/dev/nvme0n1p6   261718016  390625279 128907264  61.5G Linux filesystem
/dev/nvme0n1p7   390625280  421875711  31250432  14.9G Linux swap
/dev/nvme0n1p8   421875712 1007812607 585936896 279.4G Linux filesystem
/dev/nvme0n1p9  1007812608 1398437887 390625280 186.3G Linux filesystem
/dev/nvme0n1p10 1398437888 1593749503 195311616  93.1G Linux filesystem

Partition table entries are not in disk order.

-- 
Jens Axboe

On 1/25/21 11:13 AM, Christoph Hellwig wrote:

On Mon, Jan 25, 2021 at 11:03:24AM -0700, Jens Axboe wrote:

quoted

Partition table entries are not in disk order.

And the issue shows up with the series just up to the this patch,
without any later patches?

At that patch specifically. I bisected it, and then I double checked
by running the previous commit (boots fine), then apply this one, and
then I run into that error. So it should be 100% reliable.

-- 
Jens Axboe

On Mon, Jan 25, 2021 at 11:03:24AM -0700, Jens Axboe wrote:

Partition table entries are not in disk order.

And the issue shows up with the series just up to the this patch,
without any later patches?

On Mon, Jan 25, 2021 at 11:15:04AM -0700, Jens Axboe wrote:

On 1/25/21 11:13 AM, Christoph Hellwig wrote:

quoted

On Mon, Jan 25, 2021 at 11:03:24AM -0700, Jens Axboe wrote:

quoted

Partition table entries are not in disk order.

And the issue shows up with the series just up to the this patch,
without any later patches?

At that patch specifically. I bisected it, and then I double checked
by running the previous commit (boots fine), then apply this one, and
then I run into that error. So it should be 100% reliable.

Ok, I have an idea.  With EOD message you mean this printk, right:

	pr_info_ratelimited("attempt to access beyond end of device\n"
                            "%s: rw=%d, want=%llu, limit=%llu\n",
			    ...

right?

On 1/25/21 11:18 AM, Christoph Hellwig wrote:

On Mon, Jan 25, 2021 at 11:15:04AM -0700, Jens Axboe wrote:

quoted

On 1/25/21 11:13 AM, Christoph Hellwig wrote:

quoted

On Mon, Jan 25, 2021 at 11:03:24AM -0700, Jens Axboe wrote:

quoted

Partition table entries are not in disk order.

And the issue shows up with the series just up to the this patch,
without any later patches?

At that patch specifically. I bisected it, and then I double checked
by running the previous commit (boots fine), then apply this one, and
then I run into that error. So it should be 100% reliable.

Ok, I have an idea.  With EOD message you mean this printk, right:

	pr_info_ratelimited("attempt to access beyond end of device\n"
                            "%s: rw=%d, want=%llu, limit=%llu\n",
			    ...

right?

Yep

-- 
Jens Axboe

On Mon, Jan 25, 2021 at 11:19:23AM -0700, Jens Axboe wrote:

On 1/25/21 11:18 AM, Christoph Hellwig wrote:

quoted

On Mon, Jan 25, 2021 at 11:15:04AM -0700, Jens Axboe wrote:

quoted

On 1/25/21 11:13 AM, Christoph Hellwig wrote:

quoted

On Mon, Jan 25, 2021 at 11:03:24AM -0700, Jens Axboe wrote:

quoted

Partition table entries are not in disk order.

And the issue shows up with the series just up to the this patch,
without any later patches?

At that patch specifically. I bisected it, and then I double checked
by running the previous commit (boots fine), then apply this one, and
then I run into that error. So it should be 100% reliable.

Ok, I have an idea.  With EOD message you mean this printk, right:

	pr_info_ratelimited("attempt to access beyond end of device\n"
                            "%s: rw=%d, want=%llu, limit=%llu\n",
			    ...

right?

Yep

Can you give this untested patch a spin?  This should fix the
case where we check the eod for the original partition with the
remapped bi_sectors.  Looking into a local reproducer now.

diff --git a/block/blk-core.c b/block/blk-core.c
index 88f60890443264..6253a2f9a1c08f 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c

@@ -813,13 +813,16 @@ static noinline_for_stack bool submit_bio_checks(struct bio *bio)
 
 	if (should_fail_bio(bio))
 		goto end_io;
+
 	if (unlikely(bio_check_ro(bio)))
 		goto end_io;
-	if (unlikely(bio_check_eod(bio)))
-		goto end_io;
-	if (bio->bi_bdev->bd_partno && !bio_flagged(bio, BIO_REMAPPED) &&
-	    unlikely(blk_partition_remap(bio)))
-		goto end_io;
+	if (!bio_flagged(bio, BIO_REMAPPED)) {
+		if (unlikely(bio_check_eod(bio)))
+			goto end_io;
+		if (bio->bi_bdev->bd_partno &&
+		    unlikely(blk_partition_remap(bio)))
+			goto end_io;
+	}
 
 	/*
 	 * Filter flush bio's early so that bio based drivers without flush

On 1/25/21 11:21 AM, Christoph Hellwig wrote:

On Mon, Jan 25, 2021 at 11:19:23AM -0700, Jens Axboe wrote:

quoted

On 1/25/21 11:18 AM, Christoph Hellwig wrote:

quoted

On Mon, Jan 25, 2021 at 11:15:04AM -0700, Jens Axboe wrote:

quoted

On 1/25/21 11:13 AM, Christoph Hellwig wrote:

quoted

On Mon, Jan 25, 2021 at 11:03:24AM -0700, Jens Axboe wrote:

quoted

Partition table entries are not in disk order.

And the issue shows up with the series just up to the this patch,
without any later patches?

At that patch specifically. I bisected it, and then I double checked
by running the previous commit (boots fine), then apply this one, and
then I run into that error. So it should be 100% reliable.

Ok, I have an idea.  With EOD message you mean this printk, right:

	pr_info_ratelimited("attempt to access beyond end of device\n"
                            "%s: rw=%d, want=%llu, limit=%llu\n",
			    ...

right?

Yep

Can you give this untested patch a spin?  This should fix the
case where we check the eod for the original partition with the
remapped bi_sectors.  Looking into a local reproducer now.

Yep, with that applied on top my laptop boots again.

-- 
Jens Axboe