Accepted for the upcoming merge window, thanks. Mikulas
On Fri, 21 Nov 2025, Benjamin Marzinski wrote: > There's no point to the MPATHF_RETAIN_ATTACHED_HW_HANDLER flag any more. > The way setup_scsi_dh() worked, if that flag wasn't set, it would > attempt to attach any passed in hardware handler. This would always fail > if a different hardware handler was attached, which caused > setup_scsi_dh() to rerun as if the flag was set. So the code would > already retain any attached handler, because attaching a different one > would always fail. > > Also, the code had a bug. If attached_handler_name was NULL but there > was a scsi device handler attached (because either > scsi_dh_attached_handler_name failed() to allocate a name, a handler got > attached after it was called) the code would loop endlessly. > > Instead, ignore MPATHF_RETAIN_ATTACHED_HW_HANDLER, and always free the > passed in handler if *attached_handler_name is set. This simplifies the > code, and avoids the endless loop bug, while keeping the functionality > the same. > > Signed-off-by: Benjamin Marzinski <[email protected]> > --- > drivers/md/dm-mpath.c | 61 ++++++++++++++++--------------------------- > 1 file changed, 23 insertions(+), 38 deletions(-) > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > index 5dd90b2cdb9b..c18358271618 100644 > --- a/drivers/md/dm-mpath.c > +++ b/drivers/md/dm-mpath.c > @@ -131,7 +131,7 @@ static void queue_if_no_path_timeout_work(struct > timer_list *t); > #define MPATHF_QUEUE_IO 0 /* Must we queue all I/O? */ > #define MPATHF_QUEUE_IF_NO_PATH 1 /* Queue I/O if last path > fails? */ > #define MPATHF_SAVED_QUEUE_IF_NO_PATH 2 /* Saved state during > suspension */ > -#define MPATHF_RETAIN_ATTACHED_HW_HANDLER 3 /* If there's already a > hw_handler present, don't change it. */ > +/* MPATHF_RETAIN_ATTACHED_HW_HANDLER no longer has any effect */ > #define MPATHF_PG_INIT_DISABLED 4 /* pg_init is not currently > allowed */ > #define MPATHF_PG_INIT_REQUIRED 5 /* pg_init needs calling? */ > #define MPATHF_PG_INIT_DELAY_RETRY 6 /* Delay pg_init retry? */ > @@ -237,16 +237,10 @@ static struct multipath *alloc_multipath(struct > dm_target *ti) > > static int alloc_multipath_stage2(struct dm_target *ti, struct multipath *m) > { > - if (m->queue_mode == DM_TYPE_NONE) { > + if (m->queue_mode == DM_TYPE_NONE) > m->queue_mode = DM_TYPE_REQUEST_BASED; > - } else if (m->queue_mode == DM_TYPE_BIO_BASED) { > + else if (m->queue_mode == DM_TYPE_BIO_BASED) > INIT_WORK(&m->process_queued_bios, process_queued_bios); > - /* > - * bio-based doesn't support any direct scsi_dh management; > - * it just discovers if a scsi_dh is attached. > - */ > - set_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags); > - } > > dm_table_set_type(ti->table, m->queue_mode); > > @@ -887,36 +881,30 @@ static int setup_scsi_dh(struct block_device *bdev, > struct multipath *m, > struct request_queue *q = bdev_get_queue(bdev); > int r; > > - if (mpath_double_check_test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, m)) { > -retain: > - if (*attached_handler_name) { > - /* > - * Clear any hw_handler_params associated with a > - * handler that isn't already attached. > - */ > - if (m->hw_handler_name && > strcmp(*attached_handler_name, m->hw_handler_name)) { > - kfree(m->hw_handler_params); > - m->hw_handler_params = NULL; > - } > - > - /* > - * Reset hw_handler_name to match the attached handler > - * > - * NB. This modifies the table line to show the actual > - * handler instead of the original table passed in. > - */ > - kfree(m->hw_handler_name); > - m->hw_handler_name = *attached_handler_name; > - *attached_handler_name = NULL; > + if (*attached_handler_name) { > + /* > + * Clear any hw_handler_params associated with a > + * handler that isn't already attached. > + */ > + if (m->hw_handler_name && strcmp(*attached_handler_name, > + m->hw_handler_name)) { > + kfree(m->hw_handler_params); > + m->hw_handler_params = NULL; > } > + > + /* > + * Reset hw_handler_name to match the attached handler > + * > + * NB. This modifies the table line to show the actual > + * handler instead of the original table passed in. > + */ > + kfree(m->hw_handler_name); > + m->hw_handler_name = *attached_handler_name; > + *attached_handler_name = NULL; > } > > if (m->hw_handler_name) { > r = scsi_dh_attach(q, m->hw_handler_name); > - if (r == -EBUSY) { > - DMINFO("retaining handler on device %pg", bdev); > - goto retain; > - } > if (r < 0) { > *error = "error attaching hardware handler"; > return r; > @@ -1138,7 +1126,7 @@ static int parse_features(struct dm_arg_set *as, struct > multipath *m) > } > > if (!strcasecmp(arg_name, "retain_attached_hw_handler")) { > - set_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags); > + /* no longer has any effect */ > continue; > } > > @@ -1823,7 +1811,6 @@ static void multipath_status(struct dm_target *ti, > status_type_t type, > DMEMIT("%u ", test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags) + > (m->pg_init_retries > 0) * 2 + > (m->pg_init_delay_msecs != > DM_PG_INIT_DELAY_DEFAULT) * 2 + > - test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, > &m->flags) + > (m->queue_mode != DM_TYPE_REQUEST_BASED) * 2); > > if (test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) > @@ -1832,8 +1819,6 @@ static void multipath_status(struct dm_target *ti, > status_type_t type, > DMEMIT("pg_init_retries %u ", m->pg_init_retries); > if (m->pg_init_delay_msecs != DM_PG_INIT_DELAY_DEFAULT) > DMEMIT("pg_init_delay_msecs %u ", > m->pg_init_delay_msecs); > - if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags)) > - DMEMIT("retain_attached_hw_handler "); > if (m->queue_mode != DM_TYPE_REQUEST_BASED) { > switch (m->queue_mode) { > case DM_TYPE_BIO_BASED: > -- > 2.50.1 >
