FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 05-15-2008, 02:50 AM
Chandra Seetharaman
 
Default scsi_dh: attach to hardware handler from dm-mpath

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 ?

The review comments below are on the assumption that this is a needed
feature.
---

Shouldn't we provide a detach also ?

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@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.
>
> 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.

> } 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.

> + unsigned long flags;
> + struct scsi_device *sdev;
> + struct scsi_device_handler *scsi_dh;
> + int err = 0;
> +
> + scsi_dh = get_device_handler(name);
> + if (!scsi_dh)
> + return -EINVAL;
> +
> + spin_lock_irqsave(q->queue_lock, flags);
> + sdev = q->queuedata;
> + if (sdev && sdev->scsi_dh_data) {
> + if (sdev->scsi_dh_data->scsi_dh != scsi_dh) {
> + err = -EBUSY;
> + } else {
> + spin_unlock_irqrestore(q->queue_lock, flags);
> + return 0;
> + }
> + }
> + if (!err || !get_device(&sdev->sdev_gendev))
> + err = -ENODEV;
> + spin_unlock_irqrestore(q->queue_lock, flags);
> +
> + if (scsi_dh->attach)
> + err = scsi_dh->attach(sdev);
> + put_device(&sdev->sdev_gendev);
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(scsi_dh_attach);
> +
> static struct notifier_block scsi_dh_nb = {
> .notifier_call = scsi_dh_notifier
> };
> diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
> index 5d05ac0..567bd38 100644
> --- a/include/scsi/scsi_dh.h
> +++ b/include/scsi/scsi_dh.h
> @@ -59,3 +59,4 @@ enum {
>
> extern int scsi_dh_activate(struct request_queue *);
> extern int scsi_dh_handler_exist(const char *);
> +extern int scsi_dh_attach(struct request_queue *, const char *);

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-15-2008, 09:46 AM
Hannes Reinecke
 
Default scsi_dh: attach to hardware handler from dm-mpath

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@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@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-16-2008, 06:53 PM
Chandra Seetharaman
 
Default scsi_dh: attach to hardware handler from dm-mpath

On Thu, 2008-05-15 at 11:46 +0200, Hannes Reinecke wrote:
> 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.

Hardware handler's existence is currently been verified during parsing
(by doing a request_module() followed by scsi_dh_handler_exist() in
parse_hw_handler()).

Hardware handler module is attached to the device itself (thru
try_module_get() in attach), so, the module will exist as long as the
device exists.

Hence, there is no need to attach it again from the multipath layer.

Or, I am missing something totally

<snip>


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-19-2008, 10:21 AM
Hannes Reinecke
 
Default scsi_dh: attach to hardware handler from dm-mpath

Hi Chandra,

Chandra Seetharaman wrote:

On Thu, 2008-05-15 at 11:46 +0200, Hannes Reinecke wrote:

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.


Hardware handler's existence is currently been verified during parsing
(by doing a request_module() followed by scsi_dh_handler_exist() in
parse_hw_handler()).


Yes, but this only verifies that the _handler_ exist, not that it is
attached to this sdev


Hardware handler module is attached to the device itself (thru
try_module_get() in attach), so, the module will exist as long as the
device exists.


Not quite. It's only attached automatically if the device map has
an entry for this module.
However, given this patchset we can now easily manually attach any
device to the device handler. Or at least try to do so.
It's then up to the device handler to refuse binding if none of
the necessary pages/information etc. was found.


Hence, there is no need to attach it again from the multipath layer.


Yes, you do. The user might have it's own custom configuration file,
covering new/unknown/unhandled/testing devices, which out of necessity
are _not_ hardcoded in the device table of the device handler.
So multipath has to have a way of attaching device handler to those
devices, too.

Cheers,

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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-19-2008, 06:20 PM
Chandra Seetharaman
 
Default scsi_dh: attach to hardware handler from dm-mpath

On Mon, 2008-05-19 at 12:21 +0200, Hannes Reinecke wrote:
> Hi Chandra,
>
> Chandra Seetharaman wrote:
> > On Thu, 2008-05-15 at 11:46 +0200, Hannes Reinecke wrote:
> >> 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.
> >
> > Hardware handler's existence is currently been verified during parsing
> > (by doing a request_module() followed by scsi_dh_handler_exist() in
> > parse_hw_handler()).
> >
> Yes, but this only verifies that the _handler_ exist, not that it is
> attached to this sdev

Agreed. And it works properly with the original design.

Here is my thinking on how it would work with the new feature (ability
to attach any device using dh_state) you added:
- User adds a new device
- User knows which scsi hardware handler their device can attach to,
lets say, "mydev".
- User attaches the device with scsi hardware handler (writing "mydev"
to "dh_state").
Device is now attached to "mydev"
- User updates his/her multipath.conf file with "mydev" for the
specific device.
- User invokes multipath, which checks for the existence of "mydev"
and allows the table insertion.
- When pg_init was required in the future, multipath calls "activate"
for "mydev" correctly.

Wouldn't this work ?

Note: In the absence of this patch (7/7) only.

>
> > Hardware handler module is attached to the device itself (thru
> > try_module_get() in attach), so, the module will exist as long as the
> > device exists.
> >
> Not quite. It's only attached automatically if the device map has
> an entry for this module.
> However, given this patchset we can now easily manually attach any
> device to the device handler. Or at least try to do so.
> It's then up to the device handler to refuse binding if none of
> the necessary pages/information etc. was found.

Doesn't dh_state solve this issue ?

>
> > Hence, there is no need to attach it again from the multipath layer.
> >
> Yes, you do. The user might have it's own custom configuration file,
> covering new/unknown/unhandled/testing devices, which out of necessity
> are _not_ hardcoded in the device table of the device handler.
> So multipath has to have a way of attaching device handler to those
> devices, too.

dh_state allows the user to do this, in which case why do we need to do
this in multipath layer ?

>
> Cheers,
>
> Hannes

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-20-2008, 12:41 PM
Hannes Reinecke
 
Default scsi_dh: attach to hardware handler from dm-mpath

Hi Chandra,

Chandra Seetharaman wrote:

On Mon, 2008-05-19 at 12:21 +0200, Hannes Reinecke wrote:

Hi Chandra,


[ .. ]

Yes, but this only verifies that the _handler_ exist, not that it is
attached to this sdev


Agreed. And it works properly with the original design.

Here is my thinking on how it would work with the new feature (ability
to attach any device using dh_state) you added:
- User adds a new device
- User knows which scsi hardware handler their device can attach to,
lets say, "mydev".
- User attaches the device with scsi hardware handler (writing "mydev"
to "dh_state").
Device is now attached to "mydev"
- User updates his/her multipath.conf file with "mydev" for the
specific device.
- User invokes multipath, which checks for the existence of "mydev"
and allows the table insertion.
- When pg_init was required in the future, multipath calls "activate"
for "mydev" correctly.

Wouldn't this work ?

Note: In the absence of this patch (7/7) only.


Yes, but only for the first time. After reboot the user would
have to do the reconfiguration again.


Hardware handler module is attached to the device itself (thru
try_module_get() in attach), so, the module will exist as long as the
device exists.


Not quite. It's only attached automatically if the device map has
an entry for this module.
However, given this patchset we can now easily manually attach any
device to the device handler. Or at least try to do so.
It's then up to the device handler to refuse binding if none of
the necessary pages/information etc. was found.


Doesn't dh_state solve this issue ?


In theory, yes.


Hence, there is no need to attach it again from the multipath layer.


Yes, you do. The user might have it's own custom configuration file,
covering new/unknown/unhandled/testing devices, which out of necessity
are _not_ hardcoded in the device table of the device handler.
So multipath has to have a way of attaching device handler to those
devices, too.


dh_state allows the user to do this, in which case why do we need to do
this in multipath layer ?


Because then we (read: I) don't have to modify multipath-tools.
If we don't take this patch we'll have to modify multipath-tools
to check for the dh_state attribute explicitly and activate
the hardware handler directly via sysfs.

But this also means that existing multipath-tools programs won't
be able to correctly activate the hardware handler in
_some_ cases (ie those cases, where the hardware table for the
device_handler doesn't contain the device in question).
And I can just imagine the bugs I'll be getting ...
So if we decide to take that road we should kill the hardware
handler feature from the device-mapper multipath interface
altogether to notify the user that the programs won't work
anymore. Otherwise it's just heading for trouble.

So I'd rather keep the userland interface stable here and add
some code to the kernel.

Cheers,

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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-20-2008, 02:05 PM
 
Default scsi_dh: attach to hardware handler from dm-mpath

multipath keeps a separate device table which may be
more current than the built-in one.
So we should make sure to always call ->attach whenever
a multipath map with hardware handler is instantiated.
And we should call ->detach on removal, too.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/md/dm-mpath.c | 13 ++++++
drivers/scsi/device_handler/scsi_dh.c | 74 +++++++++++++++++++++++++++++++++
include/scsi/scsi_dh.h | 2 +
3 files changed, 89 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index e8f704a..bd2bcf4 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -147,9 +147,12 @@ static struct priority_group *alloc_priority_group(void)
static void free_pgpaths(struct list_head *pgpaths, struct dm_target *ti)
{
struct pgpath *pgpath, *tmp;
+ struct multipath *m = (struct multipath *) ti->private;

list_for_each_entry_safe(pgpath, tmp, pgpaths, list) {
list_del(&pgpath->list);
+ if (m->hw_handler_name)
+ scsi_dh_detach(bdev_get_queue(pgpath->path.dev->bdev));
dm_put_device(ti, pgpath->path.dev);
free_pgpath(pgpath);
}
@@ -546,6 +549,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 +568,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 b80fae7..a9404b5 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -362,6 +362,80 @@ 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)
+{
+ unsigned long flags;
+ struct scsi_device *sdev;
+ struct scsi_device_handler *scsi_dh;
+ int err = 0;
+
+ scsi_dh = get_device_handler(name);
+ if (!scsi_dh)
+ return -EINVAL;
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ sdev = q->queuedata;
+ if (!sdev || !get_device(&sdev->sdev_gendev))
+ err = -ENODEV;
+ spin_unlock_irqrestore(q->queue_lock, flags);
+
+ if (!err) {
+ err = scsi_dh_handler_attach(sdev, scsi_dh);
+
+ put_device(&sdev->sdev_gendev);
+ }
+ return err;
+}
+EXPORT_SYMBOL_GPL(scsi_dh_attach);
+
+/*
+ * scsi_dh_handler_detach - Detach device handler
+ * @sdev - sdev the handler should be detached from
+ *
+ * This function will detach the device handler only
+ * if the sdev is not part of the internal list, ie
+ * if it has been attached manually.
+ */
+void scsi_dh_detach(struct request_queue *q)
+{
+ unsigned long flags;
+ struct scsi_device *sdev;
+ struct scsi_device_handler *scsi_dh = NULL;
+ int i, found = 0;
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ sdev = q->queuedata;
+ if (!sdev || !get_device(&sdev->sdev_gendev))
+ sdev = NULL;
+ spin_unlock_irqrestore(q->queue_lock, flags);
+
+ if (!sdev)
+ return;
+
+ if (sdev->scsi_dh_data) {
+ scsi_dh = sdev->scsi_dh_data->scsi_dh;
+ for(i = 0; scsi_dh->devlist[i].vendor; i++) {
+ if (!strncmp(sdev->vendor, scsi_dh->devlist[i].vendor,
+ strlen(scsi_dh->devlist[i].vendor)) &&
+ !strncmp(sdev->model, scsi_dh->devlist[i].model,
+ strlen(scsi_dh->devlist[i].model))) {
+ found = 1;
+ break;
+ }
+ }
+ /* sdev not on internal list, detach */
+ if (!found)
+ scsi_dh_handler_detach(sdev);
+ }
+ put_device(&sdev->sdev_gendev);
+}
+EXPORT_SYMBOL_GPL(scsi_dh_detach);
+
static struct notifier_block scsi_dh_nb = {
.notifier_call = scsi_dh_notifier
};
diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
index 5d05ac0..8032e51 100644
--- a/include/scsi/scsi_dh.h
+++ b/include/scsi/scsi_dh.h
@@ -59,3 +59,5 @@ enum {

extern int scsi_dh_activate(struct request_queue *);
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 *q);
--
1.5.2.4

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-20-2008, 06:52 PM
Chandra Seetharaman
 
Default scsi_dh: attach to hardware handler from dm-mpath

Hi Hannes,

Now I see why you want this change in dm-multipath. I think I agree with
these changes.

But, it brings another question: what does dh_state provide ? Help to
user to see which hardware handler a device is attached to ?

I thought more about the scsi_dh_detach function (in the context of my
earlier comment), adding it would require more housekeeping to associate
one-to-one mapping between attach and detach. We can leave it the same
way as the module will be detached when the device disappear eventually.

Thanks,

chandra
On Tue, 2008-05-20 at 14:41 +0200, Hannes Reinecke wrote:
> Hi Chandra,
>
> Chandra Seetharaman wrote:
> > On Mon, 2008-05-19 at 12:21 +0200, Hannes Reinecke wrote:
> >> Hi Chandra,
> >>
> [ .. ]
> >> Yes, but this only verifies that the _handler_ exist, not that it is
> >> attached to this sdev
> >
> > Agreed. And it works properly with the original design.
> >
> > Here is my thinking on how it would work with the new feature (ability
> > to attach any device using dh_state) you added:
> > - User adds a new device
> > - User knows which scsi hardware handler their device can attach to,
> > lets say, "mydev".
> > - User attaches the device with scsi hardware handler (writing "mydev"
> > to "dh_state").
> > Device is now attached to "mydev"
> > - User updates his/her multipath.conf file with "mydev" for the
> > specific device.
> > - User invokes multipath, which checks for the existence of "mydev"
> > and allows the table insertion.
> > - When pg_init was required in the future, multipath calls "activate"
> > for "mydev" correctly.
> >
> > Wouldn't this work ?
> >
> > Note: In the absence of this patch (7/7) only.
> >
> Yes, but only for the first time. After reboot the user would
> have to do the reconfiguration again.
>
> >>> Hardware handler module is attached to the device itself (thru
> >>> try_module_get() in attach), so, the module will exist as long as the
> >>> device exists.
> >>>
> >> Not quite. It's only attached automatically if the device map has
> >> an entry for this module.
> >> However, given this patchset we can now easily manually attach any
> >> device to the device handler. Or at least try to do so.
> >> It's then up to the device handler to refuse binding if none of
> >> the necessary pages/information etc. was found.
> >
> > Doesn't dh_state solve this issue ?
> >
> In theory, yes.
>
> >>> Hence, there is no need to attach it again from the multipath layer.
> >>>
> >> Yes, you do. The user might have it's own custom configuration file,
> >> covering new/unknown/unhandled/testing devices, which out of necessity
> >> are _not_ hardcoded in the device table of the device handler.
> >> So multipath has to have a way of attaching device handler to those
> >> devices, too.
> >
> > dh_state allows the user to do this, in which case why do we need to do
> > this in multipath layer ?
> >
> Because then we (read: I) don't have to modify multipath-tools.
> If we don't take this patch we'll have to modify multipath-tools
> to check for the dh_state attribute explicitly and activate
> the hardware handler directly via sysfs.
>
> But this also means that existing multipath-tools programs won't
> be able to correctly activate the hardware handler in
> _some_ cases (ie those cases, where the hardware table for the
> device_handler doesn't contain the device in question).
> And I can just imagine the bugs I'll be getting ...
> So if we decide to take that road we should kill the hardware
> handler feature from the device-mapper multipath interface
> altogether to notify the user that the programs won't work
> anymore. Otherwise it's just heading for trouble.
>
> So I'd rather keep the userland interface stable here and add
> some code to the kernel.
>
> Cheers,
>
> Hannes

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-21-2008, 06:18 AM
Hannes Reinecke
 
Default scsi_dh: attach to hardware handler from dm-mpath

Chandra Seetharaman wrote:

Hi Hannes,

Now I see why you want this change in dm-multipath. I think I agree with
these changes.


Ah. good.


But, it brings another question: what does dh_state provide ? Help to
user to see which hardware handler a device is attached to ?


And allowing to attach to a different hardware handler.
Not everyone is running multipathing, but might be interested in the
having the device handler nevertheless.


I thought more about the scsi_dh_detach function (in the context of my
earlier comment), adding it would require more housekeeping to associate
one-to-one mapping between attach and detach. We can leave it the same
way as the module will be detached when the device disappear eventually.


Why? You can detach with dh_state, too; just do an

echo detach > /sys/block/sdX/device/dh_state

and the hardware handler will detach.
So no additional attribute is required.

Cheers,

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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-22-2008, 08:14 AM
James Bottomley
 
Default scsi_dh: attach to hardware handler from dm-mpath

On Wed, 2008-05-21 at 08:18 +0200, Hannes Reinecke wrote:
> Chandra Seetharaman wrote:
> > Hi Hannes,
> >
> > Now I see why you want this change in dm-multipath. I think I agree with
> > these changes.
> >
> Ah. good.
>
> > But, it brings another question: what does dh_state provide ? Help to
> > user to see which hardware handler a device is attached to ?
> >
> And allowing to attach to a different hardware handler.
> Not everyone is running multipathing, but might be interested in the
> having the device handler nevertheless.
>
> > I thought more about the scsi_dh_detach function (in the context of my
> > earlier comment), adding it would require more housekeeping to associate
> > one-to-one mapping between attach and detach. We can leave it the same
> > way as the module will be detached when the device disappear eventually.
> >
> Why? You can detach with dh_state, too; just do an
>
> echo detach > /sys/block/sdX/device/dh_state
>
> and the hardware handler will detach.
> So no additional attribute is required.

Actually, if you're going down this route, it makes more sense to have
the device handler be a driver ... remember you were the one promising
multiple driver binding at the FS/Storage summit ... that way we can
use all the generic driver standard interfaces for manual
binding/unbinding. Plus we can place the attributes as driver attribute
groups.

James


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 04:00 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright ©2007 - 2008, www.linux-archive.org