Skip to contentSkip to conversation

scsi: use 'scsi_device_from_queue()' for scsi_dh

PATCH Superseded Hannes Reinecke 5 participants 8 messages Feb 16, 2017 – Feb 19, 2017

Subsystems (4)
SCSI Block layer PCI Device mapper

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Hannes Reinecke

author
#0 lore raw

PATCH v2 1/1 scsi: use 'scsi_device_from_queue()' for scsi_dh

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 <[email protected]>
---
 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(-)
drivers/scsi/scsi_dh.c +4 -18
index b8d3b97..84addee 100644
@@ -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;
drivers/scsi/scsi_lib.c +26 -0
index 9d6aed5..1a81ef9 100644
@@ -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()
  *
include/scsi/scsi_device.h +1 -0
index 8990e58..be41c76 100644
@@ -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 *,
Signature · 1 line
1.8.5.6
#1 lore raw
Quoted 7 lines · Hannes Reinecke
drivers/scsi/scsi_lib.c +7 -0
+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) {

drivers/scsi/scsi_lib.c void scsi_mq_destroy_tags(struct Scsi_Host *shost) · line 2171

Quoted 10 lines
drivers/scsi/scsi_lib.c +10 -0
+		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.

author
#2 lore raw
Quoted 21 lines · Bart Van Assche
Quoted 7 lines · Hannes Reinecke
drivers/scsi/scsi_lib.c +7 -0
+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) {

drivers/scsi/scsi_lib.c void scsi_mq_destroy_tags(struct Scsi_Host *shost) · line 2171

Quoted 10 lines
drivers/scsi/scsi_lib.c +10 -0
+		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,

Quoted 7 lines

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

Signature · 5 lines
Dr. Hannes Reinecke		   Teamlead Storage & Networking
[email protected]			               +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)
#3 lore raw
In reply to Hannes Reinecke
Quoted 5 lines · 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 <[email protected]>

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)
---

drivers/md/dm-mpath.c +4 -4
index 3570bcb..28310a2 100644
@@ -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);
 }
drivers/pci/pcie/pcie-dpc.c +2 -2
index 06d2ed4..49a061a 100644
@@ -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,
drivers/scsi/scsi_lib.c +9 -0
index e9e1e14..e23c7d5 100644
@@ -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,
include/linux/blkdev.h +38 -0
index 1ca8e8f..5899768 100644
@@ -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 */
include/scsi/scsi_dh.h +0 -1
index c7bba2b..9169a66 100644
@@ -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 */

--

#4 lore raw
Quoted 7 lines · Keith Busch
Quoted 5 lines · 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 <[email protected]>
Quoted 7 lines

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:

---

drivers/md/dm-mpath.c +4 -4
index 3570bcb..28310a2 100644
@@ -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);
 }
drivers/scsi/scsi_lib.c +9 -0
index e9e1e14..e23c7d5 100644
@@ -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,
include/linux/blkdev.h +38 -0
index 1ca8e8f..5899768 100644
@@ -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 */
include/scsi/scsi_dh.h +0 -1
index c7bba2b..9169a66 100644
@@ -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 */

--

author
#5 lore raw
In reply to Keith Busch
Quoted 7 lines · Keith Busch
Quoted 5 lines · 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 <[email protected]>
Quoted 7 lines

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

Signature · 5 lines
Dr. Hannes Reinecke		   Teamlead Storage & Networking
[email protected]			               +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)
#6 lore raw
Quoted 6 lines · Hannes Reinecke

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.

#7 lore raw
Quoted 9 lines · Christoph Hellwig
Quoted 6 lines · Hannes Reinecke

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.

Quoted 3 lines

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