[dm-devel] RE: [PATCH] scsi_dh: Adding more debug options for scsi rdac handler
Chandra Seetharaman
sekharan at us.ibm.com
Wed Aug 19 22:37:54 UTC 2009
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 at 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 at 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 at lsi.com>
> > > Reviewed-by: Vijay Chauhan <vijay.chauhan at lsi.com>
> > > Reviewed-by: Bob Stankey <Robert.stankey at 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 at vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
More information about the dm-devel
mailing list