block: add_disk() error handling stragglers
Subsystems (11)
cover block: add_disk() error handling stragglers
This patch set consists of al the straggler drivers for which we have have no patch reviews done for yet. I'd like to ask for folks to please consider chiming in, specially if you're the maintainer for the driver. Additionally if you can specify if you'll take the patch in yourself or if you want Jens to take it, that'd be great too.
Luis Chamberlain (13):
block/brd: add error handling support for add_disk()
nvme-multipath: add error handling support for add_disk() nvdimm/btt: do not call del_gendisk() if not needed nvdimm/btt: use goto error labels on btt_blk_init() nvdimm/btt: add error handling support for add_disk() nvdimm/blk: avoid calling del_gendisk() on early failures nvdimm/blk: add error handling support for add_disk()
zram: add error handling support for add_disk()
z2ram: add error handling support for add_disk()
ps3disk: add error handling support for add_disk()
ps3vram: add error handling support for add_disk() block/sunvdc: add error handling support for add_disk() mtd/ubi/block: add error handling support for add_disk()
drivers/block/brd.c | 9 +++++++-- drivers/block/ps3disk.c | 8 ++++++-- drivers/block/ps3vram.c | 7 ++++++- drivers/block/sunvdc.c | 14 +++++++++++--- drivers/block/z2ram.c | 7 +++++-- drivers/block/zram/zram_drv.c | 6 +++++- drivers/mtd/ubi/block.c | 8 +++++++- drivers/nvdimm/blk.c | 21 +++++++++++++++------ drivers/nvdimm/btt.c | 24 +++++++++++++++--------- drivers/nvme/host/multipath.c | 14 ++++++++++++-- 10 files changed, 89 insertions(+), 29 deletions(-)
Signature · 1 line
2.30.2
PATCH 1/13 block/brd: add error handling support for add_disk()
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
--- drivers/block/brd.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/block/brd.c +7 -2
@@ -372,6 +372,7 @@ static int brd_alloc(int i)struct brd_device *brd;struct gendisk *disk;char buf[DISK_NAME_LEN ]; +int err = -ENOMEM ;mutex_lock (&brd_devices_mutex);list_for_each_entry (brd, &brd_devices, brd_list) {
@@ -422,16 +423,20 @@ static int brd_alloc(int i)/* Tell the block layer that this is not a rotational device */ blk_queue_flag_set (QUEUE_FLAG_NONROT , disk->queue);blk_queue_flag_clear (QUEUE_FLAG_ADD_RANDOM , disk->queue); -add_disk (disk); + err =add_disk (disk); +if (err) + goto out_cleanup_disk;return 0 ; +out_cleanup_disk: +blk_cleanup_disk (disk); out_free_dev:mutex_lock (&brd_devices_mutex);list_del (&brd->brd_list);mutex_unlock (&brd_devices_mutex);kfree (brd); -return -ENOMEM ; +return err; }static void brd_probe (dev_t dev)
Signature · 1 line
2.30.2
PATCH 2/13 nvme-multipath: add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
Since we now can tell for sure when a disk was added, move setting the bit NVME_NSHEAD_DISK_LIVE only when we did add the disk successfully.
Nothing to do here as the cleanup is done elsewhere. We take care and use test_and_set_bit() because it is protects against two nvme paths simultaneously calling device_add_disk() on the same namespace head.
--- drivers/nvme/host/multipath.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
drivers/nvme/host/multipath.c +12 -2
@@ -496,13 +496,23 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)static void nvme_mpath_set_live (struct nvme_ns *ns) {struct nvme_ns_head *head = ns->head; +int rc;if (!head->disk)return ; +/* +* test_and_set_bit() is used because it is protecting against two nvme +* paths simultaneously calling device_add_disk() on the same namespace +* head. +*/ if (!test_and_set_bit (NVME_NSHEAD_DISK_LIVE , &head->flags)) { -device_add_disk (&head->subsys->dev, head->disk, - nvme_ns_id_attr_groups); + rc =device_add_disk (&head->subsys->dev, head->disk, + nvme_ns_id_attr_groups); +if (rc) { +clear_bit (NVME_NSHEAD_DISK_LIVE , &ns->flags); +return ; + }nvme_add_ns_head_cdev (head); }
Signature · 1 line
2.30.2
Re: [PATCH 02/13] nvme-multipath: add error handling support for add_disk()
Quoted 4 lines · 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.
Quoted 8 lines
Since we now can tell for sure when a disk was added, move setting the bit
NVME_NSHEAD_DISK_LIVEonly when we did add the disk successfully.Nothing to do here as the cleanup is done elsewhere. We take care and use
test_and_set_bit()because it is protects against two nvme paths simultaneously callingdevice_add_disk()on the same namespace head.
Looks good, thank you.
Re: [PATCH 02/13] nvme-multipath: add error handling support for add_disk()
In reply to Luis ChamberlainThanks,
applied to nvme-5.16.
Re: [PATCH 02/13] nvme-multipath: add error handling support for add_disk()
In reply to Luis ChamberlainQuoted 8 lines · 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.Since we now can tell for sure when a disk was added, move setting the bit
NVME_NSHEAD_DISK_LIVEonly when we did add the disk successfully.
Quoted 6 lines
Nothing to do here as the cleanup is done elsewhere. We take care and use
test_and_set_bit()because it is protects against two nvme paths simultaneously callingdevice_add_disk()on the same namespace head.Signed-off-by Luis Chamberlain <[email protected]>
Looks good.
PATCH 3/13 nvdimm/btt: do not call del_gendisk() if not needed
In reply to Luis ChamberlainWe know we don't need del_gendisk() if we haven't added the disk, so just skip it. This should fix a bug on older kernels, as del_gendisk() became able to deal with disks not added only recently, after the patch titled "block: add flag for add_disk() completion notation".
--- drivers/nvdimm/btt.c | 1 - 1 file changed, 1 deletion(-)
drivers/nvdimm/btt.c +0 -1
@@ -1538,7 +1538,6 @@ static int btt_blk_init(struct btt *btt)int rc =nd_integrity_init (btt->btt_disk,btt_meta_size (btt));if (rc) { -del_gendisk (btt->btt_disk);blk_cleanup_disk (btt->btt_disk);return rc; }
Signature · 1 line
2.30.2
Re: [PATCH 03/13] nvdimm/btt: do not call del_gendisk() if not needed
Quoted 6 lines · Luis Chamberlain
We know we don't need
del_gendisk()if we haven't added the disk, so just skip it. This should fix a bug on older kernels, asdel_gendisk()became able to deal with disks not added only recently, after the patch titled "block: add flag foradd_disk()completion notation".
Perhaps put this in:
commit $abbrev_commit ("block: add flag for add_disk() completion notation")
...format, but I can't seem to find that commit?
If you're touching the changelog how about one that clarifies the impact and drops "we"?
"del_gendisk() is not required if the disk has not been added. On kernels prior to commit $abbrev_commit ("block: add flag for add_disk() completion notation")
it is mandatory to not call del_gendisk() if the underlying device has not been through device_add()."
With that you can add:
Re: [PATCH 03/13] nvdimm/btt: do not call del_gendisk() if not needed
Quoted 8 lines · Dan Williams
Quoted 6 lines · Luis Chamberlain
We know we don't need
del_gendisk()if we haven't added the disk, so just skip it. This should fix a bug on older kernels, asdel_gendisk()became able to deal with disks not added only recently, after the patch titled "block: add flag foradd_disk()completion notation".
Quoted 5 lines
Perhaps put this in:
commit $abbrev_commit ("block: add flag for
add_disk()completion notation")...format, but I can't seem to find that commit?
Indeed, that patch got dropped and it would seem Christoph preferred a simpler approach with the new disk_live()
commit 40b3a52ffc5bc3b5427d5d35b035cfb19d03fdd6
Author: Christoph Hellwig <[email protected]> Date: Wed Aug 18 16:45:32 2021 +0200
block: add a sanity check for a live disk in del_gendisk
Quoted 8 lines
If you're touching the changelog how about one that clarifies the impact and drops "we"?
"del_gendisk()is not required if the disk has not been added. On kernels prior to commit $abbrev_commit ("block: add flag foradd_disk()completion notation")
it is mandatory to not calldel_gendisk()if the underlying device has not been throughdevice_add()."
Quoted 5 lines
Fixes 41cd8b70c37a ("libnvdimm, btt: add support for blk integrity")With that you can add:
Reviewed-by Dan Williams <[email protected]>
You got it.
Luis
PATCH 4/13 nvdimm/btt: use goto error labels on btt_blk_init()
In reply to Luis ChamberlainThis will make it easier to share common error paths.
--- drivers/nvdimm/btt.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
drivers/nvdimm/btt.c +12 -7
@@ -1520,10 +1520,11 @@ static int btt_blk_init(struct btt *btt) {struct nd_btt *nd_btt = btt->nd_btt;struct nd_namespace_common *ndns = nd_btt->ndns; +int rc = -ENOMEM ; btt->btt_disk =blk_alloc_disk (NUMA_NO_NODE );if (!btt->btt_disk) -return -ENOMEM ; + goto out;nvdimm_namespace_disk_name (ndns, btt->btt_disk->disk_name); btt->btt_disk->first_minor =0 ;
@@ -1535,19 +1536,23 @@ static int btt_blk_init(struct btt *btt)blk_queue_flag_set (QUEUE_FLAG_NONROT , btt->btt_disk->queue);if (btt_meta_size (btt)) { -int rc =nd_integrity_init (btt->btt_disk,btt_meta_size (btt)); - -if (rc) { -blk_cleanup_disk (btt->btt_disk); -return rc; - } + rc =nd_integrity_init (btt->btt_disk,btt_meta_size (btt)); +if (rc) + goto out_cleanup_disk; } +set_capacity (btt->btt_disk, btt->nlba * btt->sector_size >>9 );device_add_disk (&btt->nd_btt->dev, btt->btt_disk,NULL ); + btt->nd_btt->size = btt->nlba * (u64 )btt->sector_size;nvdimm_check_and_set_ro (btt->btt_disk);return 0 ; + +out_cleanup_disk: +blk_cleanup_disk (btt->btt_disk); +out: +return rc; }static void btt_blk_cleanup (struct btt *btt)
Signature · 6 lines
2.30.2 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Re: [PATCH 04/13] nvdimm/btt: use goto error labels on btt_blk_init()
Quoted 13 lines · Luis Chamberlain
This will make it easier to share common error paths.
Signed-off-by Luis Chamberlain <[email protected]>--- drivers/nvdimm/btt.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)drivers/nvdimm/btt.c +0
-0
drivers/nvdimm/btt.c static int btt_blk_init(struct btt *btt) · line 1527
Quoted 9 lines
drivers/nvdimm/btt.c +2
-1@@ -1520,10 +1520,11 @@ static int btt_blk_init(struct btt *btt) {struct nd_btt *nd_btt = btt->nd_btt;struct nd_namespace_common *ndns = nd_btt->ndns; +int rc = -ENOMEM ; btt->btt_disk =blk_alloc_disk (NUMA_NO_NODE );if (!btt->btt_disk) -return -ENOMEM ; + goto out;
I tend to not use a goto when there is nothing to unwind.
The rest looks good to me. After dropping "goto out;" you can add:
PATCH 5/13 nvdimm/btt: add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
--- drivers/nvdimm/btt.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/nvdimm/btt.c +3 -1
@@ -1542,7 +1542,9 @@ static int btt_blk_init(struct btt *btt) }set_capacity (btt->btt_disk, btt->nlba * btt->sector_size >>9 ); -device_add_disk (&btt->nd_btt->dev, btt->btt_disk,NULL ); + rc =device_add_disk (&btt->nd_btt->dev, btt->btt_disk,NULL ); +if (rc) + goto out_cleanup_disk; btt->nd_btt->size = btt->nlba * (u64 )btt->sector_size;nvdimm_check_and_set_ro (btt->btt_disk);
Signature · 1 line
2.30.2
PATCH 6/13 nvdimm/blk: avoid calling del_gendisk() on early failures
In reply to Luis ChamberlainIf nd_integrity_init() fails we'd get del_gendisk() called, but that's not correct as we should only call that if we're done with device_add_disk(). Fix this by providing unwinding prior to the devm call being registered and moving the devm registration to the very end.
This should fix calling del_gendisk() if nd_integrity_init() fails. I only spotted this issue through code inspection. It does not fix any real world bug.
--- drivers/nvdimm/blk.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
drivers/nvdimm/blk.c +12 -5
@@ -240,6 +240,7 @@ static int nsblk_attach_disk(struct nd_namespace_blk *nsblk)resource_size_t available_disk_size;struct gendisk *disk;u64 internal_nlba; +int rc; internal_nlba =div_u64 (nsblk->size,nsblk_internal_lbasize (nsblk)); available_disk_size = internal_nlba *nsblk_sector_size (nsblk);
@@ -256,20 +257,26 @@ static int nsblk_attach_disk(struct nd_namespace_blk *nsblk)blk_queue_logical_block_size (disk->queue,nsblk_sector_size (nsblk));blk_queue_flag_set (QUEUE_FLAG_NONROT , disk->queue); -if (devm_add_action_or_reset (dev, nd_blk_release_disk, disk)) -return -ENOMEM ; -if (nsblk_meta_size (nsblk)) { -int rc =nd_integrity_init (disk,nsblk_meta_size (nsblk)); + rc =nd_integrity_init (disk,nsblk_meta_size (nsblk));if (rc) -return rc; + goto out_before_devm_err; }set_capacity (disk, available_disk_size >>SECTOR_SHIFT );device_add_disk (dev, disk,NULL ); + +/* nd_blk_release_disk() is called if this fails */ +if (devm_add_action_or_reset (dev, nd_blk_release_disk, disk)) +return -ENOMEM ; +nvdimm_check_and_set_ro (disk);return 0 ; + +out_before_devm_err: +blk_cleanup_disk (disk); +return rc; }static int nd_blk_probe (struct device *dev)
Signature · 1 line
2.30.2
Re: [PATCH 06/13] nvdimm/blk: avoid calling del_gendisk() on early failures
Quoted 11 lines · Luis Chamberlain
If
nd_integrity_init()fails we'd getdel_gendisk()called, but that's not correct as we should only call that if we're done withdevice_add_disk(). Fix this by providing unwinding prior to the devm call being registered and moving the devm registration to the very end.This should fix calling
del_gendisk()ifnd_integrity_init()fails. I only spotted this issue through code inspection. It does not fix any real world bug.
Just fyi, I'm preparing patches to delete this driver completely as it is unused by any shipping platform. I hope to get that removal into v5.16.
Re: [PATCH 06/13] nvdimm/blk: avoid calling del_gendisk() on early failures
Quoted 8 lines · Dan Williams
Quoted 7 lines · Luis Chamberlain
If
nd_integrity_init()fails we'd getdel_gendisk()called, but that's not correct as we should only call that if we're done withdevice_add_disk(). Fix this by providing unwinding prior to the devm call being registered and moving the devm registration to the very end.
Quoted 8 lines
Quoted 4 lines
This should fix calling
del_gendisk()ifnd_integrity_init()fails. I only spotted this issue through code inspection. It does not fix any real world bug.Just fyi, I'm preparing patches to delete this driver completely as it is unused by any shipping platform. I hope to get that removal into v5.16.
I'll remove this from my queue, any chance you can review the changes for nvdimm/btt?
Luis
Re: [PATCH 06/13] nvdimm/blk: avoid calling del_gendisk() on early failures
In reply to Dan WilliamsQuoted 8 lines · Dan Williams
Quoted 7 lines · Luis Chamberlain
If
nd_integrity_init()fails we'd getdel_gendisk()called, but that's not correct as we should only call that if we're done withdevice_add_disk(). Fix this by providing unwinding prior to the devm call being registered and moving the devm registration to the very end.
Quoted 8 lines
Quoted 4 lines
This should fix calling
del_gendisk()ifnd_integrity_init()fails. I only spotted this issue through code inspection. It does not fix any real world bug.Just fyi, I'm preparing patches to delete this driver completely as it is unused by any shipping platform. I hope to get that removal into v5.16.
Curious if are you going to nuking it on v5.16? Otherwise it would stand in the way of the last few patches to add __must_check for the final add_disk() error handling changes.
Luis
Re: [PATCH 06/13] nvdimm/blk: avoid calling del_gendisk() on early failures
Quoted 15 lines · Luis Chamberlain
Quoted 13 lines · Dan Williams
Quoted 11 lines · Luis Chamberlain
If
nd_integrity_init()fails we'd getdel_gendisk()called, but that's not correct as we should only call that if we're done withdevice_add_disk(). Fix this by providing unwinding prior to the devm call being registered and moving the devm registration to the very end.This should fix calling
del_gendisk()ifnd_integrity_init()fails. I only spotted this issue through code inspection. It does not fix any real world bug.
Quoted 7 lines
Quoted 3 lines
Just fyi, I'm preparing patches to delete this driver completely as it is unused by any shipping platform. I hope to get that removal into v5.16.
Curious if are you going to nuking it on v5.16? Otherwise it would stand in the way of the last few patches to add
__must_checkfor the finaladd_disk()error handling changes.
True, I don't think I can get it nuked in time, so you can add my Reviewed-by for this one.
Jens Axboe
maintainerRe: [PATCH 06/13] nvdimm/blk: avoid calling del_gendisk() on early failures
Quoted 20 lines · Dan Williams
Quoted 19 lines · Luis Chamberlain
Quoted 16 lines · Dan Williams
Quoted 11 lines · Luis Chamberlain
If
nd_integrity_init()fails we'd getdel_gendisk()called, but that's not correct as we should only call that if we're done withdevice_add_disk(). Fix this by providing unwinding prior to the devm call being registered and moving the devm registration to the very end.This should fix calling
del_gendisk()ifnd_integrity_init()fails. I only spotted this issue through code inspection. It does not fix any real world bug.Just fyi, I'm preparing patches to delete this driver completely as it is unused by any shipping platform. I hope to get that removal into v5.16.
Quoted 6 lines
Quoted 3 lines
Curious if are you going to nuking it on v5.16? Otherwise it would stand in the way of the last few patches to add
__must_checkfor the finaladd_disk()error handling changes.True, I don't think I can get it nuked in time, so you can add my Reviewed-by for this one.
Luis, I lost track of the nv* patches from this discussion. If you want them in 5.16 and they are reviewed, please do resend and I'll pick them up for the middle-of-merge-window push.
Signature · 1 line
Jens Axboe
Re: [PATCH 06/13] nvdimm/blk: avoid calling del_gendisk() on early failures
Quoted 25 lines · Jens Axboe
Quoted 24 lines · Dan Williams
Quoted 22 lines · Luis Chamberlain
Quoted 8 lines · Dan Williams
Quoted 7 lines · Luis Chamberlain
If
nd_integrity_init()fails we'd getdel_gendisk()called, but that's not correct as we should only call that if we're done withdevice_add_disk(). Fix this by providing unwinding prior to the devm call being registered and moving the devm registration to the very end.Quoted 8 lines
Quoted 4 lines
This should fix calling
del_gendisk()ifnd_integrity_init()fails. I only spotted this issue through code inspection. It does not fix any real world bug.Just fyi, I'm preparing patches to delete this driver completely as it is unused by any shipping platform. I hope to get that removal into v5.16.
Curious if are you going to nuking it on v5.16? Otherwise it would stand in the way of the last few patches to add
__must_checkfor the finaladd_disk()error handling changes.
Quoted 6 lines
Quoted 2 lines
True, I don't think I can get it nuked in time, so you can add my Reviewed-by for this one.
Luis, I lost track of the nv* patches from this discussion. If you want them in 5.16 and they are reviewed, please do resend and I'll pick them up for the middle-of-merge-window push.
Sure thing, I'll resend whatever is left. I also noticed for some reason I forgot to convert nvdimm/pmem and so I'll roll those new patches in, but I suspect that those might be too late unless we get them reviewed in time.
Luis
Re: [PATCH 06/13] nvdimm/blk: avoid calling del_gendisk() on early failures
In reply to Dan WilliamsQuoted 20 lines · Dan Williams
Quoted 19 lines · Luis Chamberlain
Quoted 16 lines · Dan Williams
Quoted 11 lines · Luis Chamberlain
If
nd_integrity_init()fails we'd getdel_gendisk()called, but that's not correct as we should only call that if we're done withdevice_add_disk(). Fix this by providing unwinding prior to the devm call being registered and moving the devm registration to the very end.This should fix calling
del_gendisk()ifnd_integrity_init()fails. I only spotted this issue through code inspection. It does not fix any real world bug.Just fyi, I'm preparing patches to delete this driver completely as it is unused by any shipping platform. I hope to get that removal into v5.16.
Quoted 6 lines
Quoted 3 lines
Curious if are you going to nuking it on v5.16? Otherwise it would stand in the way of the last few patches to add
__must_checkfor the finaladd_disk()error handling changes.True, I don't think I can get it nuked in time, so you can add my Reviewed-by for this one.
This patch required the previous patch in this series to also be applied. Can I apply your Reviewed-by there too?
Luis
PATCH 7/13 nvdimm/blk: add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
Since nvdimm/blk uses devm we just need to move the devm registration towards the end. And in hindsight, that seems to also provide a fix given del_gendisk() should not be called unless the disk was already added via add_disk(). The probably of that issue happening is low though, like OOM while calling devm_add_action(), so the fix is minor.
We manually unwind in case of add_disk() failure prior to the devm registration.
--- drivers/nvdimm/blk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/nvdimm/blk.c +3 -1
@@ -265,7 +265,9 @@ static int nsblk_attach_disk(struct nd_namespace_blk *nsblk) }set_capacity (disk, available_disk_size >>SECTOR_SHIFT ); -device_add_disk (dev, disk,NULL ); + rc =device_add_disk (dev, disk,NULL ); +if (rc) + goto out_before_devm_err;/* nd_blk_release_disk() is called if this fails */ if (devm_add_action_or_reset (dev, nd_blk_release_disk, disk))
Signature · 1 line
2.30.2
PATCH 8/13 zram: add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
--- drivers/block/zram/zram_drv.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/block/zram/zram_drv.c +5 -1
@@ -1983,7 +1983,9 @@ static int zram_add(void)blk_queue_max_write_zeroes_sectors (zram->disk->queue,UINT_MAX );blk_queue_flag_set (QUEUE_FLAG_STABLE_WRITES , zram->disk->queue); -device_add_disk (NULL , zram->disk, zram_disk_attr_groups); + ret =device_add_disk (NULL , zram->disk, zram_disk_attr_groups); +if (ret) + goto out_cleanup_disk;strlcpy (zram->compressor, default_compressor,sizeof (zram->compressor));
@@ -1991,6 +1993,8 @@ static int zram_add(void)pr_info ("Added device: %s\n" , zram->disk->disk_name);return device_id; +out_cleanup_disk: +blk_cleanup_disk (zram->disk); out_free_idr:idr_remove (&zram_index_idr, device_id); out_free_dev:
Signature · 1 line
2.30.2
Re: [PATCH 08/13] zram: add error handling support for add_disk()
Quoted 5 lines · 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 <[email protected]>
PATCH 9/13 z2ram: add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Only the disk is cleaned up inside z2ram_register_disk() as the caller deals with the rest.
--- drivers/block/z2ram.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/block/z2ram.c +5 -2
@@ -318,6 +318,7 @@ static const struct blk_mq_ops z2_mq_ops = {static int z2ram_register_disk (int minor) {struct gendisk *disk; +int err; disk =blk_mq_alloc_disk (&tag_set,NULL );if (IS_ERR (disk))
@@ -333,8 +334,10 @@ static int z2ram_register_disk(int minor)sprintf (disk->disk_name,"z2ram" ); z2ram_gendisk[minor] = disk; -add_disk (disk); -return 0 ; + err =add_disk (disk); +if (err) +blk_cleanup_disk (disk); +return err; }static int __init z2_init (void )
Signature · 1 line
2.30.2
PATCH 10/13 ps3disk: add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
--- drivers/block/ps3disk.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/block/ps3disk.c +6 -2
@@ -467,9 +467,13 @@ static int ps3disk_probe(struct ps3_system_bus_device *_dev) gendisk->disk_name, priv->model, priv->raw_capacity >>11 , get_capacity(gendisk) >> 11); -device_add_disk (&dev->sbd.core, gendisk,NULL ); -return 0 ; + error =device_add_disk (&dev->sbd.core, gendisk,NULL ); +if (error) + goto fail_cleanup_disk; +return 0 ; +fail_cleanup_disk: +blk_cleanup_disk (gendisk); fail_free_tag_set:blk_mq_free_tag_set (&priv->tag_set); fail_teardown:
Signature · 1 line
2.30.2
Re: [PATCH 10/13] ps3disk: add error handling support for add_disk()
Hi Luis,
Quoted 5 lines · 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 <[email protected]>
I tested your 20211011-for-axboe-add-disk-error-handling branch on PS3 and the ps3disk changes seem to be working OK.
PATCH 11/13 ps3vram: add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
--- drivers/block/ps3vram.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/block/ps3vram.c +6 -1
@@ -755,9 +755,14 @@ static int ps3vram_probe(struct ps3_system_bus_device *dev)dev_info (&dev->core,"%s: Using %llu MiB of GPU memory\n" , gendisk->disk_name,get_capacity (gendisk) >>11 ); -device_add_disk (&dev->core, gendisk,NULL ); + error =device_add_disk (&dev->core, gendisk,NULL ); +if (error) + goto out_cleanup_disk; +return 0 ; +out_cleanup_disk: +blk_cleanup_disk (gendisk); out_cache_cleanup:remove_proc_entry (DEVICE_NAME ,NULL );ps3vram_cache_cleanup (dev);
Signature · 1 line
2.30.2
Re: [PATCH 11/13] ps3vram: add error handling support for add_disk()
Hi Luis,
Quoted 3 lines · 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.
I didn't yet test this ps3vram related change, but based on the ps3disk testing I think this change will be OK.
PATCH 12/13 block/sunvdc: add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
We re-use the same free tag call, so we also add a label for that as well.
--- drivers/block/sunvdc.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/block/sunvdc.c +11 -3
@@ -826,8 +826,8 @@ static int probe_disk(struct vdc_port *port)if (IS_ERR (g)) { printk(KERN_ERR PFX "%s: Could not allocate gendisk.\n", port->vio.name); -blk_mq_free_tag_set (&port->tag_set); -return PTR_ERR (g); + err =PTR_ERR (g); + goto out_free_tag; } port->disk = g;
@@ -879,9 +879,17 @@ static int probe_disk(struct vdc_port *port) port->vdisk_size, (port->vdisk_size >> (20 -9 )), port->vio.ver.major, port->vio.ver.minor); -device_add_disk (&port->vio.vdev->dev, g,NULL ); + err =device_add_disk (&port->vio.vdev->dev, g,NULL ); +if (err) + goto out_cleanup_disk;return 0 ; + +out_cleanup_disk: +blk_cleanup_disk (g); +out_free_tag: +blk_mq_free_tag_set (&port->tag_set); +return err; }static struct ldc_channel_config vdc_ldc_cfg = {
Signature · 1 line
2.30.2
PATCH 13/13 mtd/ubi/block: add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
--- drivers/mtd/ubi/block.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/mtd/ubi/block.c +7 -1
@@ -447,12 +447,18 @@ int ubiblock_create(struct ubi_volume_info *vi)list_add_tail (&dev->list, &ubiblock_devices);/* Must be the last step: anyone can call file ops from now on */ -add_disk (dev->gd); + ret =add_disk (dev->gd); +if (ret) + goto out_destroy_wq; +dev_info (disk_to_dev (dev->gd),"created from ubi%d:%d(%s)" , dev->ubi_num, dev->vol_id, vi->name);mutex_unlock (&devices_mutex);return 0 ; +out_destroy_wq: +list_del (&dev->list); +destroy_workqueue (dev->wq); out_remove_minor:idr_remove (&ubiblock_minor_idr, gd->first_minor); out_cleanup_disk:
Signature · 1 line
2.30.2
Geoff Levand
maintainerHi Luis,
Quoted 5 lines · Luis Chamberlain
This patch set consists of al the straggler drivers for which we have have no patch reviews done for yet. I'd like to ask for folks to please consider chiming in, specially if you're the maintainer for the driver. Additionally if you can specify if you'll take the patch in yourself or if you want Jens to take it, that'd be great too.
Do you have a git repo with the patch set applied that I can use to test with?
Thanks.
-Geoff
Quoted 10 lines · Geoff Levand
Hi Luis,
Quoted 5 lines · Luis Chamberlain
This patch set consists of al the straggler drivers for which we have have no patch reviews done for yet. I'd like to ask for folks to please consider chiming in, specially if you're the maintainer for the driver. Additionally if you can specify if you'll take the patch in yourself or if you want Jens to take it, that'd be great too.
Do you have a git repo with the patch set applied that I can use to test with?
Sure, although the second to last patch is in a state of flux given the ataflop driver currently is broken and so we're seeing how to fix that first:
Luis
Geoff Levand
maintainerHi Luis,
Quoted 10 lines · Luis Chamberlain
Quoted 9 lines · Geoff Levand
Hi Luis,
Quoted 5 lines · Luis Chamberlain
This patch set consists of al the straggler drivers for which we have have no patch reviews done for yet. I'd like to ask for folks to please consider chiming in, specially if you're the maintainer for the driver. Additionally if you can specify if you'll take the patch in yourself or if you want Jens to take it, that'd be great too.
Quoted 7 lines
Quoted 1 line
Do you have a git repo with the patch set applied that I can use to test with?
Sure, although the second to last patch is in a state of flux given the ataflop driver currently is broken and so we're seeing how to fix that first:
That branch has so many changes applied on top of the base v5.15-rc4 that the patches I need to apply to test on PS3 with don't apply.
Do you have something closer to say v5.15-rc5? Preferred would be just your add_disk() error handling patches plus what they depend on.
Thanks.
-Geoff
Quoted 19 lines · Geoff Levand
Hi Luis,
Quoted 16 lines · Luis Chamberlain
Quoted 10 lines · Geoff Levand
Hi Luis,
Quoted 5 lines · Luis Chamberlain
This patch set consists of al the straggler drivers for which we have have no patch reviews done for yet. I'd like to ask for folks to please consider chiming in, specially if you're the maintainer for the driver. Additionally if you can specify if you'll take the patch in yourself or if you want Jens to take it, that'd be great too.
Do you have a git repo with the patch set applied that I can use to test with?
Sure, although the second to last patch is in a state of flux given the ataflop driver currently is broken and so we're seeing how to fix that first:
Quoted 8 lines
That branch has so many changes applied on top of the base v5.15-rc4 that the patches I need to apply to test on PS3 with don't apply.
Do you have something closer to say v5.15-rc5? Preferred would be just your
add_disk()error handling patches plus what they depend on.
If you just want to test the ps3 changes, I've put this branch together just for yo, its based on v5.15-rc6:
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20211025-ps3-add-disk
Luis
Re: (subset) [PATCH 00/13] block: add_disk() error handling stragglers
In reply to Luis ChamberlainQuoted 21 lines · Luis Chamberlain
This patch set consists of al the straggler drivers for which we have have no patch reviews done for yet. I'd like to ask for folks to please consider chiming in, specially if you're the maintainer for the driver. Additionally if you can specify if you'll take the patch in yourself or if you want Jens to take it, that'd be great too.
Luis Chamberlain (13):
block/brd: add error handling support foradd_disk()
nvme-multipath: add error handling support foradd_disk()nvdimm/btt: do not calldel_gendisk()if not needed nvdimm/btt: use goto error labels onbtt_blk_init()nvdimm/btt: add error handling support foradd_disk()nvdimm/blk: avoid callingdel_gendisk()on early failures nvdimm/blk: add error handling support foradd_disk()
zram: add error handling support foradd_disk()
z2ram: add error handling support foradd_disk()
ps3disk: add error handling support foradd_disk()
ps3vram: add error handling support foradd_disk()block/sunvdc: add error handling support foradd_disk()mtd/ubi/block: add error handling support foradd_disk()
Quoted 1 line
[...]
Applied, thanks!
[08/13] zram: add error handling support for add_disk()
commit: 5e2e1cc4131cf4d21629c94331f2351b7dc8b87c
[10/13] ps3disk: add error handling support for add_disk()
commit: ff4cbe0fcf5d749f76040f782f0618656cd23e33
[11/13] ps3vram: add error handling support for add_disk()
commit: 3c30883acab1d20ecbd3c48dc12b147b51548742Best regards,
Signature · 1 line
Jens Axboe
Re: (subset) [PATCH 00/13] block: add_disk() error handling stragglers
In reply to Luis ChamberlainQuoted 21 lines · Luis Chamberlain
This patch set consists of al the straggler drivers for which we have have no patch reviews done for yet. I'd like to ask for folks to please consider chiming in, specially if you're the maintainer for the driver. Additionally if you can specify if you'll take the patch in yourself or if you want Jens to take it, that'd be great too.
Luis Chamberlain (13):
block/brd: add error handling support foradd_disk()
nvme-multipath: add error handling support foradd_disk()nvdimm/btt: do not calldel_gendisk()if not needed nvdimm/btt: use goto error labels onbtt_blk_init()nvdimm/btt: add error handling support foradd_disk()nvdimm/blk: avoid callingdel_gendisk()on early failures nvdimm/blk: add error handling support foradd_disk()
zram: add error handling support foradd_disk()
z2ram: add error handling support foradd_disk()
ps3disk: add error handling support foradd_disk()
ps3vram: add error handling support foradd_disk()block/sunvdc: add error handling support foradd_disk()mtd/ubi/block: add error handling support foradd_disk()
Quoted 1 line
[...]
Applied, thanks!
[01/13] block/brd: add error handling support for add_disk()
commit: e1528830bd4ebf435d91c154e309e6e028336210
Best regards,
Signature · 1 line
Jens Axboe