[PATCH 1/2] blk-mq: export setting request completion state

[PATCH 1/2] blk-mq: export setting request completion state

[Keith Busch]

This is preparing for drivers that want to directly alter the state of
their requests. No functional change here.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 block/blk-mq.c         | 4 +---
 include/linux/blk-mq.h | 9 +++++++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 22326612a5d3..f50559718b71 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -558,10 +558,8 @@ static void __blk_mq_complete_request(struct request *rq)
 	bool shared = false;
 	int cpu;
 
-	if (cmpxchg(&rq->state, MQ_RQ_IN_FLIGHT, MQ_RQ_COMPLETE) !=
-			MQ_RQ_IN_FLIGHT)
+	if (blk_mq_mark_complete(rq))
 		return;
-
 	if (rq->internal_tag != -1)
 		blk_mq_sched_completed_request(rq);
 
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 9b0fd11ce89a..0ae115e95476 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -289,6 +289,15 @@ void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues);
 
 void blk_mq_quiesce_queue_nowait(struct request_queue *q);
 
+/*
+ * Returns true if request was not in flight.
+ */
+static inline bool blk_mq_mark_complete(struct request *rq)
+{
+	return (cmpxchg(&rq->state, MQ_RQ_IN_FLIGHT, MQ_RQ_COMPLETE) !=
+			MQ_RQ_IN_FLIGHT);
+}
+
 /*
  * Driver command data is immediately after the request. So subtract request
  * size to get back to the original request, add request size to get the PDU.
-- 
2.14.4

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Keith Busch]

The scsi block layer requires requests claimed be the error handling be
completed by the error handler. A previous commit allowed completions
to proceed for blk-mq, breaking that assumption.

This patch prevents completions that may race with the timeout handler
by marking the state to complete, restoring the previous behavior.

Fixes: 12f5b931 ("blk-mq: Remove generation seqeunce")
Signed-off-by: Keith Busch <keith.busch at intel.com>
---

Tested using Jianchao's abort injection to scsi_debug described here:

  https://lore.kernel.org/lkml/a68ad043-26a1-d3d8-2009-504ba4230e0f at oracle.com/

 drivers/scsi/scsi_error.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 8932ae81a15a..86ee10b2c775 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -286,6 +286,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 	enum blk_eh_timer_return rtn = BLK_EH_DONE;
 	struct Scsi_Host *host = scmd->device->host;
 
+	if (req->q->mq_ops && blk_mq_mark_complete(req))
+		return rtn;
+
 	trace_scsi_dispatch_cmd_timeout(scmd);
 	scsi_log_completion(scmd, TIMEOUT_ERROR);
 
@@ -300,7 +303,8 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 			set_host_byte(scmd, DID_TIME_OUT);
 			scsi_eh_scmd_add(scmd);
 		}
-	}
+	} else if (req->q->mq_ops)
+		WRITE_ONCE(req->state, MQ_RQ_IN_FLIGHT);
 
 	return rtn;
 }
-- 
2.14.4

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Johannes Thumshirn]

On Thu, Jul 19, 2018@03:26:18PM -0600, Keith Busch wrote:
> The scsi block layer requires requests claimed be the error handling be
> completed by the error handler. A previous commit allowed completions
> to proceed for blk-mq, breaking that assumption.

Ahm the first sentence is a bit hard to parse, sorry.
Did you mean something like:

"The scsi block later requires requests claimed by the error handling
to be completed by the error handler"?

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Keith Busch]

On Fri, Jul 20, 2018@08:52:58AM +0200, Johannes Thumshirn wrote:
> On Thu, Jul 19, 2018@03:26:18PM -0600, Keith Busch wrote:
> > The scsi block layer requires requests claimed be the error handling be
> > completed by the error handler. A previous commit allowed completions
> > to proceed for blk-mq, breaking that assumption.
> 
> Ahm the first sentence is a bit hard to parse, sorry.
> Did you mean something like:
> 
> "The scsi block later requires requests claimed by the error handling
> to be completed by the error handler"?

Ugh, sorry. Yes, you are correct on the intended phrasing.

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Christoph Hellwig]

Has this been tested?  Especially with the blk_abort_request corner
case?

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Keith Busch]

On Fri, Jul 20, 2018@07:41:46AM -0700, Christoph Hellwig wrote:
> Has this been tested?  Especially with the blk_abort_request corner
> case?

Yes, that's what Jianchao's modification to scsi_debug used.

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Bart Van Assche]

On Thu, 2018-07-19@15:26 -0600, Keith Busch wrote:
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 8932ae81a15a..86ee10b2c775 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -286,6 +286,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
>  	enum blk_eh_timer_return rtn = BLK_EH_DONE;
>  	struct Scsi_Host *host = scmd->device->host;
>  
> +	if (req->q->mq_ops && blk_mq_mark_complete(req))
> +		return rtn;
> +
>  	trace_scsi_dispatch_cmd_timeout(scmd);
>  	scsi_log_completion(scmd, TIMEOUT_ERROR);
>  
> @@ -300,7 +303,8 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
>  			set_host_byte(scmd, DID_TIME_OUT);
>  			scsi_eh_scmd_add(scmd);
>  		}
> -	}
> +	} else if (req->q->mq_ops)
> +		WRITE_ONCE(req->state, MQ_RQ_IN_FLIGHT);
>  
>  	return rtn;
>  }

Modifying the completion state and req->state from the SCSI core are layering
violations. Have you considered to move the above changes into blk_mq_rq_timed_out()?
An additional benefit of that approach is that the req->q->mq_ops checks can be
left out.

Thanks,

Bart.

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Keith Busch]

On Fri, Jul 20, 2018@03:12:59PM +0000, Bart Van Assche wrote:
> On Thu, 2018-07-19@15:26 -0600, Keith Busch wrote:
> > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> > index 8932ae81a15a..86ee10b2c775 100644
> > --- a/drivers/scsi/scsi_error.c
> > +++ b/drivers/scsi/scsi_error.c
> > @@ -286,6 +286,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
> >  	enum blk_eh_timer_return rtn = BLK_EH_DONE;
> >  	struct Scsi_Host *host = scmd->device->host;
> >  
> > +	if (req->q->mq_ops && blk_mq_mark_complete(req))
> > +		return rtn;
> > +
> >  	trace_scsi_dispatch_cmd_timeout(scmd);
> >  	scsi_log_completion(scmd, TIMEOUT_ERROR);
> >  
> > @@ -300,7 +303,8 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
> >  			set_host_byte(scmd, DID_TIME_OUT);
> >  			scsi_eh_scmd_add(scmd);
> >  		}
> > -	}
> > +	} else if (req->q->mq_ops)
> > +		WRITE_ONCE(req->state, MQ_RQ_IN_FLIGHT);
> >  
> >  	return rtn;
> >  }
> 
> Modifying the completion state and req->state from the SCSI core are layering
> violations. Have you considered to move the above changes into blk_mq_rq_timed_out()?
> An additional benefit of that approach is that the req->q->mq_ops checks can be
> left out.

SCSI is the only block driver that wants this behavior. Moving it back
to generic where it used to be breaks other block drivers.

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Bart Van Assche]

On Fri, 2018-07-20@09:56 -0600, Keith Busch wrote:
> On Fri, Jul 20, 2018@03:12:59PM +0000, Bart Van Assche wrote:
> > On Thu, 2018-07-19@15:26 -0600, Keith Busch wrote:
> > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> > > index 8932ae81a15a..86ee10b2c775 100644
> > > --- a/drivers/scsi/scsi_error.c
> > > +++ b/drivers/scsi/scsi_error.c
> > > @@ -286,6 +286,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
> > >  	enum blk_eh_timer_return rtn = BLK_EH_DONE;
> > >  	struct Scsi_Host *host = scmd->device->host;
> > >  
> > > +	if (req->q->mq_ops && blk_mq_mark_complete(req))
> > > +		return rtn;
> > > +
> > >  	trace_scsi_dispatch_cmd_timeout(scmd);
> > >  	scsi_log_completion(scmd, TIMEOUT_ERROR);
> > >  
> > > @@ -300,7 +303,8 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
> > >  			set_host_byte(scmd, DID_TIME_OUT);
> > >  			scsi_eh_scmd_add(scmd);
> > >  		}
> > > -	}
> > > +	} else if (req->q->mq_ops)
> > > +		WRITE_ONCE(req->state, MQ_RQ_IN_FLIGHT);
> > >  
> > >  	return rtn;
> > >  }
> > 
> > Modifying the completion state and req->state from the SCSI core are layering
> > violations. Have you considered to move the above changes into blk_mq_rq_timed_out()?
> > An additional benefit of that approach is that the req->q->mq_ops checks can be
> > left out.
> 
> SCSI is the only block driver that wants this behavior. Moving it back
> to generic where it used to be breaks other block drivers.

That's new to me. What would break in the NVMe driver if the above change would be
present in the block layer?

Thanks,

Bart.

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Keith Busch]

On Fri, Jul 20, 2018@04:03:18PM +0000, Bart Van Assche wrote:
> On Fri, 2018-07-20@09:56 -0600, Keith Busch wrote:
> > SCSI is the only block driver that wants this behavior. Moving it back
> > to generic where it used to be breaks other block drivers.
> 
> That's new to me. What would break in the NVMe driver if the above change would be
> present in the block layer?

This is what causes the block layer to lose completions, and most drivers
don't want the kernel to lose their completions.

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Bart Van Assche]

On Fri, 2018-07-20@10:12 -0600, Keith Busch wrote:
> On Fri, Jul 20, 2018@04:03:18PM +0000, Bart Van Assche wrote:
> > On Fri, 2018-07-20@09:56 -0600, Keith Busch wrote:
> > > SCSI is the only block driver that wants this behavior. Moving it back
> > > to generic where it used to be breaks other block drivers.
> > 
> > That's new to me. What would break in the NVMe driver if the above change would be
> > present in the block layer?
> 
> This is what causes the block layer to lose completions, and most drivers
> don't want the kernel to lose their completions.

Hello Keith,

Have you considered to introduce a fourth state for block layer requests to
avoid that completions that occur while a timeout handler is in progress get
lost? That would avoid that completions get lost not only for the NVMe driver
but also for SCSI drivers. See e.g. the MQ_RQ_TIMED_OUT state in
https://www.mail-archive.com/linux-block at vger.kernel.org/msg22196.html

Thanks,

Bart.

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Keith Busch]

On Fri, Jul 20, 2018@04:20:01PM +0000, Bart Van Assche wrote:
> On Fri, 2018-07-20@10:12 -0600, Keith Busch wrote:
> > On Fri, Jul 20, 2018@04:03:18PM +0000, Bart Van Assche wrote:
> > > On Fri, 2018-07-20@09:56 -0600, Keith Busch wrote:
> > > > SCSI is the only block driver that wants this behavior. Moving it back
> > > > to generic where it used to be breaks other block drivers.
> > > 
> > > That's new to me. What would break in the NVMe driver if the above change would be
> > > present in the block layer?
> > 
> > This is what causes the block layer to lose completions, and most drivers
> > don't want the kernel to lose their completions.
> 
> Hello Keith,
> 
> Have you considered to introduce a fourth state for block layer requests to
> avoid that completions that occur while a timeout handler is in progress get
> lost? That would avoid that completions get lost not only for the NVMe driver
> but also for SCSI drivers. See e.g. the MQ_RQ_TIMED_OUT state in
> https://www.mail-archive.com/linux-block at vger.kernel.org/msg22196.html

Yes, I've considered that, and I really want to use it, but scsi may
still reference a freed request in scmd_eh_abort_handler that way.

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Bart Van Assche]

On Fri, 2018-07-20@10:23 -0600, Keith Busch wrote:
> On Fri, Jul 20, 2018@04:20:01PM +0000, Bart Van Assche wrote:
> > On Fri, 2018-07-20@10:12 -0600, Keith Busch wrote:
> > > On Fri, Jul 20, 2018@04:03:18PM +0000, Bart Van Assche wrote:
> > > > On Fri, 2018-07-20@09:56 -0600, Keith Busch wrote:
> > > > > SCSI is the only block driver that wants this behavior. Moving it back
> > > > > to generic where it used to be breaks other block drivers.
> > > > 
> > > > That's new to me. What would break in the NVMe driver if the above change would be
> > > > present in the block layer?
> > > 
> > > This is what causes the block layer to lose completions, and most drivers
> > > don't want the kernel to lose their completions.
> > 
> > Hello Keith,
> > 
> > Have you considered to introduce a fourth state for block layer requests to
> > avoid that completions that occur while a timeout handler is in progress get
> > lost? That would avoid that completions get lost not only for the NVMe driver
> > but also for SCSI drivers. See e.g. the MQ_RQ_TIMED_OUT state in
> > https://www.mail-archive.com/linux-block at vger.kernel.org/msg22196.html
> 
> Yes, I've considered that, and I really want to use it, but scsi may
> still reference a freed request in scmd_eh_abort_handler that way.

I think that's a misunderstanding. If scsi_times_out() queues an abort
asynchronously then it tells the block layer through its return value that the
SCSI core still owns the request and hence that the block layer should ignore any
completions that occur until the SCSI core calls scsi_finish_command(). That
scsi_finish_command() will trigger a call to __blk_mq_end_request(). The
scsi_times_out() return value I was referring to is called BLK_EH_DONE today and
was called BLK_EH_NOT_HANDLED in kernel version v4.17.

This also means that I got the BLK_EH_NOT_HANDLED case wrong in "blk-mq: Rework
blk-mq timeout handling again": in that case concurrent a blk_mq_complete_request()
call should be ignored instead of triggering request completion.

Bart.

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Keith Busch]

On Fri, Jul 20, 2018@04:45:05PM +0000, Bart Van Assche wrote:
> I think that's a misunderstanding. If scsi_times_out() queues an abort
> asynchronously then it tells the block layer through its return value that the
> SCSI core still owns the request and hence that the block layer should ignore any
> completions that occur until the SCSI core calls scsi_finish_command(). That
> scsi_finish_command() will trigger a call to __blk_mq_end_request(). The
> scsi_times_out() return value I was referring to is called BLK_EH_DONE today and
> was called BLK_EH_NOT_HANDLED in kernel version v4.17.
> 
> This also means that I got the BLK_EH_NOT_HANDLED case wrong in "blk-mq: Rework
> blk-mq timeout handling again": in that case concurrent a blk_mq_complete_request()
> call should be ignored instead of triggering request completion.

I definitely think it's worth revisiting that for the longer term.

For near term, I don't want scsi error handling broken for 4.18, but also
not revert the changes that fixed all the other drivers. Restoring the
old behavior that scsi wants isolated to the scsi driver seems like the
lowest touch option.

My patch restores the state that scsi had in 4.17. It still has that
gap that may lose requests forever when the scsi LLD always returns
BLK_EH_RESET_TIMER (see virtio-scsi, for example). That gap existed prior,
so that's not new with my patch. Maybe we can fix that with a slight
modification to my previous patch. It looks like SCSI really wants to
block completions only when it hands off the command to the error handler,
so we don't need to have the inflight -> compete -> inflight transition,
and the following is all that's needed:

---
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 8932ae81a15a..902c30d3c0ed 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -296,6 +296,8 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 		rtn = host->hostt->eh_timed_out(scmd);
 
 	if (rtn == BLK_EH_DONE) {
+		if (req->q->mq_ops && blk_mq_mark_complete(req))
+			return rtn;
 		if (scsi_abort_command(scmd) != SUCCESS) {
 			set_host_byte(scmd, DID_TIME_OUT);
 			scsi_eh_scmd_add(scmd);
--

[PATCH 1/2] blk-mq: export setting request completion state

[Christoph Hellwig]

> +/*
> + * Returns true if request was not in flight.
> + */
> +static inline bool blk_mq_mark_complete(struct request *rq)
> +{
> +	return (cmpxchg(&rq->state, MQ_RQ_IN_FLIGHT, MQ_RQ_COMPLETE) !=
> +			MQ_RQ_IN_FLIGHT);
> +}

This needs a much better comment describing when and how to use it.

Also the outer braces in the return statement are not required.

[PATCH 2/2] scsi: set timed out out mq requests to complete

[hch]

On Fri, Jul 20, 2018@11:24:45AM -0600, Keith Busch wrote:
> My patch restores the state that scsi had in 4.17. It still has that
> gap that may lose requests forever when the scsi LLD always returns
> BLK_EH_RESET_TIMER (see virtio-scsi, for example). That gap existed prior,
> so that's not new with my patch. Maybe we can fix that with a slight
> modification to my previous patch. It looks like SCSI really wants to
> block completions only when it hands off the command to the error handler,
> so we don't need to have the inflight -> compete -> inflight transition,
> and the following is all that's needed:

Btw, one thing we should do in blk-mq and scsi is to make the time
optional.  If the blk_mq driver doesn't even have a timeout structure
there is no point in timing out requests and enter the timeout handler
ever.  Same for those scsi drivers always returning BLK_EH_RESET_TIMER.

Whether never having timeouts is a good idea is a different discussion,
but as long as we have such drivers we should handle them somewhat sane.

> ---
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 8932ae81a15a..902c30d3c0ed 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -296,6 +296,8 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
>  		rtn = host->hostt->eh_timed_out(scmd);
>  
>  	if (rtn == BLK_EH_DONE) {
> +		if (req->q->mq_ops && blk_mq_mark_complete(req))
> +			return rtn;

This looks pretty sensible to me as a band-aid.  It just needs a very
detailed comment explaining what is going on here.

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Bart Van Assche]

On Mon, 2018-07-23@10:12 +0200, hch@lst.de wrote:
> Btw, one thing we should do in blk-mq and scsi is to make the time
> optional.  If the blk_mq driver doesn't even have a timeout structure
> there is no point in timing out requests and enter the timeout handler
> ever.

Are there any blk-mq drivers that do not define a timeout handler and that
use shared tags? I think such drivers need periodic calls to blk_mq_tag_idle().
Do you perhaps want to happen these calls from another context?

Thanks,

Bart.

[PATCH 2/2] scsi: set timed out out mq requests to complete

[Keith Busch]

On Mon, Jul 23, 2018@10:12:31AM +0200, hch@lst.de wrote:
> > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> > index 8932ae81a15a..902c30d3c0ed 100644
> > --- a/drivers/scsi/scsi_error.c
> > +++ b/drivers/scsi/scsi_error.c
> > @@ -296,6 +296,8 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
> >  		rtn = host->hostt->eh_timed_out(scmd);
> >  
> >  	if (rtn == BLK_EH_DONE) {
> > +		if (req->q->mq_ops && blk_mq_mark_complete(req))
> > +			return rtn;
> 
> This looks pretty sensible to me as a band-aid.  It just needs a very
> detailed comment explaining what is going on here.

Sounds good, v2 will be sent shortly.