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

Dave Wysochanski dwysocha at redhat.com
Mon Jul 30 18:54:01 UTC 2007


On Thu, 2007-07-26 at 12:15 -0700, Chandra Seetharaman wrote:
> On Thu, 2007-07-26 at 00:44 -0400, dwysocha at 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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: dm-mpath-add-retry-pg-init.patch
Type: text/x-patch
Size: 5468 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20070730/7276c9db/attachment.bin>


More information about the dm-devel mailing list