Mike, Yes, I have retested using this patch. Everything looks good. > -----Original Message----- > From: Mike Snitzer [mailto:snitzer@xxxxxxxxxx] > Sent: Thursday, May 10, 2012 9:11 AM > To: Christoph Hellwig > Cc: linux-scsi@xxxxxxxxxxxxxxx; agk@xxxxxxxxxx; hare@xxxxxxx; Moger, > Babu; sekharan@xxxxxxxxxx > Subject: Re: [PATCH v3 2/5] scsi_dh: add scsi_dh_attached_handler_name > > On Thu, May 10 2012 at 2:55am -0400, > Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > > > On Wed, May 09, 2012 at 09:12:46PM -0400, Mike Snitzer wrote: > > > Originally posted to dm-devel but Chandra reminded me to post to > > > linux-scsi for James to pick it up. > > > > > > -------8<------- > > > > > > Introduce scsi_dh_attached_handler_name() to retrieve the name of > the > > > scsi_dh that is attached to the scsi_device associated with the > provided > > > request queue. Returns NULL if a scsi_dh is not attached. > > > > > > Also, fix scsi_dh_{attach,detach} function header comments to > document > > > @q rather than @sdev. > > > > Wha'ts the use case for this? > > See this patch: > http://www.redhat.com/archives/dm-devel/2012-May/msg00046.html > > (I attributed this change to Hannes because it was based on his earlier > patch... I should probably change that given that in the end I > re-wrote/merged his patch with my earlier patch...) > > Anyway, DM mpath doesn't want to know about the "scsi_dh" structure, > but > we need the name of the attached handler (to adjust the multipath > device's 'hw_handler_name' and to get an additional ref on the attached > scsi_dh via dm-mapth.c:parse_path's scsi_dh_attach). > > BTW, only reason I split the scsi_dh change from the above dm-mpath > patch is because I'm patching SCSI and DM and need to send the changes > through different trees. > > > The name is stale as soon as the function returns. > > Yeah, I have since wondered about that myself. In practice the > attached > handler should remain attached so the name really won't be stale. > > But in theory there could be a race with scsi_dh_detach. So I think > the > following should address your concern? > > If so I'll refresh and repost the appropriate patches. > > Thanks. > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > index 1039e7f..6f11956 100644 > --- a/drivers/md/dm-mpath.c > +++ b/drivers/md/dm-mpath.c > @@ -592,10 +592,11 @@ static struct pgpath *parse_path(struct > dm_arg_set *as, struct path_selector *ps > q = bdev_get_queue(p->path.dev->bdev); > > if (m->use_default_hw_handler) { > - const char *attached_handler_name = > scsi_dh_attached_handler_name(q); > + const char *attached_handler_name = > + scsi_dh_attached_handler_name(q, GFP_KERNEL); > if (attached_handler_name) { > kfree(m->hw_handler_name); > - m->hw_handler_name = kstrdup(attached_handler_name, > GFP_KERNEL); > + m->hw_handler_name = attached_handler_name; > } > } > > diff --git a/drivers/scsi/device_handler/scsi_dh.c > b/drivers/scsi/device_handler/scsi_dh.c > index 83071e4..33e422e 100644 > --- a/drivers/scsi/device_handler/scsi_dh.c > +++ b/drivers/scsi/device_handler/scsi_dh.c > @@ -533,10 +533,12 @@ EXPORT_SYMBOL_GPL(scsi_dh_detach); > * scsi_dh_attached_handler_name - Get attached device handler's name > * @q - Request queue that is associated with the scsi_device > * that may have a device handler attached > + * @gfp - the GFP mask used in the kmalloc() call when allocating > memory > * > * Returns name of attached handler, NULL if no handler is attached. > + * Caller must take care to free the returned string. > */ > -const char *scsi_dh_attached_handler_name(struct request_queue *q) > +const char *scsi_dh_attached_handler_name(struct request_queue *q, > gfp_t gfp) > { > unsigned long flags; > struct scsi_device *sdev; > @@ -552,7 +554,7 @@ const char *scsi_dh_attached_handler_name(struct > request_queue *q) > return NULL; > > if (sdev->scsi_dh_data) > - handler_name = sdev->scsi_dh_data->scsi_dh->name; > + handler_name = kstrdup(sdev->scsi_dh_data->scsi_dh->name, > gfp); > > put_device(&sdev->sdev_gendev); > return handler_name; > diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h > index 94f502b..620c723 100644 > --- a/include/scsi/scsi_dh.h > +++ b/include/scsi/scsi_dh.h > @@ -60,7 +60,7 @@ extern int scsi_dh_activate(struct request_queue *, > activate_complete, void *); > extern int scsi_dh_handler_exist(const char *); > extern int scsi_dh_attach(struct request_queue *, const char *); > extern void scsi_dh_detach(struct request_queue *); > -extern const char *scsi_dh_attached_handler_name(struct request_queue > *q); > +extern const char *scsi_dh_attached_handler_name(struct request_queue > *, gfp_t); > extern int scsi_dh_set_params(struct request_queue *, const char *); > #else > static inline int scsi_dh_activate(struct request_queue *req, > @@ -81,7 +81,8 @@ static inline void scsi_dh_detach(struct > request_queue *q) > { > return; > } > -static inline const char *scsi_dh_attached_handler_name(struct > request_queue *q) > +static inline const char *scsi_dh_attached_handler_name(struct > request_queue *q, > + gfp_t gfp) > { > return NULL; > } > > -- 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
