Re: [PATCH v4 5/9] scsi: Do not wait for a request in scsi_eh_lock_door()

[Hannes Reinecke <hare@suse.de>]

On 11/30/20 3:46 AM, Bart Van Assche wrote:
> scsi_eh_lock_door() is the only function in the SCSI error handler that
> calls blk_get_request(). It is not guaranteed that a request is available
> when scsi_eh_lock_door() is called. Hence pass the BLK_MQ_REQ_NOWAIT flag
> to blk_get_request().
> 
> Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/scsi_error.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index d94449188270..6de6e1bf3dcb 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1993,7 +1993,12 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
>   	struct request *req;
>   	struct scsi_request *rq;
>   
> -	req = blk_get_request(sdev->request_queue, REQ_OP_SCSI_IN, 0);
> +	/*
> +	 * It is not guaranteed that a request is available nor that
> +	 * sdev->request_queue is unfrozen. Hence the BLK_MQ_REQ_NOWAIT below.
> +	 */
> +	req = blk_get_request(sdev->request_queue, REQ_OP_SCSI_IN,
> +			      BLK_MQ_REQ_NOWAIT);
>   	if (IS_ERR(req))
>   		return;
>   	rq = scsi_req(req);
> 
Well ... had been thinking about that one, too.
The idea of this function is that prior to SCSI EH the device was locked
via scsi_set_medium_removal(). And during SCSI EH the device might have 
become unlocked, so we need to lock it again.
However, scsi_set_medium_removal() not only issues the 
PREVENT_ALLOW_MEDIUM_REMOVAL command, but also sets the 'locked' flag 
based on the result.
So if we fail to get a request here, shouldn't we unset the 'locked' 
flag, too?
And what does happen if we fail here? There is no return value, hence 
SCSI EH might run to completion, and the system will continue
with an unlocked door ...
Not sure if that's a good idea.

But anyway, at the very least unset the 'locked' flag upon failure such 
that the internal state is correctly updated.

_Actually_, the flag should be unset after each successful SCSI EH step, 
to mirror the actual state. But this is probably out of scope for this 
patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer