regression on BLKRRPART ioctl for EIO

regression on BLKRRPART ioctl for EIO

[Saranya Muruganandam]

Hi,

I am noticing a regression on the BLKRRPART ioctl after we changed the
blkdev_reread_part() logic to reopen the device with commit
68e6582e8f2dc32fd2458b9926564faa1fb4560e["reopen the device in
blkdev_reread_part"]

We now ignore the errors that used to be returned for
bdev_disk_changed(). I see that this was explicitly fixed for -EBUSY
(commit 68e6582e8f2dc32fd2458b9926564faa1fb4560e: block: return -EBUSY
when there are open partitions in blkdev_reread_part)

The regression I am particularly interested in is when we get an -EIO
error from the disk and BLKRRPART incorrectly reports success when it
actually failed to reread partitions.

Looking for advice.

Thanks,
Saranya

Re: regression on BLKRRPART ioctl for EIO

[Christoph Hellwig]

What scenario are you looking at?

Re: regression on BLKRRPART ioctl for EIO

[Saranya Muruganandam]

When we fail to read from the disk, BLKRRPART used to be able to
capture and bubble this up to the caller.
It no longer does since we no longer capture the error from bdev_disk_changed.

Here is an example with fault-injection:

# echo 0 > /sys/kernel/debug/fail_make_request/interval
# echo 100 > /sys/kernel/debug/fail_make_request/probability
# echo -1 > /sys/kernel/debug/fail_make_request/times
# echo 1 > /sys/block/sdc/make-it-fail
# blockdev --rereadpt /dev/sdc # no error
# echo $?
0 # incorrectly reports success.

Whereas fdisk and sfdisk correctly report the issue :

# sfdisk /dev/sdc
sfdisk: cannot open /dev/sdc: Input/output error
# fdisk /dev/sdc

Welcome to fdisk (util-linux 2.28.2).
Changes will remain in memory only, until you decide to write them.
Be careful before using the write command.

fdisk: cannot open /dev/sdc: Input/output error

Best,
Saranya



On Mon, Feb 12, 2024 at 8:00 PM Saranya Muruganandam
<saranyamohan@google.com> wrote:
>
> When we fail to read from the disk, BLKRRPART used to be able to capture and bubble this up to the caller.
> It no longer does since we no longer capture the error from bdev_disk_changed.
>
> Here is an example with fault-injection:
>
> # echo 0 > /sys/kernel/debug/fail_make_request/interval
> # echo 100 > /sys/kernel/debug/fail_make_request/probability
> # echo -1 > /sys/kernel/debug/fail_make_request/times
> # echo 1 > /sys/block/sdc/make-it-fail
> # blockdev --rereadpt /dev/sdc # no error
> # echo $?
> 0 # incorrectly reports success.
>
> Whereas fdisk and sfdisk correctly report the issue :
>
> # sfdisk /dev/sdc
> sfdisk: cannot open /dev/sdc: Input/output error
> # fdisk /dev/sdc
>
> Welcome to fdisk (util-linux 2.28.2).
> Changes will remain in memory only, until you decide to write them.
> Be careful before using the write command.
>
> fdisk: cannot open /dev/sdc: Input/output error
>
> Best,
> Saranya
>
> On Mon, Feb 12, 2024 at 7:44 AM Christoph Hellwig <hch@lst.de> wrote:
>>
>> What scenario are you looking at?

Re: regression on BLKRRPART ioctl for EIO

[Saranya Muruganandam]

Hi,
Is there advice on how to fix this `blockdev --rereadpt` regression in
the kernel?
Would appreciate some advice.

Thanks,
Saranya


On Mon, Feb 12, 2024 at 8:01 PM Saranya Muruganandam
<saranyamohan@google.com> wrote:
>
> When we fail to read from the disk, BLKRRPART used to be able to
> capture and bubble this up to the caller.
> It no longer does since we no longer capture the error from bdev_disk_changed.
>
> Here is an example with fault-injection:
>
> # echo 0 > /sys/kernel/debug/fail_make_request/interval
> # echo 100 > /sys/kernel/debug/fail_make_request/probability
> # echo -1 > /sys/kernel/debug/fail_make_request/times
> # echo 1 > /sys/block/sdc/make-it-fail
> # blockdev --rereadpt /dev/sdc # no error
> # echo $?
> 0 # incorrectly reports success.
>
> Whereas fdisk and sfdisk correctly report the issue :
>
> # sfdisk /dev/sdc
> sfdisk: cannot open /dev/sdc: Input/output error
> # fdisk /dev/sdc
>
> Welcome to fdisk (util-linux 2.28.2).
> Changes will remain in memory only, until you decide to write them.
> Be careful before using the write command.
>
> fdisk: cannot open /dev/sdc: Input/output error
>
> Best,
> Saranya
>
>
>
> On Mon, Feb 12, 2024 at 8:00 PM Saranya Muruganandam
> <saranyamohan@google.com> wrote:
> >
> > When we fail to read from the disk, BLKRRPART used to be able to capture and bubble this up to the caller.
> > It no longer does since we no longer capture the error from bdev_disk_changed.
> >
> > Here is an example with fault-injection:
> >
> > # echo 0 > /sys/kernel/debug/fail_make_request/interval
> > # echo 100 > /sys/kernel/debug/fail_make_request/probability
> > # echo -1 > /sys/kernel/debug/fail_make_request/times
> > # echo 1 > /sys/block/sdc/make-it-fail
> > # blockdev --rereadpt /dev/sdc # no error
> > # echo $?
> > 0 # incorrectly reports success.
> >
> > Whereas fdisk and sfdisk correctly report the issue :
> >
> > # sfdisk /dev/sdc
> > sfdisk: cannot open /dev/sdc: Input/output error
> > # fdisk /dev/sdc
> >
> > Welcome to fdisk (util-linux 2.28.2).
> > Changes will remain in memory only, until you decide to write them.
> > Be careful before using the write command.
> >
> > fdisk: cannot open /dev/sdc: Input/output error
> >
> > Best,
> > Saranya
> >
> > On Mon, Feb 12, 2024 at 7:44 AM Christoph Hellwig <hch@lst.de> wrote:
> >>
> >> What scenario are you looking at?

Re: regression on BLKRRPART ioctl for EIO

[Yu Kuai]

Hi,

在 2024/02/27 10:38, Saranya Muruganandam 写道:
> Hi,
> Is there advice on how to fix this `blockdev --rereadpt` regression in
> the kernel?
> Would appreciate some advice.
> 
> Thanks,
> Saranya
> 
> 
> On Mon, Feb 12, 2024 at 8:01 PM Saranya Muruganandam
> <saranyamohan@google.com> wrote:
>>
>> When we fail to read from the disk, BLKRRPART used to be able to
>> capture and bubble this up to the caller.
>> It no longer does since we no longer capture the error from bdev_disk_changed.
>>
>> Here is an example with fault-injection:
>>
>> # echo 0 > /sys/kernel/debug/fail_make_request/interval
>> # echo 100 > /sys/kernel/debug/fail_make_request/probability
>> # echo -1 > /sys/kernel/debug/fail_make_request/times
>> # echo 1 > /sys/block/sdc/make-it-fail
>> # blockdev --rereadpt /dev/sdc # no error
>> # echo $?
>> 0 # incorrectly reports success.

I take a look at this, and it's right the root cause is commit
4601b4b130de ("block: reopen the device in blkdev_reread_part") ignore
errors from bdev_disk_changed() now.

I think we can fix this by returning error from bdev_get_whole() if
bdev_disk_changed() failed, this will cause that open disk to fail now,
however, I think this can be acceptable.

Christoph, do you think so? Or we should distinguish ioctl and open
device and only let ioctl to fail.

Thanks,
Kuai


>>
>> Whereas fdisk and sfdisk correctly report the issue :
>>
>> # sfdisk /dev/sdc
>> sfdisk: cannot open /dev/sdc: Input/output error
>> # fdisk /dev/sdc
>>
>> Welcome to fdisk (util-linux 2.28.2).
>> Changes will remain in memory only, until you decide to write them.
>> Be careful before using the write command.
>>
>> fdisk: cannot open /dev/sdc: Input/output error
>>
>> Best,
>> Saranya
>>
>>
>>
>> On Mon, Feb 12, 2024 at 8:00 PM Saranya Muruganandam
>> <saranyamohan@google.com> wrote:
>>>
>>> When we fail to read from the disk, BLKRRPART used to be able to capture and bubble this up to the caller.
>>> It no longer does since we no longer capture the error from bdev_disk_changed.
>>>
>>> Here is an example with fault-injection:
>>>
>>> # echo 0 > /sys/kernel/debug/fail_make_request/interval
>>> # echo 100 > /sys/kernel/debug/fail_make_request/probability
>>> # echo -1 > /sys/kernel/debug/fail_make_request/times
>>> # echo 1 > /sys/block/sdc/make-it-fail
>>> # blockdev --rereadpt /dev/sdc # no error
>>> # echo $?
>>> 0 # incorrectly reports success.
>>>
>>> Whereas fdisk and sfdisk correctly report the issue :
>>>
>>> # sfdisk /dev/sdc
>>> sfdisk: cannot open /dev/sdc: Input/output error
>>> # fdisk /dev/sdc
>>>
>>> Welcome to fdisk (util-linux 2.28.2).
>>> Changes will remain in memory only, until you decide to write them.
>>> Be careful before using the write command.
>>>
>>> fdisk: cannot open /dev/sdc: Input/output error
>>>
>>> Best,
>>> Saranya
>>>
>>> On Mon, Feb 12, 2024 at 7:44 AM Christoph Hellwig <hch@lst.de> wrote:
>>>>
>>>> What scenario are you looking at?
> 
> .
> 

Re: regression on BLKRRPART ioctl for EIO

[Saranya Muruganandam]

> I think we can fix this by returning error from bdev_get_whole() if
> bdev_disk_changed() failed, this will cause that open disk to fail now,
> however, I think this can be acceptable.

Thanks for the response!
I agree this would fix the regression for the ioctl.
However, since returning an error from blkdev_get_whole is new
behavior, I wasn't sure what all parts it affects.

So this is just a ping to let you know that I am also waiting to hear
from Christoph.


On Mon, Feb 26, 2024 at 7:13 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> Hi,
>
> 在 2024/02/27 10:38, Saranya Muruganandam 写道:
> > Hi,
> > Is there advice on how to fix this `blockdev --rereadpt` regression in
> > the kernel?
> > Would appreciate some advice.
> >
> > Thanks,
> > Saranya
> >
> >
> > On Mon, Feb 12, 2024 at 8:01 PM Saranya Muruganandam
> > <saranyamohan@google.com> wrote:
> >>
> >> When we fail to read from the disk, BLKRRPART used to be able to
> >> capture and bubble this up to the caller.
> >> It no longer does since we no longer capture the error from bdev_disk_changed.
> >>
> >> Here is an example with fault-injection:
> >>
> >> # echo 0 > /sys/kernel/debug/fail_make_request/interval
> >> # echo 100 > /sys/kernel/debug/fail_make_request/probability
> >> # echo -1 > /sys/kernel/debug/fail_make_request/times
> >> # echo 1 > /sys/block/sdc/make-it-fail
> >> # blockdev --rereadpt /dev/sdc # no error
> >> # echo $?
> >> 0 # incorrectly reports success.
>
> I take a look at this, and it's right the root cause is commit
> 4601b4b130de ("block: reopen the device in blkdev_reread_part") ignore
> errors from bdev_disk_changed() now.
>
> I think we can fix this by returning error from bdev_get_whole() if
> bdev_disk_changed() failed, this will cause that open disk to fail now,
> however, I think this can be acceptable.
>
> Christoph, do you think so? Or we should distinguish ioctl and open
> device and only let ioctl to fail.
>
> Thanks,
> Kuai
>
>
> >>
> >> Whereas fdisk and sfdisk correctly report the issue :
> >>
> >> # sfdisk /dev/sdc
> >> sfdisk: cannot open /dev/sdc: Input/output error
> >> # fdisk /dev/sdc
> >>
> >> Welcome to fdisk (util-linux 2.28.2).
> >> Changes will remain in memory only, until you decide to write them.
> >> Be careful before using the write command.
> >>
> >> fdisk: cannot open /dev/sdc: Input/output error
> >>
> >> Best,
> >> Saranya
> >>
> >>
> >>
> >> On Mon, Feb 12, 2024 at 8:00 PM Saranya Muruganandam
> >> <saranyamohan@google.com> wrote:
> >>>
> >>> When we fail to read from the disk, BLKRRPART used to be able to capture and bubble this up to the caller.
> >>> It no longer does since we no longer capture the error from bdev_disk_changed.
> >>>
> >>> Here is an example with fault-injection:
> >>>
> >>> # echo 0 > /sys/kernel/debug/fail_make_request/interval
> >>> # echo 100 > /sys/kernel/debug/fail_make_request/probability
> >>> # echo -1 > /sys/kernel/debug/fail_make_request/times
> >>> # echo 1 > /sys/block/sdc/make-it-fail
> >>> # blockdev --rereadpt /dev/sdc # no error
> >>> # echo $?
> >>> 0 # incorrectly reports success.
> >>>
> >>> Whereas fdisk and sfdisk correctly report the issue :
> >>>
> >>> # sfdisk /dev/sdc
> >>> sfdisk: cannot open /dev/sdc: Input/output error
> >>> # fdisk /dev/sdc
> >>>
> >>> Welcome to fdisk (util-linux 2.28.2).
> >>> Changes will remain in memory only, until you decide to write them.
> >>> Be careful before using the write command.
> >>>
> >>> fdisk: cannot open /dev/sdc: Input/output error
> >>>
> >>> Best,
> >>> Saranya
> >>>
> >>> On Mon, Feb 12, 2024 at 7:44 AM Christoph Hellwig <hch@lst.de> wrote:
> >>>>
> >>>> What scenario are you looking at?
> >
> > .
> >
>
>

Re: regression on BLKRRPART ioctl for EIO

[Christoph Hellwig]

On Thu, Mar 07, 2024 at 10:14:05AM -0800, Saranya Muruganandam wrote:
> > I think we can fix this by returning error from bdev_get_whole() if
> > bdev_disk_changed() failed, this will cause that open disk to fail now,
> > however, I think this can be acceptable.
> 
> Thanks for the response!
> I agree this would fix the regression for the ioctl.
> However, since returning an error from blkdev_get_whole is new
> behavior, I wasn't sure what all parts it affects.
> 
> So this is just a ping to let you know that I am also waiting to hear
> from Christoph.

I'd hate returning a failure and changing the interface, but I haven't
come up with anything better yet.  Let me thing about this a bit more.

Re: regression on BLKRRPART ioctl for EIO

[Saranya Muruganandam]

Any advice yet?

I really appreciate the help.

thanks,
Saranya

On Fri, Mar 8, 2024 at 8:32 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Mar 07, 2024 at 10:14:05AM -0800, Saranya Muruganandam wrote:
> > > I think we can fix this by returning error from bdev_get_whole() if
> > > bdev_disk_changed() failed, this will cause that open disk to fail now,
> > > however, I think this can be acceptable.
> >
> > Thanks for the response!
> > I agree this would fix the regression for the ioctl.
> > However, since returning an error from blkdev_get_whole is new
> > behavior, I wasn't sure what all parts it affects.
> >
> > So this is just a ping to let you know that I am also waiting to hear
> > from Christoph.
>
> I'd hate returning a failure and changing the interface, but I haven't
> come up with anything better yet.  Let me thing about this a bit more.

Re: regression on BLKRRPART ioctl for EIO

[Christoph Hellwig]

Please try the patch below:

I would also relly help to have a blktsts test case to show your
issue and verify that it is fixed.

diff --git a/block/bdev.c b/block/bdev.c
index e7adaaf1c21927..51071d371863e0 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -645,6 +645,14 @@ static void blkdev_flush_mapping(struct block_device *bdev)
 	bdev_write_inode(bdev);
 }
 
+static void blkdev_put_whole(struct block_device *bdev)
+{
+	if (atomic_dec_and_test(&bdev->bd_openers))
+		blkdev_flush_mapping(bdev);
+	if (bdev->bd_disk->fops->release)
+		bdev->bd_disk->fops->release(bdev->bd_disk);
+}
+
 static int blkdev_get_whole(struct block_device *bdev, blk_mode_t mode)
 {
 	struct gendisk *disk = bdev->bd_disk;
@@ -663,20 +671,21 @@ static int blkdev_get_whole(struct block_device *bdev, blk_mode_t mode)
 
 	if (!atomic_read(&bdev->bd_openers))
 		set_init_blocksize(bdev);
-	if (test_bit(GD_NEED_PART_SCAN, &disk->state))
-		bdev_disk_changed(disk, false);
 	atomic_inc(&bdev->bd_openers);
+	if (test_bit(GD_NEED_PART_SCAN, &disk->state)) {
+		/*
+		 * Only return scanning errors if we are called from conexts
+		 * that explicitly want them, e.g. the BLKRRPART ioctl.
+		 */
+		ret = bdev_disk_changed(disk, false);
+		if (ret && (mode & BLK_OPEN_STRICT_SCAN)) {
+			blkdev_put_whole(bdev);
+			return ret;
+		}
+	}
 	return 0;
 }
 
-static void blkdev_put_whole(struct block_device *bdev)
-{
-	if (atomic_dec_and_test(&bdev->bd_openers))
-		blkdev_flush_mapping(bdev);
-	if (bdev->bd_disk->fops->release)
-		bdev->bd_disk->fops->release(bdev->bd_disk);
-}
-
 static int blkdev_get_part(struct block_device *part, blk_mode_t mode)
 {
 	struct gendisk *disk = part->bd_disk;
diff --git a/block/ioctl.c b/block/ioctl.c
index 0c76137adcaaa5..128f503828cee7 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -562,7 +562,8 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
 			return -EACCES;
 		if (bdev_is_partition(bdev))
 			return -EINVAL;
-		return disk_scan_partitions(bdev->bd_disk, mode);
+		return disk_scan_partitions(bdev->bd_disk,
+				mode | BLK_OPEN_STRICT_SCAN);
 	case BLKTRACESTART:
 	case BLKTRACESTOP:
 	case BLKTRACETEARDOWN:
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index f9b87c39cab047..272ce42f297cfe 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -128,6 +128,8 @@ typedef unsigned int __bitwise blk_mode_t;
 #define BLK_OPEN_WRITE_IOCTL	((__force blk_mode_t)(1 << 4))
 /* open is exclusive wrt all other BLK_OPEN_WRITE opens to the device */
 #define BLK_OPEN_RESTRICT_WRITES	((__force blk_mode_t)(1 << 5))
+/* return partition scanning errors */
+#define BLK_OPEN_STRICT_SCAN	((__force blk_mode_t)(1 << 5))
 
 struct gendisk {
 	/*

Re: regression on BLKRRPART ioctl for EIO

[Saranya Muruganandam]

Hi Christoph,
Apologies for the delay.

Thanks a ton for your patch! Your patch solution works for me.
I sent https://lore.kernel.org/all/20240405014253.748627-1-saranyamohan@google.com/
for the block layer patch
and https://lore.kernel.org/all/20240405015657.751659-1-saranyamohan@google.com/
for the blktest added.

Once I get confirmation that they look good, I have the changes for
the older LTS which I can share.
This patch doesn't cleanly apply but your idea of flagging and using
that to return errors, fixes the regression
for stable kernels like 5.10.

Thanks again.

-Saranya


On Tue, Mar 19, 2024 at 6:51 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Please try the patch below:
>
> I would also relly help to have a blktsts test case to show your
> issue and verify that it is fixed.
>
> diff --git a/block/bdev.c b/block/bdev.c
> index e7adaaf1c21927..51071d371863e0 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -645,6 +645,14 @@ static void blkdev_flush_mapping(struct block_device *bdev)
>         bdev_write_inode(bdev);
>  }
>
> +static void blkdev_put_whole(struct block_device *bdev)
> +{
> +       if (atomic_dec_and_test(&bdev->bd_openers))
> +               blkdev_flush_mapping(bdev);
> +       if (bdev->bd_disk->fops->release)
> +               bdev->bd_disk->fops->release(bdev->bd_disk);
> +}
> +
>  static int blkdev_get_whole(struct block_device *bdev, blk_mode_t mode)
>  {
>         struct gendisk *disk = bdev->bd_disk;
> @@ -663,20 +671,21 @@ static int blkdev_get_whole(struct block_device *bdev, blk_mode_t mode)
>
>         if (!atomic_read(&bdev->bd_openers))
>                 set_init_blocksize(bdev);
> -       if (test_bit(GD_NEED_PART_SCAN, &disk->state))
> -               bdev_disk_changed(disk, false);
>         atomic_inc(&bdev->bd_openers);
> +       if (test_bit(GD_NEED_PART_SCAN, &disk->state)) {
> +               /*
> +                * Only return scanning errors if we are called from conexts
> +                * that explicitly want them, e.g. the BLKRRPART ioctl.
> +                */
> +               ret = bdev_disk_changed(disk, false);
> +               if (ret && (mode & BLK_OPEN_STRICT_SCAN)) {
> +                       blkdev_put_whole(bdev);
> +                       return ret;
> +               }
> +       }
>         return 0;
>  }
>
> -static void blkdev_put_whole(struct block_device *bdev)
> -{
> -       if (atomic_dec_and_test(&bdev->bd_openers))
> -               blkdev_flush_mapping(bdev);
> -       if (bdev->bd_disk->fops->release)
> -               bdev->bd_disk->fops->release(bdev->bd_disk);
> -}
> -
>  static int blkdev_get_part(struct block_device *part, blk_mode_t mode)
>  {
>         struct gendisk *disk = part->bd_disk;
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 0c76137adcaaa5..128f503828cee7 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -562,7 +562,8 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
>                         return -EACCES;
>                 if (bdev_is_partition(bdev))
>                         return -EINVAL;
> -               return disk_scan_partitions(bdev->bd_disk, mode);
> +               return disk_scan_partitions(bdev->bd_disk,
> +                               mode | BLK_OPEN_STRICT_SCAN);
>         case BLKTRACESTART:
>         case BLKTRACESTOP:
>         case BLKTRACETEARDOWN:
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index f9b87c39cab047..272ce42f297cfe 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -128,6 +128,8 @@ typedef unsigned int __bitwise blk_mode_t;
>  #define BLK_OPEN_WRITE_IOCTL   ((__force blk_mode_t)(1 << 4))
>  /* open is exclusive wrt all other BLK_OPEN_WRITE opens to the device */
>  #define BLK_OPEN_RESTRICT_WRITES       ((__force blk_mode_t)(1 << 5))
> +/* return partition scanning errors */
> +#define BLK_OPEN_STRICT_SCAN   ((__force blk_mode_t)(1 << 5))
>
>  struct gendisk {
>         /*