| Message ID | 20241209022033.288596-1-yangerkun@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | block: retry call probe after request_module in blk_request_module | expand |
On Mon, Dec 09, 2024 at 10:20:33AM +0800, Yang Erkun wrote: > Before commit e418de3abcda ("block: switch gendisk lookup to a simple > xarray"), open loop0 will success. lookup_gendisk will first use > base_probe to load module loop, and then the retry will call loop_probe > to prepare the loop disk. Finally open for this disk will success. > However, after this commit, we lose the retry logic, and open will fail > with ENXIO. Block device autoloading is deprecated and will be removed > soon, but maybe we should keep open success until we really remove it. > So, give a retry to fix it. This looks sensible. But can we clean the logic up a bit by adding a helper to make it easier to understand? Something like the untested patch below: diff --git a/block/genhd.c b/block/genhd.c index 6fcb09b8371d..654c2cc5d8e5 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -797,7 +797,7 @@ static ssize_t disk_badblocks_store(struct device *dev, } #ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD -void blk_request_module(dev_t devt) +static bool blk_probe_dev(dev_t devt) { unsigned int major = MAJOR(devt); struct blk_major_name **n; @@ -807,14 +807,26 @@ void blk_request_module(dev_t devt) if ((*n)->major == major && (*n)->probe) { (*n)->probe(devt); mutex_unlock(&major_names_lock); - return; + return true; } } mutex_unlock(&major_names_lock); + return false; +} + +void blk_request_module(dev_t devt) +{ + int error; + + if (blk_probe_dev(devt)) + return; - if (request_module("block-major-%d-%d", MAJOR(devt), MINOR(devt)) > 0) - /* Make old-style 2.4 aliases work */ - request_module("block-major-%d", MAJOR(devt)); + error = request_module("block-major-%d-%d", MAJOR(devt), MINOR(devt)); + /* Make old-style 2.4 aliases work */ + if (error > 0) + error = request_module("block-major-%d", MAJOR(devt)); + if (!error) + blk_probe_dev(devt); } #endif /* CONFIG_BLOCK_LEGACY_AUTOLOAD */
在 2024/12/9 16:14, Christoph Hellwig 写道: > On Mon, Dec 09, 2024 at 10:20:33AM +0800, Yang Erkun wrote: >> Before commit e418de3abcda ("block: switch gendisk lookup to a simple >> xarray"), open loop0 will success. lookup_gendisk will first use >> base_probe to load module loop, and then the retry will call loop_probe >> to prepare the loop disk. Finally open for this disk will success. >> However, after this commit, we lose the retry logic, and open will fail >> with ENXIO. Block device autoloading is deprecated and will be removed >> soon, but maybe we should keep open success until we really remove it. >> So, give a retry to fix it. > > This looks sensible. But can we clean the logic up a bit by adding > a helper to make it easier to understand? Something like the untested > patch below: Thanks for the suggest, your advice looks good to me and pass the test, I will resend this patch soon! > > diff --git a/block/genhd.c b/block/genhd.c > index 6fcb09b8371d..654c2cc5d8e5 100644 > --- a/block/genhd.c > +++ b/block/genhd.c > @@ -797,7 +797,7 @@ static ssize_t disk_badblocks_store(struct device *dev, > } > > #ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD > -void blk_request_module(dev_t devt) > +static bool blk_probe_dev(dev_t devt) > { > unsigned int major = MAJOR(devt); > struct blk_major_name **n; > @@ -807,14 +807,26 @@ void blk_request_module(dev_t devt) > if ((*n)->major == major && (*n)->probe) { > (*n)->probe(devt); > mutex_unlock(&major_names_lock); > - return; > + return true; > } > } > mutex_unlock(&major_names_lock); > + return false; > +} > + > +void blk_request_module(dev_t devt) > +{ > + int error; > + > + if (blk_probe_dev(devt)) > + return; > > - if (request_module("block-major-%d-%d", MAJOR(devt), MINOR(devt)) > 0) > - /* Make old-style 2.4 aliases work */ > - request_module("block-major-%d", MAJOR(devt)); > + error = request_module("block-major-%d-%d", MAJOR(devt), MINOR(devt)); > + /* Make old-style 2.4 aliases work */ > + if (error > 0) > + error = request_module("block-major-%d", MAJOR(devt)); > + if (!error) > + blk_probe_dev(devt); > } > #endif /* CONFIG_BLOCK_LEGACY_AUTOLOAD */ >
diff --git a/block/genhd.c b/block/genhd.c index 79230c109fca..950fdabaef2e 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -802,7 +802,10 @@ void blk_request_module(dev_t devt) { unsigned int major = MAJOR(devt); struct blk_major_name **n; + int retry = 0; + int error; +retry: mutex_lock(&major_names_lock); for (n = &major_names[major_to_index(major)]; *n; n = &(*n)->next) { if ((*n)->major == major && (*n)->probe) { @@ -813,9 +816,16 @@ void blk_request_module(dev_t devt) } mutex_unlock(&major_names_lock); - if (request_module("block-major-%d-%d", MAJOR(devt), MINOR(devt)) > 0) - /* Make old-style 2.4 aliases work */ - request_module("block-major-%d", MAJOR(devt)); + if (retry++) + return; + + error = request_module("block-major-%d-%d", MAJOR(devt), MINOR(devt)); + if (!error) + goto retry; + + /* Make old-style 2.4 aliases work */ + if (error > 0 && !request_module("block-major-%d", MAJOR(devt))) + goto retry; } #endif /* CONFIG_BLOCK_LEGACY_AUTOLOAD */
Set kernel config: CONFIG_BLK_DEV_LOOP=m CONFIG_BLK_DEV_LOOP_MIN_COUNT=0 Do latter: mknod loop0 b 7 0 exec 4<> loop0 Before commit e418de3abcda ("block: switch gendisk lookup to a simple xarray"), open loop0 will success. lookup_gendisk will first use base_probe to load module loop, and then the retry will call loop_probe to prepare the loop disk. Finally open for this disk will success. However, after this commit, we lose the retry logic, and open will fail with ENXIO. Block device autoloading is deprecated and will be removed soon, but maybe we should keep open success until we really remove it. So, give a retry to fix it. Fixes: e418de3abcda ("block: switch gendisk lookup to a simple xarray") Signed-off-by: Yang Erkun <yangerkun@huawei.com> --- block/genhd.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)