dm-mpath: Simplify the setup_scsi_dh code

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 <bmarzins@redhat.com>
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 5dd90b2cdb9bb8eff0878caabc69620f0e33b23a..c18358271618d2f2668bf6e048f3c62ed8838275 100644 (file)
--- 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: