| Message ID | 20240410233932.256871-1-saranyamohan@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | block: Fix BLKRRPART regression | expand |
On Apr 10, 2024 / 23:39, Saranya Muruganandam wrote: > The BLKRRPART ioctl used to report errors such as EIO before we changed > the blkdev_reread_part() logic. > > Add a flag and capture the errors returned by bdev_disk_changed() > when the flag is set. Set this flag for the BLKRRPART path when we > want the errors to be reported when rereading partitions on the disk. > > Link: https://lore.kernel.org/all/20240320015134.GA14267@lst.de/ > Suggested-by: Christoph Hellwig <hch@lst.de> > Tested: Tested by simulating failure to the block device and will > propose a new test to blktests. > Fixes: 4601b4b130de ("block: reopen the device in blkdev_reread_part") > Reported-by: Saranya Muruganandam <saranyamohan@google.com> > Signed-off-by: Saranya Muruganandam <saranyamohan@google.com> The change looks good to me. I also confirmed the fix with the new, corresponding blktests test case. Reviewed-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
On 4/12/2024 12:12 AM, Shinichiro Kawasaki wrote: > On Apr 10, 2024 / 23:39, Saranya Muruganandam wrote: >> The BLKRRPART ioctl used to report errors such as EIO before we changed >> the blkdev_reread_part() logic. >> >> Add a flag and capture the errors returned by bdev_disk_changed() >> when the flag is set. Set this flag for the BLKRRPART path when we >> want the errors to be reported when rereading partitions on the disk. >> >> Link: https://lore.kernel.org/all/20240320015134.GA14267@lst.de/ >> Suggested-by: Christoph Hellwig <hch@lst.de> >> Tested: Tested by simulating failure to the block device and will >> propose a new test to blktests. >> Fixes: 4601b4b130de ("block: reopen the device in blkdev_reread_part") >> Reported-by: Saranya Muruganandam <saranyamohan@google.com> >> Signed-off-by: Saranya Muruganandam <saranyamohan@google.com> > > The change looks good to me. I also confirmed the fix with the new, > corresponding blktests test case. > > Reviewed-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> which patch is the final one ? Is it this one or the one posted with title :- "block: propagate partition scanning errors to the BLKRRPART ioctl​" ? Please clarify so I can make sure to run some tests at my end. -ck
On Apr 16, 2024 / 09:10, Chaitanya Kulkarni wrote: > On 4/12/2024 12:12 AM, Shinichiro Kawasaki wrote: > > On Apr 10, 2024 / 23:39, Saranya Muruganandam wrote: > >> The BLKRRPART ioctl used to report errors such as EIO before we changed > >> the blkdev_reread_part() logic. > >> > >> Add a flag and capture the errors returned by bdev_disk_changed() > >> when the flag is set. Set this flag for the BLKRRPART path when we > >> want the errors to be reported when rereading partitions on the disk. > >> > >> Link: https://lore.kernel.org/all/20240320015134.GA14267@lst.de/ > >> Suggested-by: Christoph Hellwig <hch@lst.de> > >> Tested: Tested by simulating failure to the block device and will > >> propose a new test to blktests. > >> Fixes: 4601b4b130de ("block: reopen the device in blkdev_reread_part") > >> Reported-by: Saranya Muruganandam <saranyamohan@google.com> > >> Signed-off-by: Saranya Muruganandam <saranyamohan@google.com> > > > > The change looks good to me. I also confirmed the fix with the new, > > corresponding blktests test case. > > > > Reviewed-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > which patch is the final one ? Is it this one or the one posted with > title :- > > "block: propagate partition scanning errors to the BLKRRPART ioctl​" ? > > Please clarify so I can make sure to run some tests at my end. The patch I reviewed and tested is the one that Saranya reflected my comment. It is found here: https://lore.kernel.org/linux-block/20240410233932.256871-1-saranyamohan@google.com/
On Tue, Apr 16, 2024 at 09:10:10AM +0000, Chaitanya Kulkarni wrote: > which patch is the final one ? Is it this one or the one posted with > title :- The correct patch is still the one I sent on Marc 28th and not any version that reattributes the changes and moves the flag into a totally weird place.
diff --git a/block/bdev.c b/block/bdev.c index 7a5f611c3d2e3..cea51dca87531 100644 --- a/block/bdev.c +++ b/block/bdev.c @@ -652,6 +652,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; @@ -670,20 +678,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 contexts + * 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 0c76137adcaaa..128f503828cee 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 c3e8f7cf96be9..d16320852c4ba 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 << 6)) struct gendisk { /*
The BLKRRPART ioctl used to report errors such as EIO before we changed the blkdev_reread_part() logic. Add a flag and capture the errors returned by bdev_disk_changed() when the flag is set. Set this flag for the BLKRRPART path when we want the errors to be reported when rereading partitions on the disk. Link: https://lore.kernel.org/all/20240320015134.GA14267@lst.de/ Suggested-by: Christoph Hellwig <hch@lst.de> Tested: Tested by simulating failure to the block device and will propose a new test to blktests. Fixes: 4601b4b130de ("block: reopen the device in blkdev_reread_part") Reported-by: Saranya Muruganandam <saranyamohan@google.com> Signed-off-by: Saranya Muruganandam <saranyamohan@google.com> Change-Id: Idf3d97390ed78061556f8468d10d6cab24ae20b1 --- block/bdev.c | 29 +++++++++++++++++++---------- block/ioctl.c | 3 ++- include/linux/blkdev.h | 2 ++ 3 files changed, 23 insertions(+), 11 deletions(-)