* [PATCH 0/3] block: simple *add_disk*() driver conversions
@ 2021-07-15 19:51 Luis Chamberlain
2021-07-15 19:51 ` [PATCH 1/3] loop: add error handling support for add_disk() Luis Chamberlain
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Luis Chamberlain @ 2021-07-15 19:51 UTC (permalink / raw
To: axboe
Cc: hare, bvanassche, ming.lei, hch, jack, osandov, linux-block,
linux-kernel, Luis Chamberlain
As requested by Christoph, I'm going to add a few demo driver
conversions, to show how drivers can manage *add_disk*() error
handling.
I've converted all drivers at this point, but I think it makes sense to
split these conversion up into groups. This is the first group, which
demos trivial changes. This group is larger than this, but this series
with 3 driver examples should suffice to start review on that group type.
If this seems fine and the block changes get merged I can extend this
group later to include all trivial driver conversions. But patch review
on these would help to get started.
Luis Chamberlain (3):
loop: add error handling support for add_disk()
null_blk: add error handling support for add_disk()
nbd: add error handling support for add_disk()
drivers/block/loop.c | 9 ++++++++-
drivers/block/nbd.c | 6 +++++-
drivers/block/null_blk/main.c | 3 +--
3 files changed, 14 insertions(+), 4 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/3] loop: add error handling support for add_disk()
2021-07-15 19:51 [PATCH 0/3] block: simple *add_disk*() driver conversions Luis Chamberlain
@ 2021-07-15 19:51 ` Luis Chamberlain
2021-07-16 0:47 ` kernel test robot
2021-07-15 19:51 ` [PATCH 2/3] null_blk: " Luis Chamberlain
2021-07-15 19:51 ` [PATCH 3/3] nbd: " Luis Chamberlain
2 siblings, 1 reply; 7+ messages in thread
From: Luis Chamberlain @ 2021-07-15 19:51 UTC (permalink / raw
To: axboe
Cc: hare, bvanassche, ming.lei, hch, jack, osandov, linux-block,
linux-kernel, Luis Chamberlain
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/block/loop.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index f37b9e3d833c..efbd8e29aca7 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2326,10 +2326,17 @@ static int loop_add(int i)
disk->private_data = lo;
disk->queue = lo->lo_queue;
sprintf(disk->disk_name, "loop%d", i);
- add_disk(disk);
+
+ err = add_disk(disk);
+ if (err)
+ goto out_cleanup_disk;
+
mutex_unlock(&loop_ctl_mutex);
+
return i;
+out_cleanup_disk:
+ blk_cleanup_disk(disk);
out_cleanup_tags:
blk_mq_free_tag_set(&lo->tag_set);
out_free_idr:
--
2.27.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] null_blk: add error handling support for add_disk()
2021-07-15 19:51 [PATCH 0/3] block: simple *add_disk*() driver conversions Luis Chamberlain
2021-07-15 19:51 ` [PATCH 1/3] loop: add error handling support for add_disk() Luis Chamberlain
@ 2021-07-15 19:51 ` Luis Chamberlain
2021-07-16 2:13 ` kernel test robot
2021-07-16 2:49 ` kernel test robot
2021-07-15 19:51 ` [PATCH 3/3] nbd: " Luis Chamberlain
2 siblings, 2 replies; 7+ messages in thread
From: Luis Chamberlain @ 2021-07-15 19:51 UTC (permalink / raw
To: axboe
Cc: hare, bvanassche, ming.lei, hch, jack, osandov, linux-block,
linux-kernel, Luis Chamberlain
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling. The actual cleanup in case of error is
already handled by the caller of null_gendisk_register().
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/block/null_blk/main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index d734e9ee1546..2a8f3eee7159 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1721,8 +1721,7 @@ static int null_gendisk_register(struct nullb *nullb)
return ret;
}
- add_disk(disk);
- return 0;
+ return add_disk(disk);
}
static int null_init_tag_set(struct nullb *nullb, struct blk_mq_tag_set *set)
--
2.27.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] nbd: add error handling support for add_disk()
2021-07-15 19:51 [PATCH 0/3] block: simple *add_disk*() driver conversions Luis Chamberlain
2021-07-15 19:51 ` [PATCH 1/3] loop: add error handling support for add_disk() Luis Chamberlain
2021-07-15 19:51 ` [PATCH 2/3] null_blk: " Luis Chamberlain
@ 2021-07-15 19:51 ` Luis Chamberlain
2 siblings, 0 replies; 7+ messages in thread
From: Luis Chamberlain @ 2021-07-15 19:51 UTC (permalink / raw
To: axboe
Cc: hare, bvanassche, ming.lei, hch, jack, osandov, linux-block,
linux-kernel, Luis Chamberlain
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/block/nbd.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index b7d663736d35..0ecc1e4e16fd 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1730,10 +1730,14 @@ static int nbd_dev_add(int index)
disk->fops = &nbd_fops;
disk->private_data = nbd;
sprintf(disk->disk_name, "nbd%d", index);
- add_disk(disk);
+ err = add_disk(disk);
+ if (err)
+ goto out_err_disk;
nbd_total_devices++;
return index;
+out_err_disk:
+ blk_cleanup_disk(disk);
out_free_idr:
idr_remove(&nbd_index_idr, index);
out_free_tags:
--
2.27.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] loop: add error handling support for add_disk()
2021-07-15 19:51 ` [PATCH 1/3] loop: add error handling support for add_disk() Luis Chamberlain
@ 2021-07-16 0:47 ` kernel test robot
0 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2021-07-16 0:47 UTC (permalink / raw
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 7016 bytes --]
Hi Luis,
I love your patch! Yet something to improve:
[auto build test ERROR on block/for-next]
[also build test ERROR on v5.14-rc1 next-20210715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Luis-Chamberlain/block-simple-add_disk-driver-conversions/20210716-035618
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: h8300-randconfig-r032-20210715 (attached as .config)
compiler: h8300-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/bc7835fa74d828de37e4f8a676d8dd45160299db
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Luis-Chamberlain/block-simple-add_disk-driver-conversions/20210716-035618
git checkout bc7835fa74d828de37e4f8a676d8dd45160299db
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=h8300 SHELL=/bin/bash drivers/block/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from include/linux/kernel.h:11,
from include/linux/list.h:9,
from include/linux/module.h:12,
from drivers/block/loop.c:52:
include/linux/scatterlist.h: In function 'sg_set_buf':
include/asm-generic/page.h:89:50: warning: ordered comparison of pointer with null pointer [-Wextra]
89 | #define virt_addr_valid(kaddr) (((void *)(kaddr) >= (void *)PAGE_OFFSET) && \
| ^~
include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
78 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
include/linux/scatterlist.h:137:2: note: in expansion of macro 'BUG_ON'
137 | BUG_ON(!virt_addr_valid(buf));
| ^~~~~~
include/linux/scatterlist.h:137:10: note: in expansion of macro 'virt_addr_valid'
137 | BUG_ON(!virt_addr_valid(buf));
| ^~~~~~~~~~~~~~~
drivers/block/loop.c: In function 'loop_add':
>> drivers/block/loop.c:2330:6: error: void value not ignored as it ought to be
2330 | err = add_disk(disk);
| ^
vim +2330 drivers/block/loop.c
2237
2238 static int loop_add(int i)
2239 {
2240 struct loop_device *lo;
2241 struct gendisk *disk;
2242 int err;
2243
2244 err = -ENOMEM;
2245 lo = kzalloc(sizeof(*lo), GFP_KERNEL);
2246 if (!lo)
2247 goto out;
2248 lo->lo_state = Lo_unbound;
2249
2250 err = mutex_lock_killable(&loop_ctl_mutex);
2251 if (err)
2252 goto out_free_dev;
2253
2254 /* allocate id, if @id >= 0, we're requesting that specific id */
2255 if (i >= 0) {
2256 err = idr_alloc(&loop_index_idr, lo, i, i + 1, GFP_KERNEL);
2257 if (err == -ENOSPC)
2258 err = -EEXIST;
2259 } else {
2260 err = idr_alloc(&loop_index_idr, lo, 0, 0, GFP_KERNEL);
2261 }
2262 if (err < 0)
2263 goto out_unlock;
2264 i = err;
2265
2266 err = -ENOMEM;
2267 lo->tag_set.ops = &loop_mq_ops;
2268 lo->tag_set.nr_hw_queues = 1;
2269 lo->tag_set.queue_depth = 128;
2270 lo->tag_set.numa_node = NUMA_NO_NODE;
2271 lo->tag_set.cmd_size = sizeof(struct loop_cmd);
2272 lo->tag_set.flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
2273 lo->tag_set.driver_data = lo;
2274
2275 err = blk_mq_alloc_tag_set(&lo->tag_set);
2276 if (err)
2277 goto out_free_idr;
2278
2279 disk = lo->lo_disk = blk_mq_alloc_disk(&lo->tag_set, lo);
2280 if (IS_ERR(disk)) {
2281 err = PTR_ERR(disk);
2282 goto out_cleanup_tags;
2283 }
2284 lo->lo_queue = lo->lo_disk->queue;
2285
2286 blk_queue_max_hw_sectors(lo->lo_queue, BLK_DEF_MAX_SECTORS);
2287
2288 /*
2289 * By default, we do buffer IO, so it doesn't make sense to enable
2290 * merge because the I/O submitted to backing file is handled page by
2291 * page. For directio mode, merge does help to dispatch bigger request
2292 * to underlayer disk. We will enable merge once directio is enabled.
2293 */
2294 blk_queue_flag_set(QUEUE_FLAG_NOMERGES, lo->lo_queue);
2295
2296 /*
2297 * Disable partition scanning by default. The in-kernel partition
2298 * scanning can be requested individually per-device during its
2299 * setup. Userspace can always add and remove partitions from all
2300 * devices. The needed partition minors are allocated from the
2301 * extended minor space, the main loop device numbers will continue
2302 * to match the loop minors, regardless of the number of partitions
2303 * used.
2304 *
2305 * If max_part is given, partition scanning is globally enabled for
2306 * all loop devices. The minors for the main loop devices will be
2307 * multiples of max_part.
2308 *
2309 * Note: Global-for-all-devices, set-only-at-init, read-only module
2310 * parameteters like 'max_loop' and 'max_part' make things needlessly
2311 * complicated, are too static, inflexible and may surprise
2312 * userspace tools. Parameters like this in general should be avoided.
2313 */
2314 if (!part_shift)
2315 disk->flags |= GENHD_FL_NO_PART_SCAN;
2316 disk->flags |= GENHD_FL_EXT_DEVT;
2317 atomic_set(&lo->lo_refcnt, 0);
2318 mutex_init(&lo->lo_mutex);
2319 lo->lo_number = i;
2320 spin_lock_init(&lo->lo_lock);
2321 spin_lock_init(&lo->lo_work_lock);
2322 disk->major = LOOP_MAJOR;
2323 disk->first_minor = i << part_shift;
2324 disk->minors = 1 << part_shift;
2325 disk->fops = &lo_fops;
2326 disk->private_data = lo;
2327 disk->queue = lo->lo_queue;
2328 sprintf(disk->disk_name, "loop%d", i);
2329
> 2330 err = add_disk(disk);
2331 if (err)
2332 goto out_cleanup_disk;
2333
2334 mutex_unlock(&loop_ctl_mutex);
2335
2336 return i;
2337
2338 out_cleanup_disk:
2339 blk_cleanup_disk(disk);
2340 out_cleanup_tags:
2341 blk_mq_free_tag_set(&lo->tag_set);
2342 out_free_idr:
2343 idr_remove(&loop_index_idr, i);
2344 out_unlock:
2345 mutex_unlock(&loop_ctl_mutex);
2346 out_free_dev:
2347 kfree(lo);
2348 out:
2349 return err;
2350 }
2351
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 21469 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] null_blk: add error handling support for add_disk()
2021-07-15 19:51 ` [PATCH 2/3] null_blk: " Luis Chamberlain
@ 2021-07-16 2:13 ` kernel test robot
2021-07-16 2:49 ` kernel test robot
1 sibling, 0 replies; 7+ messages in thread
From: kernel test robot @ 2021-07-16 2:13 UTC (permalink / raw
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 4052 bytes --]
Hi Luis,
I love your patch! Yet something to improve:
[auto build test ERROR on block/for-next]
[also build test ERROR on v5.14-rc1 next-20210715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Luis-Chamberlain/block-simple-add_disk-driver-conversions/20210716-035618
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: h8300-randconfig-r032-20210715 (attached as .config)
compiler: h8300-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/df9eb8cd323290580b5c3c6b2bed0261e67bc16b
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Luis-Chamberlain/block-simple-add_disk-driver-conversions/20210716-035618
git checkout df9eb8cd323290580b5c3c6b2bed0261e67bc16b
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=h8300 SHELL=/bin/bash drivers/block/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from include/linux/kernel.h:11,
from include/linux/list.h:9,
from include/linux/module.h:12,
from drivers/block/null_blk/main.c:6:
include/linux/scatterlist.h: In function 'sg_set_buf':
include/asm-generic/page.h:89:50: warning: ordered comparison of pointer with null pointer [-Wextra]
89 | #define virt_addr_valid(kaddr) (((void *)(kaddr) >= (void *)PAGE_OFFSET) && \
| ^~
include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
78 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
include/linux/scatterlist.h:137:2: note: in expansion of macro 'BUG_ON'
137 | BUG_ON(!virt_addr_valid(buf));
| ^~~~~~
include/linux/scatterlist.h:137:10: note: in expansion of macro 'virt_addr_valid'
137 | BUG_ON(!virt_addr_valid(buf));
| ^~~~~~~~~~~~~~~
drivers/block/null_blk/main.c: In function 'null_gendisk_register':
>> drivers/block/null_blk/main.c:1724:9: error: void value not ignored as it ought to be
1724 | return add_disk(disk);
| ^~~~~~~~~~~~~~
drivers/block/null_blk/main.c:1725:1: error: control reaches end of non-void function [-Werror=return-type]
1725 | }
| ^
cc1: some warnings being treated as errors
vim +1724 drivers/block/null_blk/main.c
1698
1699 static int null_gendisk_register(struct nullb *nullb)
1700 {
1701 sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
1702 struct gendisk *disk = nullb->disk;
1703
1704 set_capacity(disk, size);
1705
1706 disk->flags |= GENHD_FL_EXT_DEVT | GENHD_FL_SUPPRESS_PARTITION_INFO;
1707 disk->major = null_major;
1708 disk->first_minor = nullb->index;
1709 disk->minors = 1;
1710 if (queue_is_mq(nullb->q))
1711 disk->fops = &null_rq_ops;
1712 else
1713 disk->fops = &null_bio_ops;
1714 disk->private_data = nullb;
1715 strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
1716
1717 if (nullb->dev->zoned) {
1718 int ret = null_register_zoned_dev(nullb);
1719
1720 if (ret)
1721 return ret;
1722 }
1723
> 1724 return add_disk(disk);
1725 }
1726
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 21469 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] null_blk: add error handling support for add_disk()
2021-07-15 19:51 ` [PATCH 2/3] null_blk: " Luis Chamberlain
2021-07-16 2:13 ` kernel test robot
@ 2021-07-16 2:49 ` kernel test robot
1 sibling, 0 replies; 7+ messages in thread
From: kernel test robot @ 2021-07-16 2:49 UTC (permalink / raw
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2981 bytes --]
Hi Luis,
I love your patch! Yet something to improve:
[auto build test ERROR on block/for-next]
[also build test ERROR on v5.14-rc1 next-20210715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Luis-Chamberlain/block-simple-add_disk-driver-conversions/20210716-035618
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: x86_64-randconfig-b001-20210715 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 0e49c54a8cbd3e779e5526a5888c683c01cc3c50)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/df9eb8cd323290580b5c3c6b2bed0261e67bc16b
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Luis-Chamberlain/block-simple-add_disk-driver-conversions/20210716-035618
git checkout df9eb8cd323290580b5c3c6b2bed0261e67bc16b
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/block/null_blk/main.c:1724:9: error: returning 'void' from a function with incompatible result type 'int'
return add_disk(disk);
^~~~~~~~~~~~~~
1 error generated.
vim +1724 drivers/block/null_blk/main.c
1698
1699 static int null_gendisk_register(struct nullb *nullb)
1700 {
1701 sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
1702 struct gendisk *disk = nullb->disk;
1703
1704 set_capacity(disk, size);
1705
1706 disk->flags |= GENHD_FL_EXT_DEVT | GENHD_FL_SUPPRESS_PARTITION_INFO;
1707 disk->major = null_major;
1708 disk->first_minor = nullb->index;
1709 disk->minors = 1;
1710 if (queue_is_mq(nullb->q))
1711 disk->fops = &null_rq_ops;
1712 else
1713 disk->fops = &null_bio_ops;
1714 disk->private_data = nullb;
1715 strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
1716
1717 if (nullb->dev->zoned) {
1718 int ret = null_register_zoned_dev(nullb);
1719
1720 if (ret)
1721 return ret;
1722 }
1723
> 1724 return add_disk(disk);
1725 }
1726
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 36949 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-16 2:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-07-15 19:51 [PATCH 0/3] block: simple *add_disk*() driver conversions Luis Chamberlain
2021-07-15 19:51 ` [PATCH 1/3] loop: add error handling support for add_disk() Luis Chamberlain
2021-07-16 0:47 ` kernel test robot
2021-07-15 19:51 ` [PATCH 2/3] null_blk: " Luis Chamberlain
2021-07-16 2:13 ` kernel test robot
2021-07-16 2:49 ` kernel test robot
2021-07-15 19:51 ` [PATCH 3/3] nbd: " Luis Chamberlain
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.