[PATCH 10/24] scsi-multipath: add scsi_mpath_{start,end}_request()

John Garry posted 24 patches 1 month, 1 week ago
[PATCH 10/24] scsi-multipath: add scsi_mpath_{start,end}_request()
Posted by John Garry 1 month, 1 week ago
Add scsi_mpath_{start,end}_request() to handle updating private multipath
request data, like nvme_mpath_{start,end}_request().

Since we may need to update mpath_disk data, add a callbacks in
scsi_driver to actually do this work for the scsi driver.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_lib.c       |  4 ++++
 include/scsi/scsi_driver.h    |  2 ++
 include/scsi/scsi_multipath.h | 24 ++++++++++++++++++++++++
 3 files changed, 30 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7ed0defc8161e..61179caa7b2c8 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -654,6 +654,8 @@ static bool scsi_end_request(struct request *req, blk_status_t error,
 	 */
 	destroy_rcu_head(&cmd->rcu);
 
+	scsi_mpath_end_request(req);
+
 	/*
 	 * In the MQ case the command gets freed by __blk_mq_end_request,
 	 * so we have to do all cleanup that depends on it earlier.
@@ -1887,6 +1889,8 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
 	cmd->submitter = SUBMITTED_BY_BLOCK_LAYER;
 
+	scsi_mpath_start_request(req);
+
 	blk_mq_start_request(req);
 	if (blk_mq_is_reserved_rq(req)) {
 		reason = shost->hostt->queue_reserved_command(shost, cmd);
diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
index 85e792dc4db50..44e50229a75e7 100644
--- a/include/scsi/scsi_driver.h
+++ b/include/scsi/scsi_driver.h
@@ -20,6 +20,8 @@ struct scsi_driver {
 	int (*eh_action)(struct scsi_cmnd *, int);
 	void (*eh_reset)(struct scsi_cmnd *);
 	#ifdef CONFIG_SCSI_MULTIPATH
+	void (*mpath_start_cmd)(struct scsi_cmnd *);
+	void (*mpath_end_cmd)(struct scsi_cmnd *);
 	struct mpath_disk *(*to_mpath_disk)(struct request *);
 	#endif
 };
diff --git a/include/scsi/scsi_multipath.h b/include/scsi/scsi_multipath.h
index 07db217edb085..6cb3107260952 100644
--- a/include/scsi/scsi_multipath.h
+++ b/include/scsi/scsi_multipath.h
@@ -56,6 +56,23 @@ void scsi_mpath_add_sysfs_link(struct scsi_device *sdev);
 void scsi_mpath_remove_sysfs_link(struct scsi_device *sdev);
 int scsi_mpath_get_head(struct scsi_mpath_head *);
 void scsi_mpath_put_head(struct scsi_mpath_head *);
+
+static inline void scsi_mpath_start_request(struct request *req)
+{
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
+
+	if (is_mpath_request(req))
+		scsi_cmd_to_driver(cmd)->mpath_start_cmd(cmd);
+}
+
+static inline void scsi_mpath_end_request(struct request *req)
+{
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
+
+	if (is_mpath_request(req))
+		scsi_cmd_to_driver(cmd)->mpath_start_cmd(cmd);
+}
+
 #else /* CONFIG_SCSI_MULTIPATH */
 
 struct scsi_mpath_head {
@@ -104,6 +121,13 @@ static inline void scsi_mpath_put_head(struct scsi_mpath_head *)
 {
 }
 
+static inline void scsi_mpath_start_request(struct request *)
+{
+}
+static inline void scsi_mpath_end_request(struct request *)
+{
+}
+
 static inline void scsi_mpath_add_sysfs_link(struct scsi_device *sdev)
 {
 }
-- 
2.43.5
Re: [PATCH 10/24] scsi-multipath: add scsi_mpath_{start,end}_request()
Posted by Benjamin Marzinski 1 month ago
On Wed, Feb 25, 2026 at 03:36:13PM +0000, John Garry wrote:
> Add scsi_mpath_{start,end}_request() to handle updating private multipath
> request data, like nvme_mpath_{start,end}_request().
> 
> Since we may need to update mpath_disk data, add a callbacks in
> scsi_driver to actually do this work for the scsi driver.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>  drivers/scsi/scsi_lib.c       |  4 ++++
>  include/scsi/scsi_driver.h    |  2 ++
>  include/scsi/scsi_multipath.h | 24 ++++++++++++++++++++++++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7ed0defc8161e..61179caa7b2c8 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -654,6 +654,8 @@ static bool scsi_end_request(struct request *req, blk_status_t error,
>  	 */
>  	destroy_rcu_head(&cmd->rcu);
>  
> +	scsi_mpath_end_request(req);
> +
>  	/*
>  	 * In the MQ case the command gets freed by __blk_mq_end_request,
>  	 * so we have to do all cleanup that depends on it earlier.

This looks wrong. We start accounting in scsi_queue_rq(), and we need to
end it whenever we complete or requeue the request, otherwise the
accounting will get off. But not all requests go through
scsi_end_request(). scsi_mpath_failover_req(), for instance, calls
blk_mq_end_request() directly, and other functions, like
scsi_queue_insert() call blk_mq_requeue_request(). I'm pretty sure that
this should go in scsi_complete(), as well in the error path of
scsi_queue_rq().

-Ben
Re: [PATCH 10/24] scsi-multipath: add scsi_mpath_{start,end}_request()
Posted by John Garry 1 month ago
On 04/03/2026 06:13, Benjamin Marzinski wrote:
>>   
>> +	scsi_mpath_end_request(req);
>> +
>>   	/*
>>   	 * In the MQ case the command gets freed by __blk_mq_end_request,
>>   	 * so we have to do all cleanup that depends on it earlier.
> This looks wrong. We start accounting in scsi_queue_rq(), and we need to
> end it whenever we complete or requeue the request, otherwise the
> accounting will get off. But not all requests go through
> scsi_end_request(). scsi_mpath_failover_req(), for instance, calls
> blk_mq_end_request() directly, and other functions, like
> scsi_queue_insert() call blk_mq_requeue_request(). I'm pretty sure that
> this should go in scsi_complete(), as well in the error path of
> scsi_queue_rq().

ok, let me check that further.

Thanks for the notice.
Re: [PATCH 10/24] scsi-multipath: add scsi_mpath_{start,end}_request()
Posted by Benjamin Marzinski 1 month ago
On Wed, Mar 04, 2026 at 11:11:08AM +0000, John Garry wrote:
> On 04/03/2026 06:13, Benjamin Marzinski wrote:
> > > +	scsi_mpath_end_request(req);
> > > +
> > >   	/*
> > >   	 * In the MQ case the command gets freed by __blk_mq_end_request,
> > >   	 * so we have to do all cleanup that depends on it earlier.
> > This looks wrong. We start accounting in scsi_queue_rq(), and we need to
> > end it whenever we complete or requeue the request, otherwise the
> > accounting will get off. But not all requests go through
> > scsi_end_request(). scsi_mpath_failover_req(), for instance, calls
> > blk_mq_end_request() directly, and other functions, like
> > scsi_queue_insert() call blk_mq_requeue_request(). I'm pretty sure that
> > this should go in scsi_complete(), as well in the error path of
> > scsi_queue_rq().
> 
> ok, let me check that further.
> 

I think I was a little hasty here. Looking at sd_mpath_start_command()
and sd_mpath_end_command() in patch 17, I can see that they protect
against repeat calls, so requeueing the request should be o.k. There's
still a problem when scsi_mpath_failover_req() calls blk_mq_end_request()
directly, and when scsi_queue_rq() exits with a failure where the
request won't requeued (all the returns except BLK_STS_OK,
BLK_STS_RESOURCE, and BLK_STS_DEV_RESOURCE).

-Ben

> Thanks for the notice.
Re: [PATCH 10/24] scsi-multipath: add scsi_mpath_{start,end}_request()
Posted by Benjamin Marzinski 1 month ago
On Wed, Feb 25, 2026 at 03:36:13PM +0000, John Garry wrote:
> Add scsi_mpath_{start,end}_request() to handle updating private multipath
> request data, like nvme_mpath_{start,end}_request().
> 
> Since we may need to update mpath_disk data, add a callbacks in
> scsi_driver to actually do this work for the scsi driver.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>  drivers/scsi/scsi_lib.c       |  4 ++++
>  include/scsi/scsi_driver.h    |  2 ++
>  include/scsi/scsi_multipath.h | 24 ++++++++++++++++++++++++
>  3 files changed, 30 insertions(+)
> 
>  	#ifdef CONFIG_SCSI_MULTIPATH
> +	void (*mpath_start_cmd)(struct scsi_cmnd *);
> +	void (*mpath_end_cmd)(struct scsi_cmnd *);
>  	struct mpath_disk *(*to_mpath_disk)(struct request *);
>  	#endif
>  };
> diff --git a/include/scsi/scsi_multipath.h b/include/scsi/scsi_multipath.h
> index 07db217edb085..6cb3107260952 100644
> --- a/include/scsi/scsi_multipath.h
> +++ b/include/scsi/scsi_multipath.h
> @@ -56,6 +56,23 @@ void scsi_mpath_add_sysfs_link(struct scsi_device *sdev);
>  void scsi_mpath_remove_sysfs_link(struct scsi_device *sdev);
>  int scsi_mpath_get_head(struct scsi_mpath_head *);
>  void scsi_mpath_put_head(struct scsi_mpath_head *);
> +
> +static inline void scsi_mpath_start_request(struct request *req)
> +{
> +	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
> +
> +	if (is_mpath_request(req))
> +		scsi_cmd_to_driver(cmd)->mpath_start_cmd(cmd);
> +}
> +
> +static inline void scsi_mpath_end_request(struct request *req)
> +{
> +	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
> +
> +	if (is_mpath_request(req))
> +		scsi_cmd_to_driver(cmd)->mpath_start_cmd(cmd);

Copy-paste error. It should be:

		scsi_cmd_to_driver(cmd)->mpath_end_cmd(cmd);

-Ben
Re: [PATCH 10/24] scsi-multipath: add scsi_mpath_{start,end}_request()
Posted by John Garry 1 month ago
On 02/03/2026 04:08, Benjamin Marzinski wrote:
>> +static inline void scsi_mpath_start_request(struct request *req)
>> +{
>> +	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
>> +
>> +	if (is_mpath_request(req))
>> +		scsi_cmd_to_driver(cmd)->mpath_start_cmd(cmd);
>> +}
>> +
>> +static inline void scsi_mpath_end_request(struct request *req)
>> +{
>> +	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
>> +
>> +	if (is_mpath_request(req))
>> +		scsi_cmd_to_driver(cmd)->mpath_start_cmd(cmd);
> Copy-paste error. It should be:
> 
> 		scsi_cmd_to_driver(cmd)->mpath_end_cmd(cmd);


Of course, obviously my testing needs more coverage.

Thanks!