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

[Chandra Seetharaman <sekharan@us.ibm.com>]

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.
----------------------------------------------------------------------