[dm-devel] RE: patch for handling clariion I/O to

[Edward Goggin]

> -----Original Message-----
> From: Christophe Varoqui [mailto:christophe.varoqui at free.fr] 
> Sent: Wednesday, October 04, 2006 6:09 PM
> To: goggin, edward
> Cc: device-mapper development
> Subject: Re: patch for handling clariion I/O to
> 
> Le mercredi 04 octobre 2006 à 16:01 -0400, Edward Goggin a écrit :
> > Christophe,
> > 
> > This patch attempts to prevent an I/O hang which can occur 
> with host I/O
> > to an "in-active" CLARiiON snapshot LU.  While the snapshot LU is
> > presented to the host, inquiry, TUR, and read capacity 
> commands succeed,
> > but read and write commands are failed.
> > 
> > The patch is not entirely CLARiiON specific since I've 
> added a field to
> > the multipath structure (failallpaths) and I've modified 
> the interface
> > of the sg_read function in libcheckers/readsector0.c.  If you prefer
> > that I do this with a solution which is entirely isolated to the
> > CLARiiON, I can do that instead.
> > 
> 
> I don't see you ever reseting "allfailpaths". Isn't it a problem ?
>
 
I'm counting on the MALLOC macro, (defined in memory.h and called from
alloc_multipath) to call zalloc to zero/reset all fields of struct multipath.

> If "failallpaths" usage is limited to "emc_clariion.c", I would rather
> see it added to the "struct emc_clariion_checker_context" instead of
> "struct multipath" :
> 

I would also.  I will need to add logic to this code to coalesce paths
to common block devices since that path checker code only deals with
individual paths.  I'll do that though since that approach is a more
isolated solution in addition to having the two advantages you've
pointed out below.

> - no need to include libmultipath headers in checkers
> - no need to declare (and use) the "container_of" macro ;)
> 
> 
> The sg_read() change looks opportune indeed. I'll apply it as 
> soon as we
> have the other pieces sorted out.
> 

Ok.  I'll try to have something early next week.

> Thanks,
> cvaroqui
> 
> 
> 
> > diff --git a/libcheckers/emc_clariion.c b/libcheckers/emc_clariion.c
> > index a883e3d..e4004a7 100644
> > --- a/libcheckers/emc_clariion.c
> > +++ b/libcheckers/emc_clariion.c
> > @@ -3,6 +3,7 @@
> >   */
> >  #include <stdio.h>
> >  #include <stdlib.h>
> > +#include <stddef.h>
> >  #include <string.h>
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> > @@ -14,6 +15,14 @@
> >  #include "checkers.h"
> >  
> >  #include "../libmultipath/sg_include.h"
> > +#include "../libmultipath/vector.h"
> > +#include "../libmultipath/structs.h"
> > +
> > +#define container_of(ptr, type, member) ({			\
> > +        const typeof( ((type *)0)->member ) *__mptr = 
> (ptr);	\
> > +        (type *)( (char *)__mptr - offsetof(type,member) );})
> > +
> > +int sg_read (int sg_fd, unsigned char * buff, unsigned char *
> > senseBuff);
> >  
> >  #define INQUIRY_CMD     0x12
> >  #define INQUIRY_CMDLEN  6
> > @@ -40,13 +49,18 @@ void emc_clariion_free (struct checker *
> >  
> >  int emc_clariion(struct checker * c)
> >  {
> > -	unsigned char sense_buffer[256] = { 0, };
> > +	unsigned char sense_buffer[256] = { 0, }, *sbb;
> >  	unsigned char sb[128] = { 0, };
> >  	unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0,
> > 0,
> >  						sizeof(sb), 0};
> >  	struct sg_io_hdr io_hdr;
> >  	struct emc_clariion_checker_context * ct =
> >  		(struct emc_clariion_checker_context *)c->context;
> > +	struct path *p;
> > +	int ret;
> > +
> > +	p = container_of(c, struct path, checker);
> > +
> >  
> >  	memset(&io_hdr, 0, sizeof (struct sg_io_hdr));
> >  	io_hdr.interface_id = 'S';
> > @@ -110,7 +124,27 @@ int emc_clariion(struct checker * c)
> >  		ct->wwn_set = 1;
> >  	}
> >  	
> > -	
> > -	MSG(c, "emc_clariion_checker: Path healthy");
> > -        return PATH_UP;
> > +	if (p->mpp && p->mpp->failallpaths) {
> > +		ret = PATH_DOWN;
> > +		MSG(c, "emc_clariion_checker: Path to inactive
> > snapshot.");
> > +	}
> > +	else {
> > +		unsigned char buf[512];
> > +
> > +		ret = sg_read(c->fd, &buf[0], sbb = &sense_buffer[0]);
> > +
> > +		if (((sbb[2]&0xf) == 5) && (sbb[12] == 0x25) &&
> > (sbb[13]==1)) {
> > +			if (p->mpp)
> > +				p->mpp->failallpaths = 1;
> > +			MSG(c, "emc_clariion_checker: Path to inactive
> > snapshot.");
> > +			ret = PATH_DOWN;
> > +		}
> > +		else if (((sbb[2]&0xf)==2) && (sbb[12]==4) &&
> > (sbb[13]==3)) {
> > +			MSG(c, "emc_clariion_checker: Passive path is
> > healthy.");
> > +			ret = PATH_UP;	/* not ghost */
> > +		}
> > +		else
> > +			MSG(c, "emc_clariion_checker: Active path is
> > healthy.");
> > +	}
> > +	return ret;
> >  }
> > diff --git a/libcheckers/readsector0.c b/libcheckers/readsector0.c
> > index b0acec9..dd18528 100644
> > --- a/libcheckers/readsector0.c
> > +++ b/libcheckers/readsector0.c
> > @@ -34,8 +34,8 @@ void readsector0_free (struct checker * 
> >  	return;
> >  }
> >  
> > -static int
> > -sg_read (int sg_fd, unsigned char * buff)
> > +int
> > +sg_read (int sg_fd, unsigned char * buff, unsigned char * 
> senseBuff)
> >  {
> >  	/* defaults */
> >  	int blocks = 1;
> > @@ -45,7 +45,6 @@ sg_read (int sg_fd, unsigned char * buff
> >  	int * diop = NULL;
> >  
> >  	unsigned char rdCmd[cdbsz];
> > -	unsigned char senseBuff[SENSE_BUFF_LEN];
> >  	struct sg_io_hdr io_hdr;
> >  	int res;
> >  	int rd_opcode[] = {0x8, 0x28, 0xa8, 0x88};
> > @@ -97,9 +96,10 @@ extern int
> >  readsector0 (struct checker * c)
> >  {
> >  	unsigned char buf[512];
> > +	unsigned char sbuf[SENSE_BUFF_LEN];
> >  	int ret;
> >  
> > -	ret = sg_read(c->fd, &buf[0]);
> > +	ret = sg_read(c->fd, &buf[0], &sbuf[0]);
> >  
> >  	switch (ret)
> >  	{
> > diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> > index faa2b8f..049c82b 100644
> > --- a/libmultipath/structs.h
> > +++ b/libmultipath/structs.h
> > @@ -127,6 +127,7 @@ struct multipath {
> >  	int no_path_retry; /* number of retries after all paths are down
> > */
> >  	int retry_tick;    /* remaining times for retries */
> >  	int minio;
> > +	int failallpaths;
> >  	unsigned long long size;
> >  	vector paths;
> >  	vector pg;
> > 
> > 
> > 
> 
>