* [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh
@ 2017-02-16 15:12 Hannes Reinecke
2017-02-16 16:09 ` Bart Van Assche
2017-02-16 17:05 ` Keith Busch
0 siblings, 2 replies; 9+ messages in thread
From: Hannes Reinecke @ 2017-02-16 15:12 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Christoph Hellwig, Keith Busch, linux-scsi, Bart van Assche,
Hannes Reinecke, Hannes Reinecke
The device handler needs to check if a given queue belongs to
a scsi device; only then does it make sense to attach a device
handler.
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
drivers/scsi/scsi_dh.c | 22 ++++------------------
drivers/scsi/scsi_lib.c | 26 ++++++++++++++++++++++++++
include/scsi/scsi_device.h | 1 +
3 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/drivers/scsi/scsi_dh.c b/drivers/scsi/scsi_dh.c
index b8d3b97..84addee 100644
--- a/drivers/scsi/scsi_dh.c
+++ b/drivers/scsi/scsi_dh.c
@@ -219,20 +219,6 @@ int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh)
}
EXPORT_SYMBOL_GPL(scsi_unregister_device_handler);
-static struct scsi_device *get_sdev_from_queue(struct request_queue *q)
-{
- struct scsi_device *sdev;
- unsigned long flags;
-
- 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);
-
- return sdev;
-}
-
/*
* scsi_dh_activate - activate the path associated with the scsi_device
* corresponding to the given request queue.
@@ -251,7 +237,7 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
struct scsi_device *sdev;
int err = SCSI_DH_NOSYS;
- sdev = get_sdev_from_queue(q);
+ sdev = scsi_device_from_queue(q);
if (!sdev) {
if (fn)
fn(data, err);
@@ -298,7 +284,7 @@ int scsi_dh_set_params(struct request_queue *q, const char *params)
struct scsi_device *sdev;
int err = -SCSI_DH_NOSYS;
- sdev = get_sdev_from_queue(q);
+ sdev = scsi_device_from_queue(q);
if (!sdev)
return err;
@@ -321,7 +307,7 @@ int scsi_dh_attach(struct request_queue *q, const char *name)
struct scsi_device_handler *scsi_dh;
int err = 0;
- sdev = get_sdev_from_queue(q);
+ sdev = scsi_device_from_queue(q);
if (!sdev)
return -ENODEV;
@@ -359,7 +345,7 @@ const char *scsi_dh_attached_handler_name(struct request_queue *q, gfp_t gfp)
struct scsi_device *sdev;
const char *handler_name = NULL;
- sdev = get_sdev_from_queue(q);
+ sdev = scsi_device_from_queue(q);
if (!sdev)
return NULL;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9d6aed5..1a81ef9 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2145,6 +2145,32 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost)
blk_mq_free_tag_set(&shost->tag_set);
}
+/**
+ * scsi_device_from_queue - return sdev associated with a request_queue
+ * @q: The request queue to return the sdev from
+ *
+ * Return the sdev associated with a request queue or NULL if the
+ * request_queue does not reference a SCSI device.
+ */
+struct scsi_device *scsi_device_from_queue(struct request_queue *q)
+{
+ struct scsi_device *sdev = NULL;
+ unsigned long flags;
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ if (q->mq_ops) {
+ if (q->mq_ops == &scsi_mq_ops)
+ sdev = q->queuedata;
+ } else if (q->request_fn == scsi_request_fn)
+ sdev = q->queuedata;
+ if (!sdev || !get_device(&sdev->sdev_gendev))
+ sdev = NULL;
+ spin_unlock_irqrestore(q->queue_lock, flags);
+
+ return sdev;
+}
+EXPORT_SYMBOL_GPL(scsi_device_from_queue);
+
/*
* Function: scsi_block_requests()
*
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 8990e58..be41c76 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -315,6 +315,7 @@ extern int scsi_add_device(struct Scsi_Host *host, uint channel,
extern int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh);
void scsi_attach_vpd(struct scsi_device *sdev);
+extern struct scsi_device *scsi_device_from_queue(struct request_queue *q);
extern int scsi_device_get(struct scsi_device *);
extern void scsi_device_put(struct scsi_device *);
extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
--
1.8.5.6
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh
2017-02-16 15:12 [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh Hannes Reinecke
@ 2017-02-16 16:09 ` Bart Van Assche
2017-02-17 7:19 ` Hannes Reinecke
2017-02-16 17:05 ` Keith Busch
1 sibling, 1 reply; 9+ messages in thread
From: Bart Van Assche @ 2017-02-16 16:09 UTC (permalink / raw)
To: hare, martin.petersen; +Cc: hch, linux-scsi, keith.busch, hare
On Thu, 2017-02-16 at 16:12 +0100, Hannes Reinecke wrote:
> +struct scsi_device *scsi_device_from_queue(struct request_queue *q)
> +{
> + struct scsi_device *sdev = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(q->queue_lock, flags);
> + if (q->mq_ops) {
> + if (q->mq_ops == &scsi_mq_ops)
> + sdev = q->queuedata;
> + } else if (q->request_fn == scsi_request_fn)
> + sdev = q->queuedata;
> + if (!sdev || !get_device(&sdev->sdev_gendev))
> + sdev = NULL;
> + spin_unlock_irqrestore(q->queue_lock, flags);
> +
> + return sdev;
> +}
Hello Hannes,
Do we need to take the queue lock? Neither q->mq_ops nor q->request_fn are
modified after a block device has been created. q->queuedata is not modified
by any SCSI driver after it has been set. And since the caller of
scsi_device_from_queue() has to guarantee that the queue does not disappear
while this function is in progress, the queue lock does not have to be held
around the get_device() call either. Otherwise this patch looks fine to me.
Bart.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh
2017-02-16 15:12 [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh Hannes Reinecke
2017-02-16 16:09 ` Bart Van Assche
@ 2017-02-16 17:05 ` Keith Busch
2017-02-16 17:13 ` Keith Busch
2017-02-17 8:06 ` Hannes Reinecke
1 sibling, 2 replies; 9+ messages in thread
From: Keith Busch @ 2017-02-16 17:05 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Martin K. Petersen, Christoph Hellwig, linux-scsi,
Bart van Assche, Hannes Reinecke
On Thu, Feb 16, 2017 at 04:12:23PM +0100, Hannes Reinecke wrote:
> The device handler needs to check if a given queue belongs to
> a scsi device; only then does it make sense to attach a device
> handler.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
The thing I don't like is that this still has dm-mpath directly calling
into scsi. I don't think dm-mpath has any business calling protocol
specific APIs, and doesn't help other protocols with similar needs.
Can we solve this with an indirection layer instead?
(untested; just checking if this direction is preferable)
---
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 3570bcb..28310a2 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -847,7 +847,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags)) {
retain:
- attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL);
+ attached_handler_name = blk_dh_attached_handler_name(q, GFP_KERNEL);
if (attached_handler_name) {
/*
* Clear any hw_handler_params associated with a
@@ -870,7 +870,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
}
if (m->hw_handler_name) {
- r = scsi_dh_attach(q, m->hw_handler_name);
+ r = blk_dh_attach(q, m->hw_handler_name);
if (r == -EBUSY) {
char b[BDEVNAME_SIZE];
@@ -885,7 +885,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
}
if (m->hw_handler_params) {
- r = scsi_dh_set_params(q, m->hw_handler_params);
+ r = blk_dh_set_params(q, m->hw_handler_params);
if (r < 0) {
ti->error = "unable to set hardware "
"handler parameters";
@@ -1526,7 +1526,7 @@ static void activate_path(struct work_struct *work)
struct request_queue *q = bdev_get_queue(pgpath->path.dev->bdev);
if (pgpath->is_active && !blk_queue_dying(q))
- scsi_dh_activate(q, pg_init_done, pgpath);
+ blk_dh_activate(q, pg_init_done, pgpath);
else
pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
}
diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 06d2ed4..49a061a 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -107,7 +107,7 @@ static int dpc_probe(struct pcie_device *dev)
int status;
u16 ctl, cap;
- dpc = kzalloc(&dev->device, sizeof(*dpc), GFP_KERNEL);
+ dpc = kzalloc(sizeof(*dpc), GFP_KERNEL);
if (!dpc)
return -ENOMEM;
@@ -116,7 +116,7 @@ static int dpc_probe(struct pcie_device *dev)
INIT_WORK(&dpc->work, interrupt_event_handler);
set_service_data(dev, dpc);
- status = request_irq(&dev->device, dev->irq, dpc_irq, IRQF_SHARED,
+ status = request_irq(dev->irq, dpc_irq, IRQF_SHARED,
"pcie-dpc", dpc);
if (status) {
dev_warn(&dev->device, "request IRQ%d failed: %d\n", dev->irq,
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e9e1e14..e23c7d5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2028,6 +2028,13 @@ static u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
return bounce_limit;
}
+static struct dh_ops scsi_dh_ops = {
+ .dh_activate = scsi_dh_activate,
+ .dh_attach = scsi_dh_attach,
+ .dh_attached_handler_name = scsi_dh_attached_handler_name,
+ .dh_set_params = scsi_dh_set_params,
+};
+
static void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
{
struct device *dev = shost->dma_dev;
@@ -2062,6 +2069,8 @@ static void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
* blk_queue_update_dma_alignment() later.
*/
blk_queue_dma_alignment(q, 0x03);
+
+ q->dh_ops = &scsi_dh_ops;
}
struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1ca8e8f..5899768 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -378,6 +378,14 @@ static inline int blkdev_reset_zones_ioctl(struct block_device *bdev,
#endif /* CONFIG_BLK_DEV_ZONED */
+typedef void (*activate_complete)(void *, int);
+struct dh_ops {
+ int (*dh_activate)(struct request_queue *, activate_complete, void *);
+ int (*dh_attach)(struct request_queue *, const char *);
+ const char *(*dh_attached_handler_name)(struct request_queue *, gfp_t);
+ int (*dh_set_params)(struct request_queue *, const char *);
+};
+
struct request_queue {
/*
* Together with queue_head for cacheline sharing
@@ -408,6 +416,7 @@ struct request_queue {
lld_busy_fn *lld_busy_fn;
struct blk_mq_ops *mq_ops;
+ struct dh_ops *dh_ops;
unsigned int *mq_map;
@@ -572,6 +581,35 @@ struct request_queue {
bool mq_sysfs_init_done;
};
+static inline int blk_dh_activate(struct request_queue *q, activate_complete fn, void *data)
+{
+ if (q->dh_ops && q->dh_ops->dh_activate)
+ return q->dh_ops->dh_activate(q, fn, data);
+ fn(data, 0);
+ return 0;
+}
+
+static inline int blk_dh_attach(struct request_queue *q, const char *name)
+{
+ if (q->dh_ops && q->dh_ops->dh_attach)
+ return q->dh_ops->dh_attach(q, name);
+ return 0;
+}
+
+static inline const char *blk_dh_attached_handler_name(struct request_queue *q, gfp_t gfp)
+{
+ if (q->dh_ops && q->dh_ops->dh_attached_handler_name)
+ return q->dh_ops->dh_attached_handler_name(q, gfp);
+ return NULL;
+}
+
+static inline int blk_dh_set_params(struct request_queue *q, const char *params)
+{
+ if (q->dh_ops && q->dh_ops->dh_set_params)
+ return q->dh_ops->dh_set_params(q, params);
+ return 0;
+}
+
#define QUEUE_FLAG_QUEUED 1 /* uses generic tag queueing */
#define QUEUE_FLAG_STOPPED 2 /* queue is stopped */
#define QUEUE_FLAG_SYNCFULL 3 /* read queue has been filled */
diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
index c7bba2b..9169a66 100644
--- a/include/scsi/scsi_dh.h
+++ b/include/scsi/scsi_dh.h
@@ -57,7 +57,6 @@ enum {
SCSI_DH_DRIVER_MAX,
};
-typedef void (*activate_complete)(void *, int);
struct scsi_device_handler {
/* Used by the infrastructure */
struct list_head list; /* list of scsi_device_handlers */
--
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh
2017-02-16 17:05 ` Keith Busch
@ 2017-02-16 17:13 ` Keith Busch
2017-02-17 8:06 ` Hannes Reinecke
1 sibling, 0 replies; 9+ messages in thread
From: Keith Busch @ 2017-02-16 17:13 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Martin K. Petersen, Christoph Hellwig, linux-scsi,
Bart van Assche, Hannes Reinecke
On Thu, Feb 16, 2017 at 12:05:19PM -0500, Keith Busch wrote:
> On Thu, Feb 16, 2017 at 04:12:23PM +0100, Hannes Reinecke wrote:
> > The device handler needs to check if a given queue belongs to
> > a scsi device; only then does it make sense to attach a device
> > handler.
> >
> > Signed-off-by: Hannes Reinecke <hare@suse.com>
>
> The thing I don't like is that this still has dm-mpath directly calling
> into scsi. I don't think dm-mpath has any business calling protocol
> specific APIs, and doesn't help other protocols with similar needs.
>
> Can we solve this with an indirection layer instead?
>
> (untested; just checking if this direction is preferable)
Eh osrry, I accidently included unrelated stuff in the previous. Here's
what I meant to send:
---
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 3570bcb..28310a2 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -847,7 +847,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags)) {
retain:
- attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL);
+ attached_handler_name = blk_dh_attached_handler_name(q, GFP_KERNEL);
if (attached_handler_name) {
/*
* Clear any hw_handler_params associated with a
@@ -870,7 +870,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
}
if (m->hw_handler_name) {
- r = scsi_dh_attach(q, m->hw_handler_name);
+ r = blk_dh_attach(q, m->hw_handler_name);
if (r == -EBUSY) {
char b[BDEVNAME_SIZE];
@@ -885,7 +885,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
}
if (m->hw_handler_params) {
- r = scsi_dh_set_params(q, m->hw_handler_params);
+ r = blk_dh_set_params(q, m->hw_handler_params);
if (r < 0) {
ti->error = "unable to set hardware "
"handler parameters";
@@ -1526,7 +1526,7 @@ static void activate_path(struct work_struct *work)
struct request_queue *q = bdev_get_queue(pgpath->path.dev->bdev);
if (pgpath->is_active && !blk_queue_dying(q))
- scsi_dh_activate(q, pg_init_done, pgpath);
+ blk_dh_activate(q, pg_init_done, pgpath);
else
pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
}
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e9e1e14..e23c7d5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2028,6 +2028,13 @@ static u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
return bounce_limit;
}
+static struct dh_ops scsi_dh_ops = {
+ .dh_activate = scsi_dh_activate,
+ .dh_attach = scsi_dh_attach,
+ .dh_attached_handler_name = scsi_dh_attached_handler_name,
+ .dh_set_params = scsi_dh_set_params,
+};
+
static void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
{
struct device *dev = shost->dma_dev;
@@ -2062,6 +2069,8 @@ static void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
* blk_queue_update_dma_alignment() later.
*/
blk_queue_dma_alignment(q, 0x03);
+
+ q->dh_ops = &scsi_dh_ops;
}
struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1ca8e8f..5899768 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -378,6 +378,14 @@ static inline int blkdev_reset_zones_ioctl(struct block_device *bdev,
#endif /* CONFIG_BLK_DEV_ZONED */
+typedef void (*activate_complete)(void *, int);
+struct dh_ops {
+ int (*dh_activate)(struct request_queue *, activate_complete, void *);
+ int (*dh_attach)(struct request_queue *, const char *);
+ const char *(*dh_attached_handler_name)(struct request_queue *, gfp_t);
+ int (*dh_set_params)(struct request_queue *, const char *);
+};
+
struct request_queue {
/*
* Together with queue_head for cacheline sharing
@@ -408,6 +416,7 @@ struct request_queue {
lld_busy_fn *lld_busy_fn;
struct blk_mq_ops *mq_ops;
+ struct dh_ops *dh_ops;
unsigned int *mq_map;
@@ -572,6 +581,35 @@ struct request_queue {
bool mq_sysfs_init_done;
};
+static inline int blk_dh_activate(struct request_queue *q, activate_complete fn, void *data)
+{
+ if (q->dh_ops && q->dh_ops->dh_activate)
+ return q->dh_ops->dh_activate(q, fn, data);
+ fn(data, 0);
+ return 0;
+}
+
+static inline int blk_dh_attach(struct request_queue *q, const char *name)
+{
+ if (q->dh_ops && q->dh_ops->dh_attach)
+ return q->dh_ops->dh_attach(q, name);
+ return 0;
+}
+
+static inline const char *blk_dh_attached_handler_name(struct request_queue *q, gfp_t gfp)
+{
+ if (q->dh_ops && q->dh_ops->dh_attached_handler_name)
+ return q->dh_ops->dh_attached_handler_name(q, gfp);
+ return NULL;
+}
+
+static inline int blk_dh_set_params(struct request_queue *q, const char *params)
+{
+ if (q->dh_ops && q->dh_ops->dh_set_params)
+ return q->dh_ops->dh_set_params(q, params);
+ return 0;
+}
+
#define QUEUE_FLAG_QUEUED 1 /* uses generic tag queueing */
#define QUEUE_FLAG_STOPPED 2 /* queue is stopped */
#define QUEUE_FLAG_SYNCFULL 3 /* read queue has been filled */
diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
index c7bba2b..9169a66 100644
--- a/include/scsi/scsi_dh.h
+++ b/include/scsi/scsi_dh.h
@@ -57,7 +57,6 @@ enum {
SCSI_DH_DRIVER_MAX,
};
-typedef void (*activate_complete)(void *, int);
struct scsi_device_handler {
/* Used by the infrastructure */
struct list_head list; /* list of scsi_device_handlers */
--
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh
2017-02-16 16:09 ` Bart Van Assche
@ 2017-02-17 7:19 ` Hannes Reinecke
0 siblings, 0 replies; 9+ messages in thread
From: Hannes Reinecke @ 2017-02-17 7:19 UTC (permalink / raw)
To: Bart Van Assche, martin.petersen; +Cc: hch, linux-scsi, keith.busch, hare
On 02/16/2017 05:09 PM, Bart Van Assche wrote:
> On Thu, 2017-02-16 at 16:12 +0100, Hannes Reinecke wrote:
>> +struct scsi_device *scsi_device_from_queue(struct request_queue *q)
>> +{
>> + struct scsi_device *sdev = NULL;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(q->queue_lock, flags);
>> + if (q->mq_ops) {
>> + if (q->mq_ops == &scsi_mq_ops)
>> + sdev = q->queuedata;
>> + } else if (q->request_fn == scsi_request_fn)
>> + sdev = q->queuedata;
>> + if (!sdev || !get_device(&sdev->sdev_gendev))
>> + sdev = NULL;
>> + spin_unlock_irqrestore(q->queue_lock, flags);
>> +
>> + return sdev;
>> +}
>
> Hello Hannes,
>
> Do we need to take the queue lock? Neither q->mq_ops nor q->request_fn are
> modified after a block device has been created. q->queuedata is not modified
> by any SCSI driver after it has been set. And since the caller of
> scsi_device_from_queue() has to guarantee that the queue does not disappear
> while this function is in progress, the queue lock does not have to be held
> around the get_device() call either. Otherwise this patch looks fine to me.
>
Well, we did for the original code path, so I assumed that it'd be
prudent to do so.
And it's hardly time-critical anyway.
But yes, I guess we could do without the lock.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh
2017-02-16 17:05 ` Keith Busch
2017-02-16 17:13 ` Keith Busch
@ 2017-02-17 8:06 ` Hannes Reinecke
2017-02-17 8:27 ` Christoph Hellwig
1 sibling, 1 reply; 9+ messages in thread
From: Hannes Reinecke @ 2017-02-17 8:06 UTC (permalink / raw)
To: Keith Busch
Cc: Martin K. Petersen, Christoph Hellwig, linux-scsi,
Bart van Assche, Hannes Reinecke
On 02/16/2017 06:05 PM, Keith Busch wrote:
> On Thu, Feb 16, 2017 at 04:12:23PM +0100, Hannes Reinecke wrote:
>> The device handler needs to check if a given queue belongs to
>> a scsi device; only then does it make sense to attach a device
>> handler.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>
> The thing I don't like is that this still has dm-mpath directly calling
> into scsi. I don't think dm-mpath has any business calling protocol
> specific APIs, and doesn't help other protocols with similar needs.
>
> Can we solve this with an indirection layer instead?
>
> (untested; just checking if this direction is preferable)
We could, but why?
ATM we're only having SCSI devices able to use device handler; adding
another layer of indirection doesn't solve anything here.
Moving the infrastructure one level up will only make sense if we're
getting non-SCSI device handler (ANA?), but until then I'd think it's
just overengineering.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh
2017-02-17 8:06 ` Hannes Reinecke
@ 2017-02-17 8:27 ` Christoph Hellwig
2017-02-19 5:59 ` Mike Snitzer
0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2017-02-17 8:27 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Keith Busch, Martin K. Petersen, Christoph Hellwig, linux-scsi,
Bart van Assche, Hannes Reinecke
On Fri, Feb 17, 2017 at 09:06:14AM +0100, Hannes Reinecke wrote:
> We could, but why?
> ATM we're only having SCSI devices able to use device handler; adding
> another layer of indirection doesn't solve anything here.
> Moving the infrastructure one level up will only make sense if we're
> getting non-SCSI device handler (ANA?), but until then I'd think it's
> just overengineering.
Agreed. Independent of what does the balancing between queues hardware
handler should be attached by the low-level driver for any future
transport without any control from DM.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh
2017-02-17 8:27 ` Christoph Hellwig
@ 2017-02-19 5:59 ` Mike Snitzer
0 siblings, 0 replies; 9+ messages in thread
From: Mike Snitzer @ 2017-02-19 5:59 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Hannes Reinecke, Keith Busch, Martin K. Petersen, linux-scsi,
Bart van Assche, Hannes Reinecke, device-mapper development,
linux-block
On Fri, Feb 17, 2017 at 3:27 AM, Christoph Hellwig <hch@lst.de> wrote:
>
> On Fri, Feb 17, 2017 at 09:06:14AM +0100, Hannes Reinecke wrote:
> > We could, but why?
> > ATM we're only having SCSI devices able to use device handler; adding
> > another layer of indirection doesn't solve anything here.
> > Moving the infrastructure one level up will only make sense if we're
> > getting non-SCSI device handler (ANA?), but until then I'd think it's
> > just overengineering.
>
> Agreed. Independent of what does the balancing between queues hardware
> handler should be attached by the low-level driver for any future
> transport without any control from DM.
But doesn't Keith's abstraction makes a lot of sense given that you're
providing a device handler interface for NVMe?
The most important part of scsi_dh that DM uses is its calls to
scsi_dh_activate. Attachment isn't interesting or the issue (DM's
call to scsi_dh_attach is purely legacy now that SCSI attaches the
proper scsi_dh during SCSI's device scan).
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh
@ 2017-02-19 5:59 ` Mike Snitzer
0 siblings, 0 replies; 9+ messages in thread
From: Mike Snitzer @ 2017-02-19 5:59 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-block, Hannes Reinecke, linux-scsi, Martin K. Petersen,
Keith Busch, device-mapper development, Bart van Assche
On Fri, Feb 17, 2017 at 3:27 AM, Christoph Hellwig <hch@lst.de> wrote:
>
> On Fri, Feb 17, 2017 at 09:06:14AM +0100, Hannes Reinecke wrote:
> > We could, but why?
> > ATM we're only having SCSI devices able to use device handler; adding
> > another layer of indirection doesn't solve anything here.
> > Moving the infrastructure one level up will only make sense if we're
> > getting non-SCSI device handler (ANA?), but until then I'd think it's
> > just overengineering.
>
> Agreed. Independent of what does the balancing between queues hardware
> handler should be attached by the low-level driver for any future
> transport without any control from DM.
But doesn't Keith's abstraction makes a lot of sense given that you're
providing a device handler interface for NVMe?
The most important part of scsi_dh that DM uses is its calls to
scsi_dh_activate. Attachment isn't interesting or the issue (DM's
call to scsi_dh_attach is purely legacy now that SCSI attaches the
proper scsi_dh during SCSI's device scan).
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-02-19 6:05 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-16 15:12 [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh Hannes Reinecke
2017-02-16 16:09 ` Bart Van Assche
2017-02-17 7:19 ` Hannes Reinecke
2017-02-16 17:05 ` Keith Busch
2017-02-16 17:13 ` Keith Busch
2017-02-17 8:06 ` Hannes Reinecke
2017-02-17 8:27 ` Christoph Hellwig
2017-02-19 5:59 ` Mike Snitzer
2017-02-19 5:59 ` Mike Snitzer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.