| Message ID | 20210823202930.137278-9-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: first batch of add_disk() error handling conversions | expand |
On Mon, Aug 23, 2021 at 01:29:28PM -0700, Luis Chamberlain wrote: > - add_disk(md->disk); > + r = add_disk(md->disk); > + if (r) > + goto out_cleanup_disk; > > r = dm_sysfs_init(md); > - if (r) { > - del_gendisk(md->disk); > - return r; > - } > + if (r) > + goto out_del_gendisk; > md->type = type; > return 0; > + > +out_cleanup_disk: > + blk_cleanup_disk(md->disk); > +out_del_gendisk: > + del_gendisk(md->disk); > + return r; I think the add_disk should just return r. If you look at the callers they eventualy end up in dm_table_destroy, which does this cleanup.
On Tue, Aug 24, 2021 at 07:21:30AM +0100, Christoph Hellwig wrote: > On Mon, Aug 23, 2021 at 01:29:28PM -0700, Luis Chamberlain wrote: > > - add_disk(md->disk); > > + r = add_disk(md->disk); > > + if (r) > > + goto out_cleanup_disk; > > > > r = dm_sysfs_init(md); > > - if (r) { > > - del_gendisk(md->disk); > > - return r; > > - } > > + if (r) > > + goto out_del_gendisk; > > md->type = type; > > return 0; > > + > > +out_cleanup_disk: > > + blk_cleanup_disk(md->disk); > > +out_del_gendisk: > > + del_gendisk(md->disk); > > + return r; > > I think the add_disk should just return r. If you look at the > callers they eventualy end up in dm_table_destroy, which does > this cleanup. I don't see it. What part of dm_table_destroy() does this? Luis
On Fri, Aug 27, 2021 at 11:55:14AM -0700, Luis Chamberlain wrote: > > I think the add_disk should just return r. If you look at the > > callers they eventualy end up in dm_table_destroy, which does > > this cleanup. > > I don't see it. What part of dm_table_destroy() does this? Sorry, dm_destroy, not dm_table_destroy. For dm_early_create it's trivial as that calls both dm_table_destroy and dm_destroy in the error path. The normal table_load case is a separate ioctl, but if that fails userspace needs to call the DM_DEV_REMOVE_CMD to cleanup the state - similar to any other failure.
On Sat, Aug 28, 2021 at 08:35:57AM +0100, Christoph Hellwig wrote: > On Fri, Aug 27, 2021 at 11:55:14AM -0700, Luis Chamberlain wrote: > > > I think the add_disk should just return r. If you look at the > > > callers they eventualy end up in dm_table_destroy, which does > > > this cleanup. > > > > I don't see it. What part of dm_table_destroy() does this? > > Sorry, dm_destroy, not dm_table_destroy. For dm_early_create it's > trivial as that calls both dm_table_destroy and dm_destroy in the error > path. The normal table_load case is a separate ioctl, but if that > fails userspace needs to call the DM_DEV_REMOVE_CMD to cleanup > the state - similar to any other failure. I see, ok sure I'll document this on the commit log as its not so obvious. Luis
diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 7981b7287628..cd26fde4ab56 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -2077,15 +2077,21 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t) if (r) return r; - add_disk(md->disk); + r = add_disk(md->disk); + if (r) + goto out_cleanup_disk; r = dm_sysfs_init(md); - if (r) { - del_gendisk(md->disk); - return r; - } + if (r) + goto out_del_gendisk; md->type = type; return 0; + +out_cleanup_disk: + blk_cleanup_disk(md->disk); +out_del_gendisk: + del_gendisk(md->disk); + return r; } struct mapped_device *dm_get_md(dev_t dev)
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/md/dm.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)