[patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init

[patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init

[Dave Wysochanski]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: dm-mpath-add-retry-pg-init.patch --]
[-- Type: text/x-patch, Size: 5509 bytes --]

Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
core.  The flag is a generic one, but in this instance we use it to flag
cases where we must retry a pg_init command.  The patch is useful for cases
where a hw handler sends a path initialization command to the storage and
it sees the command complete with an error code indicating the command
should be retried.  In this case, the hardware handler should call 
dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
that the dm-mpath core will actually retry the pg_init.  The number of actual
retries is governed by the multipath feature argument "pg_init_retries".  
Once the dm-mpath core has retried the command "pg_init_retries" times
without success, a subsequent dm_pg_init_complete() with MP_RETRY will
cause the path to be failed via fail_path().  To specify a value of
pg_init_retries, add a line similar to the following in the 'device' section
of your /etc/multipath.conf file:
features                "2 pg_init_retries 7"

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Acked-by: Mike Christie <michaelc@cs.wisc.edu>
Acked-by: Chandra Seetharaman <sekharan@us.ibm.com>

---
Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
+++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
@@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
 #define MP_FAIL_PATH 1
 #define MP_BYPASS_PG 2
 #define MP_ERROR_IO  4	/* Don't retry this I/O */
+#define MP_RETRY 8
 
 #endif
Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
+++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
@@ -75,6 +75,8 @@ struct multipath {
 	unsigned queue_io;		/* Must we queue all I/O? */
 	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
 	unsigned saved_queue_if_no_path;/* Saved state during suspension */
+	unsigned pg_init_retries;	/* Number of times to retry pg_init */
+	unsigned pg_init_count;		/* Number of times pg_init called */
 
 	struct work_struct process_queued_ios;
 	struct bio_list queued_ios;
@@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
 	if (hwh->type && hwh->type->pg_init) {
 		m->pg_init_required = 1;
 		m->queue_io = 1;
+		m->pg_init_count = 0;
 	} else {
 		m->pg_init_required = 0;
 		m->queue_io = 0;
@@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
 		must_queue = 0;
 
 	if (m->pg_init_required && !m->pg_init_in_progress) {
+		m->pg_init_count++;
 		m->pg_init_required = 0;
 		m->pg_init_in_progress = 1;
 		init_required = 1;
@@ -689,9 +693,11 @@ static int parse_features(struct arg_set
 	int r;
 	unsigned argc;
 	struct dm_target *ti = m->ti;
+	char *name;
 
 	static struct param _params[] = {
-		{0, 1, "invalid number of feature args"},
+		{0, 3, "invalid number of feature args"},
+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);
@@ -701,12 +707,23 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
-		return queue_if_no_path(m, 1, 0);
-	else {
-		ti->error = "Unrecognised multipath feature request";
-		return -EINVAL;
+	while (argc && !r) {
+		name = shift(as);
+		argc--;
+		if (!strnicmp(name, MESG_STR("queue_if_no_path")))
+			r = queue_if_no_path(m, 1, 0);
+		else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
+			 (argc >= 1)) {
+			r = read_param(_params + 1, shift(as),
+				       &m->pg_init_retries, &ti->error);
+			argc--;
+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,
@@ -976,6 +993,23 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries) {
+		m->pg_init_required = 1;
+		spin_unlock_irqrestore(&m->lock, flags);
+	} else {
+		spin_unlock_irqrestore(&m->lock, flags);
+		fail_path(pgpath);
+	}
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
@@ -995,8 +1029,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)
@@ -1149,8 +1186,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
+	else if (m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
+			m->pg_init_retries);
 	else
 		DMEMIT("0 ");
 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

[patch 0/3] Add HP hardware handler support to dm-multipath

[dwysocha]

The following 3 patches add HP hardware handler support to dm-multipath.
The first patch is very basic and provides a baseline of support but it is not
complete (has no retries, error code handling, etc).  Second and third patches
add retries and some error code handling.

I believe all comments to date have been addressed, including whitespace
issues, and passing of scripts/checkpatch.pl.


-- 

[patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[dwysocha]

[-- Attachment #1: dm-mpath-add-retry-pg-init.patch --]
[-- Type: text/plain, Size: 5444 bytes --]

This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
core.  The flag is a generic one, but in this instance we use it to flag
cases where we must retry a pg_init command.  The patch is useful for cases
where a hw handler sends a path initialization command to the storage and
it sees the command complete with an error code indicating the command
should be retried.  In this case, the hardware handler should call 
dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
that the dm-mpath core will actually retry the pg_init.  The number of actual
retries is governed by the multipath feature argument "pg_init_retries".  
Once the dm-mpath core has retried the command "pg_init_retries" times
without success, a subsequent dm_pg_init_complete() with MP_RETRY will
cause the path to be failed via fail_path().  To specify a value of
pg_init_retries, add a line similar to the following in the 'device' section
of your /etc/multipath.conf file:
features                "2 pg_init_retries 7"

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Acked-by: Mike Christie <michaelc@cs.wisc.edu>
Acked-by: Chandra Seetharaman <sekharan@us.ibm.com>

---
Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
+++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
@@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
 #define MP_FAIL_PATH 1
 #define MP_BYPASS_PG 2
 #define MP_ERROR_IO  4	/* Don't retry this I/O */
+#define MP_RETRY 8
 
 #endif
Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
+++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
@@ -75,6 +75,8 @@ struct multipath {
 	unsigned queue_io;		/* Must we queue all I/O? */
 	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
 	unsigned saved_queue_if_no_path;/* Saved state during suspension */
+	unsigned pg_init_retries;	/* Number of times to retry pg_init */
+	unsigned pg_init_count;		/* Number of times pg_init called */
 
 	struct work_struct process_queued_ios;
 	struct bio_list queued_ios;
@@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
 	if (hwh->type && hwh->type->pg_init) {
 		m->pg_init_required = 1;
 		m->queue_io = 1;
+		m->pg_init_count = 0;
 	} else {
 		m->pg_init_required = 0;
 		m->queue_io = 0;
@@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
 		must_queue = 0;
 
 	if (m->pg_init_required && !m->pg_init_in_progress) {
+		m->pg_init_count++;
 		m->pg_init_required = 0;
 		m->pg_init_in_progress = 1;
 		init_required = 1;
@@ -689,9 +693,11 @@ static int parse_features(struct arg_set
 	int r;
 	unsigned argc;
 	struct dm_target *ti = m->ti;
+	char *name;
 
 	static struct param _params[] = {
-		{0, 1, "invalid number of feature args"},
+		{0, 3, "invalid number of feature args"},
+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);
@@ -701,12 +707,23 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
-		return queue_if_no_path(m, 1, 0);
-	else {
-		ti->error = "Unrecognised multipath feature request";
-		return -EINVAL;
+	while (argc && !r) {
+		name = shift(as);
+		argc--;
+		if (!strnicmp(name, MESG_STR("queue_if_no_path")))
+			r = queue_if_no_path(m, 1, 0);
+		else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
+			 (argc >= 1)) {
+			r = read_param(_params + 1, shift(as),
+				       &m->pg_init_retries, &ti->error);
+			argc--;
+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,
@@ -976,6 +993,23 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries) {
+		m->pg_init_required = 1;
+		spin_unlock_irqrestore(&m->lock, flags);
+	} else {
+		spin_unlock_irqrestore(&m->lock, flags);
+		fail_path(pgpath);
+	}
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
@@ -995,8 +1029,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)
@@ -1149,8 +1186,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
+	else if (m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
+			m->pg_init_retries);
 	else
 		DMEMIT("0 ");
 

-- 

[patch 0/3] Add HP hardware handler support to dm-multipath

[dwysocha]

The following 3 patches add HP hardware handler support to dm-multipath.
The first patch is very basic and provides a baseline of support but it is not
complete (has no retries, error code handling, etc).  Second and third patches
add retries and some error code handling.

I believe most, if not all, comments have been addressed with these latest
patches.  Alasdair, Mike, please let me know if I missed something.

-- 

[patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[dwysocha]

[-- Attachment #1: dm-mpath-add-retry-pg-init.patch --]
[-- Type: text/plain, Size: 5358 bytes --]

This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
core.  The flag is a generic one, but in this instance we use it to flag
cases where we must retry a pg_init command.  The patch is useful for cases
where a hw handler sends a path initialization command to the storage and
it sees the command complete with an error code indicating the command
should be retried.  In this case, the hardware handler should call 
dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
that the dm-mpath core will actually retry the pg_init.  The number of actual
retries is governed by the multipath feature argument "pg_init_retries".  
Once the dm-mpath core has retried the command "pg_init_retries" times
without success, a subsequent dm_pg_init_complete() with MP_RETRY will
cause the path to be failed via fail_path().  To specify a value of
pg_init_retries, add a line similar to the following in the 'device' section
of your /etc/multipath.conf file:
features                "2 pg_init_retries 7"



Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
+++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
@@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
 #define MP_FAIL_PATH 1
 #define MP_BYPASS_PG 2
 #define MP_ERROR_IO  4	/* Don't retry this I/O */
+#define MP_RETRY 8
 
 #endif
Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
+++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
@@ -75,6 +75,8 @@ struct multipath {
 	unsigned queue_io;		/* Must we queue all I/O? */
 	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
 	unsigned saved_queue_if_no_path;/* Saved state during suspension */
+	unsigned pg_init_retries;       /* Number of times to retry pg_init */
+	unsigned pg_init_count;         /* Number of times pg_init called */
 
 	struct work_struct process_queued_ios;
 	struct bio_list queued_ios;
@@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
 	if (hwh->type && hwh->type->pg_init) {
 		m->pg_init_required = 1;
 		m->queue_io = 1;
+		m->pg_init_count = 0;
 	} else {
 		m->pg_init_required = 0;
 		m->queue_io = 0;
@@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
 		must_queue = 0;
 
 	if (m->pg_init_required && !m->pg_init_in_progress) {
+		m->pg_init_count++;
 		m->pg_init_required = 0;
 		m->pg_init_in_progress = 1;
 		init_required = 1;
@@ -689,9 +693,11 @@ static int parse_features(struct arg_set
 	int r;
 	unsigned argc;
 	struct dm_target *ti = m->ti;
+	char *name;
 
 	static struct param _params[] = {
-		{0, 1, "invalid number of feature args"},
+		{0, 4, "invalid number of feature args"},
+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);
@@ -701,12 +707,26 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
-		return queue_if_no_path(m, 1, 0);
-	else {
-		ti->error = "Unrecognised multipath feature request";
-		return -EINVAL;
+	while (argc && !r) {
+		name = shift(as);
+		argc--;
+		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
+			r = queue_if_no_path(m, 1, 0);
+			DMDEBUG("setting queue_if_no_path");
+		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
+			   (argc >= 1)) {
+			r = read_param(_params + 1, shift(as),
+					&m->pg_init_retries, &ti->error);
+			argc--;
+			DMDEBUG("setting pg_init_retries to %u",
+				m->pg_init_retries);
+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,
@@ -976,6 +996,21 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries)
+		m->pg_init_required = 1;
+	else
+		fail_path(pgpath);
+	spin_unlock_irqrestore(&m->lock, flags);
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
@@ -995,8 +1030,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)
@@ -1149,8 +1187,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
+	else if (m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
+			m->pg_init_retries);
 	else
 		DMEMIT("0 ");
 

-- 

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Mike Christie]

dwysocha@redhat.com wrote:


looks ok

Acked-by: Mike Christie <michaelc@cs.wisc.edu>

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Dave Wysochanski]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

On Thu, 2007-07-26 at 10:20 -0500, Mike Christie wrote:
> dwysocha@redhat.com wrote:
> 
> 
> looks ok
> 
> Acked-by: Mike Christie <michaelc@cs.wisc.edu>
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

There was actually a locking bug in the retry_pg() function (need to
drop m->lock before calling fail_path, since fail_path grabs m->lock).
I found this after realizing my original tests did not exercise the
retry path very well and did some more thorough tests.  Attached patch
fixes it.



[-- Attachment #2: dm-mpath-add-retry-pg-init.patch --]
[-- Type: text/x-patch, Size: 5582 bytes --]

Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
core.  The flag is a generic one, but in this instance we use it to flag
cases where we must retry a pg_init command.  The patch is useful for cases
where a hw handler sends a path initialization command to the storage and
it sees the command complete with an error code indicating the command
should be retried.  In this case, the hardware handler should call 
dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
that the dm-mpath core will actually retry the pg_init.  The number of actual
retries is governed by the multipath feature argument "pg_init_retries".  
Once the dm-mpath core has retried the command "pg_init_retries" times
without success, a subsequent dm_pg_init_complete() with MP_RETRY will
cause the path to be failed via fail_path().  To specify a value of
pg_init_retries, add a line similar to the following in the 'device' section
of your /etc/multipath.conf file:
features                "2 pg_init_retries 7"

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Acked-by: Mike Christie <michaelc@cs.wisc.edu>

---
Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
+++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
@@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
 #define MP_FAIL_PATH 1
 #define MP_BYPASS_PG 2
 #define MP_ERROR_IO  4	/* Don't retry this I/O */
+#define MP_RETRY 8
 
 #endif
Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
+++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
@@ -75,6 +75,8 @@ struct multipath {
 	unsigned queue_io;		/* Must we queue all I/O? */
 	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
 	unsigned saved_queue_if_no_path;/* Saved state during suspension */
+	unsigned pg_init_retries;       /* Number of times to retry pg_init */
+	unsigned pg_init_count;         /* Number of times pg_init called */
 
 	struct work_struct process_queued_ios;
 	struct bio_list queued_ios;
@@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
 	if (hwh->type && hwh->type->pg_init) {
 		m->pg_init_required = 1;
 		m->queue_io = 1;
+		m->pg_init_count = 0;
 	} else {
 		m->pg_init_required = 0;
 		m->queue_io = 0;
@@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
 		must_queue = 0;
 
 	if (m->pg_init_required && !m->pg_init_in_progress) {
+		m->pg_init_count++;
 		m->pg_init_required = 0;
 		m->pg_init_in_progress = 1;
 		init_required = 1;
@@ -689,9 +693,11 @@ static int parse_features(struct arg_set
 	int r;
 	unsigned argc;
 	struct dm_target *ti = m->ti;
+	char *name;
 
 	static struct param _params[] = {
-		{0, 1, "invalid number of feature args"},
+		{0, 4, "invalid number of feature args"},
+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);
@@ -701,12 +707,26 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
-		return queue_if_no_path(m, 1, 0);
-	else {
-		ti->error = "Unrecognised multipath feature request";
-		return -EINVAL;
+	while (argc && !r) {
+		name = shift(as);
+		argc--;
+		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
+			r = queue_if_no_path(m, 1, 0);
+			DMDEBUG("setting queue_if_no_path");
+		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
+			   (argc >= 1)) {
+			r = read_param(_params + 1, shift(as),
+					&m->pg_init_retries, &ti->error);
+			argc--;
+			DMDEBUG("setting pg_init_retries to %u",
+				m->pg_init_retries);
+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,
@@ -976,6 +996,23 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries) {
+		m->pg_init_required = 1;
+		spin_unlock_irqrestore(&m->lock, flags);
+	} else {
+		spin_unlock_irqrestore(&m->lock, flags);
+		fail_path(pgpath);
+	}
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
@@ -995,8 +1032,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)
@@ -1149,8 +1189,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
+	else if (m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
+			m->pg_init_retries);
 	else
 		DMEMIT("0 ");
 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Chandra Seetharaman]

On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> plain text document attachment (dm-mpath-add-retry-pg-init.patch)
> This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
> core.  The flag is a generic one, but in this instance we use it to flag
> cases where we must retry a pg_init command.  The patch is useful for cases
> where a hw handler sends a path initialization command to the storage and
> it sees the command complete with an error code indicating the command
> should be retried.  In this case, the hardware handler should call 
> dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
> to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
> that the dm-mpath core will actually retry the pg_init.  The number of actual
> retries is governed by the multipath feature argument "pg_init_retries".  
> Once the dm-mpath core has retried the command "pg_init_retries" times
> without success, a subsequent dm_pg_init_complete() with MP_RETRY will
> cause the path to be failed via fail_path().  To specify a value of
> pg_init_retries, add a line similar to the following in the 'device' section
> of your /etc/multipath.conf file:
> features                "2 pg_init_retries 7"
> 
> 
> 
> Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> ===================================================================
> --- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
> +++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> @@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
>  #define MP_FAIL_PATH 1
>  #define MP_BYPASS_PG 2
>  #define MP_ERROR_IO  4	/* Don't retry this I/O */
> +#define MP_RETRY 8
> 
>  #endif
> Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
> ===================================================================
> --- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
> +++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
> @@ -75,6 +75,8 @@ struct multipath {
>  	unsigned queue_io;		/* Must we queue all I/O? */
>  	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
>  	unsigned saved_queue_if_no_path;/* Saved state during suspension */
> +	unsigned pg_init_retries;       /* Number of times to retry pg_init */
> +	unsigned pg_init_count;         /* Number of times pg_init called */
> 
>  	struct work_struct process_queued_ios;
>  	struct bio_list queued_ios;
> @@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
>  	if (hwh->type && hwh->type->pg_init) {
>  		m->pg_init_required = 1;
>  		m->queue_io = 1;
> +		m->pg_init_count = 0;
>  	} else {
>  		m->pg_init_required = 0;
>  		m->queue_io = 0;
> @@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
>  		must_queue = 0;
> 
>  	if (m->pg_init_required && !m->pg_init_in_progress) {
> +		m->pg_init_count++;
>  		m->pg_init_required = 0;
>  		m->pg_init_in_progress = 1;
>  		init_required = 1;
> @@ -689,9 +693,11 @@ static int parse_features(struct arg_set
>  	int r;
>  	unsigned argc;
>  	struct dm_target *ti = m->ti;
> +	char *name;
> 
>  	static struct param _params[] = {
> -		{0, 1, "invalid number of feature args"},
> +		{0, 4, "invalid number of feature args"},

Isn't it "3" (instead of "4") ?

> +		{0, 50, "invalid number of pg_init retries"},
>  	};
> 
>  	r = read_param(_params, shift(as), &argc, &ti->error);
> @@ -701,12 +707,26 @@ static int parse_features(struct arg_set
>  	if (!argc)
>  		return 0;
> 
> -	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
> -		return queue_if_no_path(m, 1, 0);
> -	else {
> -		ti->error = "Unrecognised multipath feature request";
> -		return -EINVAL;
> +	while (argc && !r) {
> +		name = shift(as);
> +		argc--;
> +		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
> +			r = queue_if_no_path(m, 1, 0);
> +			DMDEBUG("setting queue_if_no_path");

Shouldn't this DEBUG be printed only when r == 0 ?

> +		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
> +			   (argc >= 1)) {

mixed use of space/tab.
> +			r = read_param(_params + 1, shift(as),
> +					&m->pg_init_retries, &ti->error);
> +			argc--;
> +			DMDEBUG("setting pg_init_retries to %u",
> +				m->pg_init_retries);

Shouldn't this DEBUG be printed only when r == 0 ?
> +		} else {
> +			ti->error = "Unrecognised multipath feature request";
> +			return -EINVAL;
> +		}
>  	}
> +
> +	return r;
>  }
> 
>  static int multipath_ctr(struct dm_target *ti, unsigned int argc,
> @@ -976,6 +996,21 @@ static int bypass_pg_num(struct multipat
>  }
> 
>  /*
> + * Retry pg_init on the same path group and path
> + */
> +static void retry_pg(struct multipath *m, struct pgpath *pgpath)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&m->lock, flags);
> +	if (m->pg_init_count <= m->pg_init_retries)
> +		m->pg_init_required = 1;
> +	else
> +		fail_path(pgpath);
> +	spin_unlock_irqrestore(&m->lock, flags);
> +}
> +
> +/*
>   * pg_init must call this when it has completed its initialisation
>   */
>  void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
> @@ -995,8 +1030,11 @@ void dm_pg_init_complete(struct dm_path 
>  	if (err_flags & MP_BYPASS_PG)
>  		bypass_pg(m, pg, 1);
> 
> +	if (err_flags & MP_RETRY)
> +		retry_pg(m, pgpath);
> +
>  	spin_lock_irqsave(&m->lock, flags);
> -	if (err_flags) {
> +	if (err_flags & ~MP_RETRY) {
>  		m->current_pgpath = NULL;
>  		m->current_pg = NULL;
>  	} else if (!m->pg_init_required)
> @@ -1149,8 +1187,13 @@ static int multipath_status(struct dm_ta
>  	/* Features */
>  	if (type == STATUSTYPE_INFO)
>  		DMEMIT("1 %u ", m->queue_size);
> -	else if (m->queue_if_no_path)
> +	else if (m->queue_if_no_path && !m->pg_init_retries)
>  		DMEMIT("1 queue_if_no_path ");
> +	else if (!m->queue_if_no_path && m->pg_init_retries)
> +		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
> +	else if (m->queue_if_no_path && m->pg_init_retries)
> +		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
> +			m->pg_init_retries);
>  	else
>  		DMEMIT("0 ");
> 
> 
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Dave Wysochanski]

[-- Attachment #1: Type: text/plain, Size: 6482 bytes --]

On Thu, 2007-07-26 at 12:15 -0700, Chandra Seetharaman wrote:
> On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> > plain text document attachment (dm-mpath-add-retry-pg-init.patch)
> > This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
> > core.  The flag is a generic one, but in this instance we use it to flag
> > cases where we must retry a pg_init command.  The patch is useful for cases
> > where a hw handler sends a path initialization command to the storage and
> > it sees the command complete with an error code indicating the command
> > should be retried.  In this case, the hardware handler should call 
> > dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
> > to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
> > that the dm-mpath core will actually retry the pg_init.  The number of actual
> > retries is governed by the multipath feature argument "pg_init_retries".  
> > Once the dm-mpath core has retried the command "pg_init_retries" times
> > without success, a subsequent dm_pg_init_complete() with MP_RETRY will
> > cause the path to be failed via fail_path().  To specify a value of
> > pg_init_retries, add a line similar to the following in the 'device' section
> > of your /etc/multipath.conf file:
> > features                "2 pg_init_retries 7"
> > 
> > 
> > 
> > Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> > ===================================================================
> > --- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
> > +++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> > @@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
> >  #define MP_FAIL_PATH 1
> >  #define MP_BYPASS_PG 2
> >  #define MP_ERROR_IO  4	/* Don't retry this I/O */
> > +#define MP_RETRY 8
> > 
> >  #endif
> > Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
> > ===================================================================
> > --- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
> > +++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
> > @@ -75,6 +75,8 @@ struct multipath {
> >  	unsigned queue_io;		/* Must we queue all I/O? */
> >  	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
> >  	unsigned saved_queue_if_no_path;/* Saved state during suspension */
> > +	unsigned pg_init_retries;       /* Number of times to retry pg_init */
> > +	unsigned pg_init_count;         /* Number of times pg_init called */
> > 
> >  	struct work_struct process_queued_ios;
> >  	struct bio_list queued_ios;
> > @@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
> >  	if (hwh->type && hwh->type->pg_init) {
> >  		m->pg_init_required = 1;
> >  		m->queue_io = 1;
> > +		m->pg_init_count = 0;
> >  	} else {
> >  		m->pg_init_required = 0;
> >  		m->queue_io = 0;
> > @@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
> >  		must_queue = 0;
> > 
> >  	if (m->pg_init_required && !m->pg_init_in_progress) {
> > +		m->pg_init_count++;
> >  		m->pg_init_required = 0;
> >  		m->pg_init_in_progress = 1;
> >  		init_required = 1;
> > @@ -689,9 +693,11 @@ static int parse_features(struct arg_set
> >  	int r;
> >  	unsigned argc;
> >  	struct dm_target *ti = m->ti;
> > +	char *name;
> > 
> >  	static struct param _params[] = {
> > -		{0, 1, "invalid number of feature args"},
> > +		{0, 4, "invalid number of feature args"},
> 
> Isn't it "3" (instead of "4") ?
> 
> > +		{0, 50, "invalid number of pg_init retries"},
> >  	};
> > 
> >  	r = read_param(_params, shift(as), &argc, &ti->error);
> > @@ -701,12 +707,26 @@ static int parse_features(struct arg_set
> >  	if (!argc)
> >  		return 0;
> > 
> > -	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
> > -		return queue_if_no_path(m, 1, 0);
> > -	else {
> > -		ti->error = "Unrecognised multipath feature request";
> > -		return -EINVAL;
> > +	while (argc && !r) {
> > +		name = shift(as);
> > +		argc--;
> > +		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
> > +			r = queue_if_no_path(m, 1, 0);
> > +			DMDEBUG("setting queue_if_no_path");
> 
> Shouldn't this DEBUG be printed only when r == 0 ?
> 
> > +		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
> > +			   (argc >= 1)) {
> 
> mixed use of space/tab.
> > +			r = read_param(_params + 1, shift(as),
> > +					&m->pg_init_retries, &ti->error);
> > +			argc--;
> > +			DMDEBUG("setting pg_init_retries to %u",
> > +				m->pg_init_retries);
> 
> Shouldn't this DEBUG be printed only when r == 0 ?
> > +		} else {
> > +			ti->error = "Unrecognised multipath feature request";
> > +			return -EINVAL;
> > +		}
> >  	}
> > +
> > +	return r;
> >  }
> > 
> >  static int multipath_ctr(struct dm_target *ti, unsigned int argc,
> > @@ -976,6 +996,21 @@ static int bypass_pg_num(struct multipat
> >  }
> > 
> >  /*
> > + * Retry pg_init on the same path group and path
> > + */
> > +static void retry_pg(struct multipath *m, struct pgpath *pgpath)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&m->lock, flags);
> > +	if (m->pg_init_count <= m->pg_init_retries)
> > +		m->pg_init_required = 1;
> > +	else
> > +		fail_path(pgpath);
> > +	spin_unlock_irqrestore(&m->lock, flags);
> > +}
> > +
> > +/*
> >   * pg_init must call this when it has completed its initialisation
> >   */
> >  void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
> > @@ -995,8 +1030,11 @@ void dm_pg_init_complete(struct dm_path 
> >  	if (err_flags & MP_BYPASS_PG)
> >  		bypass_pg(m, pg, 1);
> > 
> > +	if (err_flags & MP_RETRY)
> > +		retry_pg(m, pgpath);
> > +
> >  	spin_lock_irqsave(&m->lock, flags);
> > -	if (err_flags) {
> > +	if (err_flags & ~MP_RETRY) {
> >  		m->current_pgpath = NULL;
> >  		m->current_pg = NULL;
> >  	} else if (!m->pg_init_required)
> > @@ -1149,8 +1187,13 @@ static int multipath_status(struct dm_ta
> >  	/* Features */
> >  	if (type == STATUSTYPE_INFO)
> >  		DMEMIT("1 %u ", m->queue_size);
> > -	else if (m->queue_if_no_path)
> > +	else if (m->queue_if_no_path && !m->pg_init_retries)
> >  		DMEMIT("1 queue_if_no_path ");
> > +	else if (!m->queue_if_no_path && m->pg_init_retries)
> > +		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
> > +	else if (m->queue_if_no_path && m->pg_init_retries)
> > +		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
> > +			m->pg_init_retries);
> >  	else
> >  		DMEMIT("0 ");
> > 
> > 

The attached patch should address your comments.  I removed the DMDEBUG
statements as they did not seem too useful beyond basic tests.



[-- Attachment #2: dm-mpath-add-retry-pg-init.patch --]
[-- Type: text/x-patch, Size: 5468 bytes --]

Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
core.  The flag is a generic one, but in this instance we use it to flag
cases where we must retry a pg_init command.  The patch is useful for cases
where a hw handler sends a path initialization command to the storage and
it sees the command complete with an error code indicating the command
should be retried.  In this case, the hardware handler should call 
dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
that the dm-mpath core will actually retry the pg_init.  The number of actual
retries is governed by the multipath feature argument "pg_init_retries".  
Once the dm-mpath core has retried the command "pg_init_retries" times
without success, a subsequent dm_pg_init_complete() with MP_RETRY will
cause the path to be failed via fail_path().  To specify a value of
pg_init_retries, add a line similar to the following in the 'device' section
of your /etc/multipath.conf file:
features                "2 pg_init_retries 7"

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Acked-by: Mike Christie <michaelc@cs.wisc.edu>

---
Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
+++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
@@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
 #define MP_FAIL_PATH 1
 #define MP_BYPASS_PG 2
 #define MP_ERROR_IO  4	/* Don't retry this I/O */
+#define MP_RETRY 8
 
 #endif
Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
+++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
@@ -75,6 +75,8 @@ struct multipath {
 	unsigned queue_io;		/* Must we queue all I/O? */
 	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
 	unsigned saved_queue_if_no_path;/* Saved state during suspension */
+	unsigned pg_init_retries;       /* Number of times to retry pg_init */
+	unsigned pg_init_count;         /* Number of times pg_init called */
 
 	struct work_struct process_queued_ios;
 	struct bio_list queued_ios;
@@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
 	if (hwh->type && hwh->type->pg_init) {
 		m->pg_init_required = 1;
 		m->queue_io = 1;
+		m->pg_init_count = 0;
 	} else {
 		m->pg_init_required = 0;
 		m->queue_io = 0;
@@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
 		must_queue = 0;
 
 	if (m->pg_init_required && !m->pg_init_in_progress) {
+		m->pg_init_count++;
 		m->pg_init_required = 0;
 		m->pg_init_in_progress = 1;
 		init_required = 1;
@@ -689,9 +693,11 @@ static int parse_features(struct arg_set
 	int r;
 	unsigned argc;
 	struct dm_target *ti = m->ti;
+	char *name;
 
 	static struct param _params[] = {
-		{0, 1, "invalid number of feature args"},
+		{0, 3, "invalid number of feature args"},
+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);
@@ -701,12 +707,23 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
-		return queue_if_no_path(m, 1, 0);
-	else {
-		ti->error = "Unrecognised multipath feature request";
-		return -EINVAL;
+	while (argc && !r) {
+		name = shift(as);
+		argc--;
+		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
+			r = queue_if_no_path(m, 1, 0);
+		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
+				(argc >= 1)) {
+			r = read_param(_params + 1, shift(as),
+					&m->pg_init_retries, &ti->error);
+			argc--;
+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,
@@ -976,6 +993,23 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries) {
+		m->pg_init_required = 1;
+		spin_unlock_irqrestore(&m->lock, flags);
+	} else {
+		spin_unlock_irqrestore(&m->lock, flags);
+		fail_path(pgpath);
+	}
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
@@ -995,8 +1029,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)
@@ -1149,8 +1186,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
+	else if (m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
+			m->pg_init_retries);
 	else
 		DMEMIT("0 ");
 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Chandra Seetharaman]

Definitions of pg_init_retries and pg_init_count still uses spaces
(instead of tabs, which is the case with rest of the definitions around.

Other than that it looks good.

thanks

chandra
On Mon, 2007-07-30 at 14:54 -0400, Dave Wysochanski wrote:
> On Thu, 2007-07-26 at 12:15 -0700, Chandra Seetharaman wrote:
> > On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> > > plain text document attachment (dm-mpath-add-retry-pg-init.patch)
> > > This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
> > > core.  The flag is a generic one, but in this instance we use it to flag
> > > cases where we must retry a pg_init command.  The patch is useful for cases
> > > where a hw handler sends a path initialization command to the storage and
> > > it sees the command complete with an error code indicating the command
> > > should be retried.  In this case, the hardware handler should call 
> > > dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
> > > to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
> > > that the dm-mpath core will actually retry the pg_init.  The number of actual
> > > retries is governed by the multipath feature argument "pg_init_retries".  
> > > Once the dm-mpath core has retried the command "pg_init_retries" times
> > > without success, a subsequent dm_pg_init_complete() with MP_RETRY will
> > > cause the path to be failed via fail_path().  To specify a value of
> > > pg_init_retries, add a line similar to the following in the 'device' section
> > > of your /etc/multipath.conf file:
> > > features                "2 pg_init_retries 7"
> > > 
> > > 
> > > 
> > > Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> > > ===================================================================
> > > --- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
> > > +++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> > > @@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
> > >  #define MP_FAIL_PATH 1
> > >  #define MP_BYPASS_PG 2
> > >  #define MP_ERROR_IO  4	/* Don't retry this I/O */
> > > +#define MP_RETRY 8
> > > 
> > >  #endif
> > > Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
> > > ===================================================================
> > > --- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
> > > +++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
> > > @@ -75,6 +75,8 @@ struct multipath {
> > >  	unsigned queue_io;		/* Must we queue all I/O? */
> > >  	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
> > >  	unsigned saved_queue_if_no_path;/* Saved state during suspension */
> > > +	unsigned pg_init_retries;       /* Number of times to retry pg_init */
> > > +	unsigned pg_init_count;         /* Number of times pg_init called */
> > > 
> > >  	struct work_struct process_queued_ios;
> > >  	struct bio_list queued_ios;
> > > @@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
> > >  	if (hwh->type && hwh->type->pg_init) {
> > >  		m->pg_init_required = 1;
> > >  		m->queue_io = 1;
> > > +		m->pg_init_count = 0;
> > >  	} else {
> > >  		m->pg_init_required = 0;
> > >  		m->queue_io = 0;
> > > @@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
> > >  		must_queue = 0;
> > > 
> > >  	if (m->pg_init_required && !m->pg_init_in_progress) {
> > > +		m->pg_init_count++;
> > >  		m->pg_init_required = 0;
> > >  		m->pg_init_in_progress = 1;
> > >  		init_required = 1;
> > > @@ -689,9 +693,11 @@ static int parse_features(struct arg_set
> > >  	int r;
> > >  	unsigned argc;
> > >  	struct dm_target *ti = m->ti;
> > > +	char *name;
> > > 
> > >  	static struct param _params[] = {
> > > -		{0, 1, "invalid number of feature args"},
> > > +		{0, 4, "invalid number of feature args"},
> > 
> > Isn't it "3" (instead of "4") ?
> > 
> > > +		{0, 50, "invalid number of pg_init retries"},
> > >  	};
> > > 
> > >  	r = read_param(_params, shift(as), &argc, &ti->error);
> > > @@ -701,12 +707,26 @@ static int parse_features(struct arg_set
> > >  	if (!argc)
> > >  		return 0;
> > > 
> > > -	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
> > > -		return queue_if_no_path(m, 1, 0);
> > > -	else {
> > > -		ti->error = "Unrecognised multipath feature request";
> > > -		return -EINVAL;
> > > +	while (argc && !r) {
> > > +		name = shift(as);
> > > +		argc--;
> > > +		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
> > > +			r = queue_if_no_path(m, 1, 0);
> > > +			DMDEBUG("setting queue_if_no_path");
> > 
> > Shouldn't this DEBUG be printed only when r == 0 ?
> > 
> > > +		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
> > > +			   (argc >= 1)) {
> > 
> > mixed use of space/tab.
> > > +			r = read_param(_params + 1, shift(as),
> > > +					&m->pg_init_retries, &ti->error);
> > > +			argc--;
> > > +			DMDEBUG("setting pg_init_retries to %u",
> > > +				m->pg_init_retries);
> > 
> > Shouldn't this DEBUG be printed only when r == 0 ?
> > > +		} else {
> > > +			ti->error = "Unrecognised multipath feature request";
> > > +			return -EINVAL;
> > > +		}
> > >  	}
> > > +
> > > +	return r;
> > >  }
> > > 
> > >  static int multipath_ctr(struct dm_target *ti, unsigned int argc,
> > > @@ -976,6 +996,21 @@ static int bypass_pg_num(struct multipat
> > >  }
> > > 
> > >  /*
> > > + * Retry pg_init on the same path group and path
> > > + */
> > > +static void retry_pg(struct multipath *m, struct pgpath *pgpath)
> > > +{
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&m->lock, flags);
> > > +	if (m->pg_init_count <= m->pg_init_retries)
> > > +		m->pg_init_required = 1;
> > > +	else
> > > +		fail_path(pgpath);
> > > +	spin_unlock_irqrestore(&m->lock, flags);
> > > +}
> > > +
> > > +/*
> > >   * pg_init must call this when it has completed its initialisation
> > >   */
> > >  void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
> > > @@ -995,8 +1030,11 @@ void dm_pg_init_complete(struct dm_path 
> > >  	if (err_flags & MP_BYPASS_PG)
> > >  		bypass_pg(m, pg, 1);
> > > 
> > > +	if (err_flags & MP_RETRY)
> > > +		retry_pg(m, pgpath);
> > > +
> > >  	spin_lock_irqsave(&m->lock, flags);
> > > -	if (err_flags) {
> > > +	if (err_flags & ~MP_RETRY) {
> > >  		m->current_pgpath = NULL;
> > >  		m->current_pg = NULL;
> > >  	} else if (!m->pg_init_required)
> > > @@ -1149,8 +1187,13 @@ static int multipath_status(struct dm_ta
> > >  	/* Features */
> > >  	if (type == STATUSTYPE_INFO)
> > >  		DMEMIT("1 %u ", m->queue_size);
> > > -	else if (m->queue_if_no_path)
> > > +	else if (m->queue_if_no_path && !m->pg_init_retries)
> > >  		DMEMIT("1 queue_if_no_path ");
> > > +	else if (!m->queue_if_no_path && m->pg_init_retries)
> > > +		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
> > > +	else if (m->queue_if_no_path && m->pg_init_retries)
> > > +		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
> > > +			m->pg_init_retries);
> > >  	else
> > >  		DMEMIT("0 ");
> > > 
> > > 
> 
> The attached patch should address your comments.  I removed the DMDEBUG
> statements as they did not seem too useful beyond basic tests.
> 
> 
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Mike Christie]

Chandra Seetharaman wrote:
> Definitions of pg_init_retries and pg_init_count still uses spaces
> (instead of tabs, which is the case with rest of the definitions around.
> 

I agree with Chandra on that one though.

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Dave Wysochanski]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

On Mon, 2007-07-30 at 16:11 -0500, Mike Christie wrote:
> Chandra Seetharaman wrote:
> > Definitions of pg_init_retries and pg_init_count still uses spaces
> > (instead of tabs, which is the case with rest of the definitions around.
> > 
> 
> I agree with Chandra on that one though.

See attached.



[-- Attachment #2: dm-mpath-add-retry-pg-init.patch --]
[-- Type: text/x-patch, Size: 5515 bytes --]

Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
core.  The flag is a generic one, but in this instance we use it to flag
cases where we must retry a pg_init command.  The patch is useful for cases
where a hw handler sends a path initialization command to the storage and
it sees the command complete with an error code indicating the command
should be retried.  In this case, the hardware handler should call 
dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
that the dm-mpath core will actually retry the pg_init.  The number of actual
retries is governed by the multipath feature argument "pg_init_retries".  
Once the dm-mpath core has retried the command "pg_init_retries" times
without success, a subsequent dm_pg_init_complete() with MP_RETRY will
cause the path to be failed via fail_path().  To specify a value of
pg_init_retries, add a line similar to the following in the 'device' section
of your /etc/multipath.conf file:
features                "2 pg_init_retries 7"

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Acked-by: Mike Christie <michaelc@cs.wisc.edu>
Acked-by: Chandra Seetharaman <sekharan@us.ibm.com>

---
Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
+++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
@@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
 #define MP_FAIL_PATH 1
 #define MP_BYPASS_PG 2
 #define MP_ERROR_IO  4	/* Don't retry this I/O */
+#define MP_RETRY 8
 
 #endif
Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
+++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
@@ -75,6 +75,8 @@ struct multipath {
 	unsigned queue_io;		/* Must we queue all I/O? */
 	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
 	unsigned saved_queue_if_no_path;/* Saved state during suspension */
+	unsigned pg_init_retries;	/* Number of times to retry pg_init */
+	unsigned pg_init_count;		/* Number of times pg_init called */
 
 	struct work_struct process_queued_ios;
 	struct bio_list queued_ios;
@@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
 	if (hwh->type && hwh->type->pg_init) {
 		m->pg_init_required = 1;
 		m->queue_io = 1;
+		m->pg_init_count = 0;
 	} else {
 		m->pg_init_required = 0;
 		m->queue_io = 0;
@@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
 		must_queue = 0;
 
 	if (m->pg_init_required && !m->pg_init_in_progress) {
+		m->pg_init_count++;
 		m->pg_init_required = 0;
 		m->pg_init_in_progress = 1;
 		init_required = 1;
@@ -689,9 +693,11 @@ static int parse_features(struct arg_set
 	int r;
 	unsigned argc;
 	struct dm_target *ti = m->ti;
+	char *name;
 
 	static struct param _params[] = {
-		{0, 1, "invalid number of feature args"},
+		{0, 3, "invalid number of feature args"},
+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);
@@ -701,12 +707,23 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
-		return queue_if_no_path(m, 1, 0);
-	else {
-		ti->error = "Unrecognised multipath feature request";
-		return -EINVAL;
+	while (argc && !r) {
+		name = shift(as);
+		argc--;
+		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
+			r = queue_if_no_path(m, 1, 0);
+		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
+			   (argc >= 1)) {
+			r = read_param(_params + 1, shift(as),
+				       &m->pg_init_retries, &ti->error);
+			argc--;
+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,
@@ -976,6 +993,23 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries) {
+		m->pg_init_required = 1;
+		spin_unlock_irqrestore(&m->lock, flags);
+	} else {
+		spin_unlock_irqrestore(&m->lock, flags);
+		fail_path(pgpath);
+	}
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
@@ -995,8 +1029,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)
@@ -1149,8 +1186,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
+	else if (m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
+			m->pg_init_retries);
 	else
 		DMEMIT("0 ");
 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]