* [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 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 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 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.