On Mon, Jan 13 2020 at 5:41P -0500,
Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxx> wrote:
> From: Anatol Pomazau <anatol@xxxxxxxxxx>
>
> Add a configurable timeout mechanism to disable queue_if_no_path without
> assistance from multipathd. In reality, this reimplements the
> no_path_retry mechanism from multipathd in kernel space, which is
> interesting to prevent processes from hanging indefinitely in cases
> where the daemon might be unable to respond, after a failure or for
> whatever reason.
>
> Despite replicating the policy configuration on kernel space, it is
> quite an important case to prevent IOs from hanging forever, waiting for
> userspace to behave correctly.
>
> v2:
> - Use a module parameter instead of configuring per table
> - Simplify code
>
> Co-developed-by: Frank Mayhar <fmayhar@xxxxxxxxxx>
> Signed-off-by: Frank Mayhar <fmayhar@xxxxxxxxxx>
> Co-developed-by: Bharath Ravi <rbharath@xxxxxxxxxx>
> Signed-off-by: Bharath Ravi <rbharath@xxxxxxxxxx>
> Co-developed-by: Khazhismel Kumykov <khazhy@xxxxxxxxxx>
> Signed-off-by: Khazhismel Kumykov <khazhy@xxxxxxxxxx>
> Signed-off-by: Anatol Pomazau <anatol@xxxxxxxxxx>
> Co-developed-by: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxx>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxx>
All these tags seem rather excessive (especially in that you aren't cc
most of them on the submission). Please review/scrub. Just seems odd
that so many had a hand in this relatively small patch.
The Signed-off-by for anatol@xxxxxxxxxx seems wrong, or did Anatol
shephard this patch at some point? Or did you mean Reviewed-by?
Something else?
Anyway, if in the end you hold these tags to reflect the development
evolution of this patch then so be it ;)
I've reviewed the changes and found various things I felt were
worthwhile to modify. Summary:
- included missing <linux/timer.h>
- added MODULE_PARM_DESC
- moved new functions to reduce needed forward declarations
- tweaked queue_if_no_path_timeout_work's DMWARN message
- added lockdep_assert_held to enable_nopath_timeout
- renamed static queue_if_no_path_timeout to queue_if_no_path_timeout_secs
- use READ_ONCE when accessing queue_if_no_path_timeout_secs
Think that was it.
Please review/apply//test and if you're OK with the end result I'll
get it staged for 5.6 inclusion.
Thanks,
Mike
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index b09e3f925f47..2bc18c9c3abc 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -20,6 +20,7 @@
#include <linux/pagemap.h>
#include <linux/slab.h>
#include <linux/time.h>
+#include <linux/timer.h>
#include <linux/workqueue.h>
#include <linux/delay.h>
#include <scsi/scsi_dh.h>
@@ -31,6 +32,8 @@
#define DM_PG_INIT_DELAY_DEFAULT ((unsigned) -1)
#define QUEUE_IF_NO_PATH_TIMEOUT_DEFAULT 0
+static unsigned long queue_if_no_path_timeout_secs = QUEUE_IF_NO_PATH_TIMEOUT_DEFAULT;
+
/* Path properties */
struct pgpath {
struct list_head list;
@@ -104,10 +107,6 @@ struct dm_mpath_io {
size_t nr_bytes;
};
-static unsigned long queue_if_no_path_timeout = QUEUE_IF_NO_PATH_TIMEOUT_DEFAULT;
-module_param_named(queue_if_no_path_timeout_secs,
- queue_if_no_path_timeout, ulong, 0644);
-
typedef int (*action_fn) (struct pgpath *pgpath);
static struct workqueue_struct *kmultipathd, *kmpath_handlerd;
@@ -115,10 +114,7 @@ static void trigger_event(struct work_struct *work);
static void activate_or_offline_path(struct pgpath *pgpath);
static void activate_path_work(struct work_struct *work);
static void process_queued_bios(struct work_struct *work);
-
static void queue_if_no_path_timeout_work(struct timer_list *t);
-static void enable_nopath_timeout(struct multipath *m);
-static void disable_nopath_timeout(struct multipath *m);
/*-----------------------------------------------
* Multipath state flags.
@@ -730,6 +726,43 @@ static int queue_if_no_path(struct multipath *m, bool queue_if_no_path,
return 0;
}
+/*
+ * If the queue_if_no_path timeout fires, turn off queue_if_no_path and
+ * process any queued I/O.
+ */
+static void queue_if_no_path_timeout_work(struct timer_list *t)
+{
+ struct multipath *m = from_timer(m, t, nopath_timer);
+ struct mapped_device *md = dm_table_get_md(m->ti->table);
+
+ DMWARN("queue_if_no_path timeout on %s, failing queued IO", dm_device_name(md));
+ queue_if_no_path(m, false, false);
+}
+
+/*
+ * Enable the queue_if_no_path timeout if necessary.
+ * Called with m->lock held.
+ */
+static void enable_nopath_timeout(struct multipath *m)
+{
+ unsigned long queue_if_no_path_timeout =
+ READ_ONCE(queue_if_no_path_timeout_secs) * HZ;
+
+ lockdep_assert_held(&m->lock);
+
+ if (queue_if_no_path_timeout > 0 &&
+ atomic_read(&m->nr_valid_paths) == 0 &&
+ test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) {
+ mod_timer(&m->nopath_timer,
+ jiffies + queue_if_no_path_timeout);
+ }
+}
+
+static void disable_nopath_timeout(struct multipath *m)
+{
+ del_timer_sync(&m->nopath_timer);
+}
+
/*
* An event is triggered whenever a path is taken out of use.
* Includes path failure and PG bypass.
@@ -1231,20 +1264,6 @@ static void multipath_dtr(struct dm_target *ti)
free_multipath(m);
}
-/*
- * If the queue_if_no_path timeout fires, turn off queue_if_no_path and
- * process any queued I/O.
- */
-static void queue_if_no_path_timeout_work(struct timer_list *t)
-{
- struct multipath *m = from_timer(m, t, nopath_timer);
- struct mapped_device *md = dm_table_get_md((m)->ti->table);
-
- DMWARN("queue_if_no_path timeout on %s", dm_device_name(md));
-
- queue_if_no_path(m, false, false);
-}
-
/*
* Take a path out of use.
*/
@@ -1352,25 +1371,6 @@ static int action_dev(struct multipath *m, struct dm_dev *dev,
return r;
}
-/*
- * Enable the queue_if_no_path timeout if necessary. Called with m->lock
- * held.
- */
-static void enable_nopath_timeout(struct multipath *m)
-{
- if (queue_if_no_path_timeout > 0 &&
- atomic_read(&m->nr_valid_paths) == 0 &&
- test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) {
- mod_timer(&m->nopath_timer,
- jiffies + queue_if_no_path_timeout * HZ);
- }
-}
-
-static void disable_nopath_timeout(struct multipath *m)
-{
- del_timer_sync(&m->nopath_timer);
-}
-
/*
* Temporarily try to avoid having to use the specified PG
*/
@@ -2127,6 +2127,10 @@ static void __exit dm_multipath_exit(void)
module_init(dm_multipath_init);
module_exit(dm_multipath_exit);
+module_param_named(queue_if_no_path_timeout_secs,
+ queue_if_no_path_timeout_secs, ulong, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(queue_if_no_path_timeout_secs, "No available paths queue IO timeout in seconds");
+
MODULE_DESCRIPTION(DM_NAME " multipath target");
MODULE_AUTHOR("Sistina Software <dm-devel@xxxxxxxxxx>");
MODULE_LICENSE("GPL");
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
