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@xxxxxxxxxx] > > 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@xxxxxxx> > > > > > > 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@xxxxxxx> > > > Reviewed-by: Vijay Chauhan <vijay.chauhan@xxxxxxx> > > > Reviewed-by: Bob Stankey <Robert.stankey@xxxxxxx> > > > > > > --- > > > --- 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@xxxxxxxxxxxxxxx > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
