[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[dm-devel] RE: [PATCH] scsi_dh: Adding more debug options for scsi rdac handler



On Wed, 2009-08-19 at 15:45 -0600, Moger, Babu wrote:
> Hi Chandra,
>    Thanks for your comments. I will start working on incorporating your comments however I may need some clarifications. Please see my response inline..
> - Babu
> 
> > -----Original Message-----
> > From: Chandra Seetharaman [mailto:sekharan us ibm com]
> > Sent: Tuesday, August 18, 2009 9:10 PM
> > To: Moger, Babu
> > Cc: Linux SCSI Mailing list; device-mapper development; Chauhan, Vijay;
> > Stankey, Robert; Dachepalli, Sudhir
> > Subject: Re: [PATCH] scsi_dh: Adding more debug options for scsi rdac
> > handler
> > 
> > Hi Babu,
> > 
> > Code looks good. But I have few comments...
> > 
> > Generic:
> >  - Is that all the debug stuff you want to add ?
> >  - I would split this into few patches (since they
> >      are can be independent and not related to
> >      configurable logging, IMO)
> >    1. Add the array_name and index and acquiring it.
> >    2. Moving the initialization code to attach()
> >    3. removal of SCSI_DH_OK in check_ownership() (more below).
> >    4. Logging stuff.
> >    (1) and (4) have dependency, but (2) and (3) are
> >    totally independent of all this.
> 
> I am fine with this proposal. I will try to see if I can do it without creating merging issues.
>       I think I can break it into three patches. 
> 	a) removal of SCSI_DH_OK in check_ownership() (more below).
> 	b) moving the initialization code to attach()
>       c) combining 1 and 4. 
> 	What do you think?

You can send (c) as a patchset of 2 (1 of 2 and 2 of 2); Even though it
is dependent, they can be functionally separated, which is the idea.

> 
> > 
> > Specific comments inline...
> > 
> > On Mon, 2009-08-17 at 11:46 -0600, Moger, Babu wrote:
> > > From: Babu Moger <babu moger lsi com>
> > >
> > > Hi All,
> > > This patch adds more debugging options to scsi rdac device handler.
> > This patch can considerably reduce the time taken to debug issues in
> > big configurations also very helpful in addressing the support issues.
> > > Here are the summary of changes.
> > >  - Added a bit mask "module parameter" rdac_logging with 2 bits for
> > each type of logging.
> > 
> > Why 2 bits ? isn't 1 bit sufficient ?
> 
> Right now 1 bit is sufficient. We thought it may be necessary in future
> in case we want to display only some messages(critical or info but not
> all) in particular category.  

ok.
> 
> > 
> > >  - currently defined only two types of logging(failover and sense
> > logging). Can be enhanced later if required.
> > >  - Only failover logging is enabled for now which is equivalent of
> > current logging.
> > by default.
> 
> Ok. I will correct it.
> 
> > 
> > >
> > > Signed-off-by: Babu Moger <babu moger lsi com>
> > > Reviewed-by: Vijay Chauhan <vijay chauhan lsi com>
> > > Reviewed-by: Bob Stankey <Robert stankey lsi com>
> > >
> > > ---
> > > --- linux-2.6.31-rc5/drivers/scsi/device_handler/scsi_dh_rdac.c.orig
> > 	2009-08-05 16:30:51.000000000 -0500
> > > +++ linux-2.6.31-rc5/drivers/scsi/device_handler/scsi_dh_rdac.c	2009-
> > 08-17 09:15:03.000000000 -0500
> > > @@ -135,6 +135,8 @@ struct rdac_controller {
> > >  		struct rdac_pg_legacy legacy;
> > >  		struct rdac_pg_expanded expanded;
> > >  	} mode_select;
> > > +	u8			index;
> > > +	u8			array_name[31];
> > 
> > Can use a #define ?
> 
> If I understood your question correctly,  #define may not work well.
>  Reason is, if we have multiple arrays(which is very common) then we
>  cannot differentiate the two arrays.. So we have to have index and
>  array name separately for each array/controller..

Sorry for not being clear. I meant to ask if we can use a #define
instead of literal 31.

> 
> > 
> > >  };
> > >  struct c8_inquiry {
> > >  	u8	peripheral_info;
> > > @@ -198,6 +200,31 @@ static const char *lun_state[] =
> > >  static LIST_HEAD(ctlr_list);
> > >  static DEFINE_SPINLOCK(list_lock);
> > >
> > > +/*
> > > + * module parameter to enable rdac debug logging.
> > > + * 2 bits for each type of logging, only two types defined for now
> > > + * Can be enhanced if required at later point
> > > + */
> > > +static int rdac_logging = 1;
> > > +module_param(rdac_logging, int, S_IRUGO|S_IWUSR);
> > > +MODULE_PARM_DESC(rdac_logging, "A bit mask of rdac logging levels, "
> > > +		"Default is 1 - failover logging enabled, "
> > > +		"set it to 0xF to enable all the logs");
> > > +
> > > +#define RDAC_LOG_FAILOVER	0
> > > +#define RDAC_LOG_SENSE		2
> > > +
> > > +#define RDAC_LOG_BITS		2
> > > +
> > > +#define RDAC_LOG_LEVEL(SHIFT)  \
> > > +        ((rdac_logging >> (SHIFT)) & ((1 << (RDAC_LOG_BITS)) - 1))
> > > +
> > > +#define RDAC_DEBUG(SHIFT, sdev, f, arg...) \
> > 
> > instead of RDAC_DEBUG, we can say RDAC_LOG ?!
> 
> Sure.. I will do that..
> 
> > 
> > > +do { \
> > > +	if (unlikely(RDAC_LOG_LEVEL(SHIFT))) \
> > > +		sdev_printk(KERN_INFO, sdev, RDAC_NAME ": " f "\n", ##
> > arg); \
> > > +} while (0);
> > > +
> > >  static inline struct rdac_dh_data *get_rdac_data(struct scsi_device
> > *sdev)
> > >  {
> > >  	struct scsi_dh_data *scsi_dh_data = sdev->scsi_dh_data;
> > > @@ -324,6 +351,13 @@ static struct rdac_controller *get_contr
> > >  	/* initialize fields of controller */
> > >  	memcpy(ctlr->subsys_id, subsys_id, SUBSYS_ID_LEN);
> > >  	memcpy(ctlr->slot_id, slot_id, SLOT_ID_LEN);
> > > +
> > > +	/* update the controller index */
> > > +	if (slot_id[1] == 0x31)
> > > +		ctlr->index = 0;
> > > +	else
> > > +		ctlr->index = 1;
> > > +
> > >  	kref_init(&ctlr->kref);
> > >  	ctlr->use_ms10 = -1;
> > >  	list_add(&ctlr->node, &ctlr_list);
> > > @@ -381,6 +415,26 @@ static int get_lun(struct scsi_device *s
> > >  	return err;
> > >  }
> > >
> > > +static int get_array_name(struct scsi_device *sdev, struct
> > rdac_dh_data *h)
> > > +{
> > > +	int err, i;
> > > +	struct c8_inquiry *inqp;
> > > +
> > > +	err = submit_inquiry(sdev, 0xC8, sizeof(struct c8_inquiry), h);
> > > +	if (err == SCSI_DH_OK) {
> > > +		inqp = &h->inq.c8;
> > > +		if (inqp->page_code != 0xc8)
> > > +			return SCSI_DH_NOSYS;
> > > +
> > > +		for(i=0; i<30; ++i) {
> > > +			h->ctlr->array_name[i] = inqp-
> > >array_user_label[(2*i)+1];
> > > +		}
> > 
> > do not have to have { } for a single line loop.
> 
> Sure.. I will remove that..
> 
> > 
> > > +		h->ctlr->array_name[30] = '\0';
> > > +	}
> > > +
> > > +	return err;
> > > +}
> > > +
> > >  static int check_ownership(struct scsi_device *sdev, struct
> > rdac_dh_data *h)
> > >  {
> > >  	int err;
> > > @@ -450,13 +504,12 @@ static int mode_select_handle_sense(stru
> > >  {
> > >  	struct scsi_sense_hdr sense_hdr;
> > >  	int err = SCSI_DH_IO, ret;
> > > +	struct rdac_dh_data *h = get_rdac_data(sdev);
> > >
> > >  	ret = scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
> > &sense_hdr);
> > >  	if (!ret)
> > >  		goto done;
> > >
> > > -	err = SCSI_DH_OK;
> > > -
> > 
> > This is a change in behavior.
> > 
> > Why is this needed ?
> 
> Setting the flag by default to SCSI_DH_OK might wrongly report the
> command as success even though the command had failed. The switch 
> statement here only lists certain check conditions. If the target 
> returns the check conditions other than the listed here then this 
> function will return SCSI_DH_OK which is not correct.

A Bug fix. Separate patch would be the right thing to do.
> 
> > 
> > >  	switch (sense_hdr.sense_key) {
> > >  	case NO_SENSE:
> > >  	case ABORTED_COMMAND:
> > > @@ -478,12 +531,15 @@ static int mode_select_handle_sense(stru
> > >  			err = SCSI_DH_RETRY;
> > >  		break;
> > >  	default:
> > > -		sdev_printk(KERN_INFO, sdev,
> > > -			    "MODE_SELECT failed with sense
> > %02x/%02x/%02x.\n",
> > > -			    sense_hdr.sense_key, sense_hdr.asc,
> > sense_hdr.ascq);
> > > +		break;
> > >  	}
> > >
> > >  done:
> > > +	RDAC_DEBUG(RDAC_LOG_FAILOVER, sdev, "array %s, ctlr %d, "
> > > +		"MODE_SELECT returned with sense %02x/%02x/%02x",
> > > +		(char *) h->ctlr->array_name, h->ctlr->index,
> > > +		sense_hdr.sense_key, sense_hdr.asc, sense_hdr.ascq);
> > > +
> > 
> > May need to be moved above "done:", as the control comes to done when
> > scsi_normalize_sense() fails.
> 
> Sometimes we have seen target returning invalid sense. We need to catch
> that as well. That is the reason we moved this after done.

We shouldn't be accessing sense_hdr data structure when
scsi_normalize_sense() fails.

> 
> > 
> > >
> > >  	return err;
> > >  }
> > >
> > > @@ -499,7 +555,9 @@ retry:
> > >  	if (!rq)
> > >  		goto done;
> > >
> > > -	sdev_printk(KERN_INFO, sdev, "%s MODE_SELECT command.\n",
> > > +	RDAC_DEBUG(RDAC_LOG_FAILOVER, sdev, "array %s, ctlr %d, "
> > > +		"%s MODE_SELECT command",
> > > +		(char *) h->ctlr->array_name, h->ctlr->index,
> > >  		(retry_cnt == RDAC_RETRY_COUNT) ? "queueing" : "retrying");
> > >
> > >  	err = blk_execute_rq(q, NULL, rq, 1);
> > > @@ -509,8 +567,12 @@ retry:
> > >  		if (err == SCSI_DH_RETRY && retry_cnt--)
> > >  			goto retry;
> > >  	}
> > > -	if (err == SCSI_DH_OK)
> > > +	if (err == SCSI_DH_OK) {
> > >  		h->state = RDAC_STATE_ACTIVE;
> > > +		RDAC_DEBUG(RDAC_LOG_FAILOVER, sdev, "array %s, ctlr %d, "
> > > +				"MODE_SELECT completed",
> > > +				(char *) h->ctlr->array_name, h->ctlr->index);
> > > +	}
> > Don;t you think we need to have failure also reported ?
> 
> Failure will be anyway reported by the function mode_select_handle_sense. Did not want to print that again.
> 
ok

> > 
> > >
> > >  done:
> > >  	return err;
> > > @@ -525,12 +587,6 @@ static int rdac_activate(struct scsi_dev
> > >  	if (err != SCSI_DH_OK)
> > >  		goto done;
> > >
> > > -	if (!h->ctlr) {
> > > -		err = initialize_controller(sdev, h);
> > > -		if (err != SCSI_DH_OK)
> > > -			goto done;
> > > -	}
> > > -
> > 
> > You can also move the set_mode_select block below.
> 
> Sure.. I will do that..
> 
> > 
> > >  	if (h->ctlr->use_ms10 == -1) {
> > >  		err = set_mode_select(sdev, h);
> > >  		if (err != SCSI_DH_OK)
> > > @@ -559,6 +615,12 @@ static int rdac_check_sense(struct scsi_
> > >  				struct scsi_sense_hdr *sense_hdr)
> > >  {
> > >  	struct rdac_dh_data *h = get_rdac_data(sdev);
> > > +
> > > +	RDAC_DEBUG(RDAC_LOG_SENSE, sdev, "array %s, ctlr %d, "
> > > +			"command returned with sense %02x/%02x/%02x",
> > > +			(char *) h->ctlr->array_name, h->ctlr->index,
> > > +			sense_hdr->sense_key, sense_hdr->asc, sense_hdr-
> > >ascq);
> > > +
> > 
> > It is not a "command", right ? Code comes here for any I/O failure.
> > cut-n-paste problem :)
> 
> I was in a confusion about what will be the right word here. Ok.. I will use "I/O"

sounds good.
> 
> > 
> > 
> > >  	switch (sense_hdr->sense_key) {
> > >  	case NOT_READY:
> > >  		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x01)
> > > @@ -678,6 +740,16 @@ static int rdac_bus_attach(struct scsi_d
> > >  	if (err != SCSI_DH_OK)
> > >  		goto failed;
> > >
> > > +	if (!h->ctlr) {
> > 
> > I do not think we need this check here.
> 
> You are right.. We don’t need this check here..
> 
> > > +		err = initialize_controller(sdev, h);
> > > +		if (err != SCSI_DH_OK)
> > > +			goto failed;
> > > +
> > > +		err = get_array_name(sdev, h);
> > > +		if (err != SCSI_DH_OK)
> > > +			goto failed;
> > > +	}
> > > +
> > >  	if (!try_module_get(THIS_MODULE))
> > >  		goto failed;
> > >
> > >
> > >
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> > in
> > > the body of a message to majordomo vger kernel org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]