[dm-devel] Re: [PATCH 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Hannes Reinecke]

Hi Chandra,

Chandra Seetharaman wrote:
> What is the purpose of this feature ?
> 
> With dh_state functionality, one could associate a handler to a new
> device and then if one does "multipath", the hardware handler would be
> associated with the multipath device. What are we achieving by this ?
> 
This allows multipath to override the device tables build into
the device handler. Reason here is that multipath has a configuration
file which allows the user to override the build-in defaults.
And this includes the hardware handler, so we need to make sure that
the hardware handler is indeed attached.

I'd rather have it here instead of doing it from userland, as I try
to avoid searching through sysfs wherever possible.

> The review comments below are on the assumption that this is a needed
> feature.
> ---
> 
> Shouldn't we provide a detach also ?
> 
Hmm. Maybe. I'll check if it's possible to distinguish between
built-in devices (which shouldn't be detached) and overridden
ones (which should be).

> On Wed, 2008-05-14 at 16:43 +0200, Hannes Reinecke wrote:
>> multipath keeps a separate device table which may be
>> more current than the built-in one.
>> So we should allow to override the multipath settings
>> by calling ->attach for each device.
>>
>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>> ---
>>  drivers/md/dm-mpath.c                 |   10 ++++++
>>  drivers/scsi/device_handler/scsi_dh.c |   52 ++++++++++++++++++++++++++++++--
>>  include/scsi/scsi_dh.h                |    1 +
>>  3 files changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>> index e8f704a..b69cd87 100644
>> --- a/drivers/md/dm-mpath.c
>> +++ b/drivers/md/dm-mpath.c
>> @@ -546,6 +546,7 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
>>  {
>>  	int r;
>>  	struct pgpath *p;
>> +	struct multipath *m = (struct multipath *) ti->private;
>>
>>  	/* we need at least a path arg */
>>  	if (as->argc < 1) {
>> @@ -564,6 +565,15 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
>>  		goto bad;
>>  	}
>>
>> +	if (m->hw_handler_name) {
>> +		r = scsi_dh_attach(bdev_get_queue(p->path.dev->bdev),
>> +				   m->hw_handler_name);
>> +		if (r < 0) {
>> +			dm_put_device(ti, p->path.dev);
>> +			goto bad;
>> +		}
>> +	}
>> +
>>  	r = ps->type->add_path(ps, &p->path, as->argc, as->argv, &ti->error);
>>  	if (r) {
>>  		dm_put_device(ti, p->path.dev);
>> diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
>> index 2dbf84b..dfca3db 100644
>> --- a/drivers/scsi/device_handler/scsi_dh.c
>> +++ b/drivers/scsi/device_handler/scsi_dh.c
>> @@ -113,8 +113,12 @@ int scsi_dh_handler_attach(struct scsi_device *sdev,
>>  {
>>  	int err = -EBUSY;
>>
>> -	if (sdev->scsi_dh_data)
>> -		return err;
>> +	if (sdev->scsi_dh_data) {
>> +		if (sdev->scsi_dh_data->scsi_dh != scsi_dh)
>> +			return err;
>> +		else
>> +			return 0;
>> +	}
> 
> This need to be moved to an earlier patch.
Yes.

>>  	err = scsi_dh->attach(sdev);
>>
>> @@ -163,8 +167,11 @@ static int scsi_dh_notifier(struct notifier_block *nb,
>>  		goto out;
>>
>>  	if (action == BUS_NOTIFY_ADD_DEVICE) {
>> -		scsi_dh_handler_attach(sdev, devinfo->handler);
>> -		device_create_file(dev, &scsi_dh_state_attr);
>> +		int err;
>> +
>> +		err = scsi_dh_handler_attach(sdev, devinfo->handler);
>> +		if (!err)
>> +			err = device_create_file(dev, &scsi_dh_state_attr);
> 
> This too.
> 
Ok.

>>  	} else if (action == BUS_NOTIFY_DEL_DEVICE) {
>>  		if (sdev->scsi_dh_data == NULL)
>>  			goto out;
>> @@ -345,6 +352,43 @@ int scsi_dh_handler_exist(const char *name)
>>  }
>>  EXPORT_SYMBOL_GPL(scsi_dh_handler_exist);
>>
>> +/*
>> + * scsi_dh_handler_attach - Attach device handler
>> + * @sdev - sdev the handler should be attached to
>> + * @name - name of the handler to attach
>> + */
>> +int scsi_dh_attach(struct request_queue *q, const char *name)
>> +{
> 
> This code seems to do incorrect things (run the case with sdev == NULL
> or sdev->scsi_dh_data->scsi_dh != scsi_dh).
> 
> This function can be simplified to just call scsi_dh_handler_attach()
> after getting scsi_dh and a get_device.
> 
> We can avoid calling scsi_dh->attach directly. instead call
> scsi_dh_device_handler(), which would do all the necessary checking.
> 
Correct. Fixed.

Chandra, thank you very much for your review.
That helped a lot (and fixed some bugs, too :-).

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)