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

[Christophe Varoqui]

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 ?

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" :

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

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