Re: [PATCH v3 3/4] sd: Make synchronize cache upon shutdown asynchronous

[Bart Van Assche <Bart.VanAssche@sandisk.com>]

On Sun, 2017-04-23 at 12:28 -0500, James Bottomley wrote:
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1250,6 +1250,12 @@ scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
>  			break;
>  		case SDEV_BLOCK:
>  		case SDEV_CREATED_BLOCK:
> +			/* q lock is held only in the non-mq case */
> +			if (req->q->mq_ops)
> +				blk_mq_stop_hw_queues(req->q);
> +			else
> +				blk_stop_queue(req->q);
> +
>  			ret = BLKPREP_DEFER;
>  			break;
>  		case SDEV_QUIESCE:

Hello James,

This change swaps the order of changing the device state and the block layer
state. Sorry but I don't like this. What will happen if e.g. the disk event
checker decides to check for events just before __scsi_remove_device()
changes the device state? I think that can that cause sd_shutdown() to be
called with the block layer queue stopped and hence that with this approach
it is still possible that sd_shutdown() hangs.

> @@ -2611,7 +2617,6 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
>  		case SDEV_QUIESCE:
>  		case SDEV_OFFLINE:
>  		case SDEV_TRANSPORT_OFFLINE:
> -		case SDEV_BLOCK:
>  			break;
>  		default:
>  			goto illegal;

A previous patch made two changes to scsi_device_set_state(). Are you sure
that we do no longer have to enable the SDEV_BLOCK to SDEV_DEL transition?

> @@ -2844,10 +2849,12 @@ static int scsi_request_fn_active(struct scsi_device *sdev)
>   */
>  static void scsi_wait_for_queuecommand(struct scsi_device *sdev)
>  {
> -	WARN_ON_ONCE(sdev->host->use_blk_mq);
> -
> -	while (scsi_request_fn_active(sdev))
> -		msleep(20);
> +	if (sdev->request_queue->mq_ops) {
> +		synchronize_rcu();
> +	} else {
> +		while (scsi_request_fn_active(sdev))
> +			msleep(20);
> +	}
>  }

The above code makes an assumption about the block layer internals, namely
that calling synchronize_rcu() is sufficient to wait for outstanding requests
to finish. Please do not embed any assumptions about block layer internals in
SCSI code but keep code that relies on block layer internals in the block
layer. If you have a look at blk_mq_quiesce_queue() then you will see that
calling synchronize_rcu() is not sufficient for hardware queues for which
BLK_MQ_F_BLOCKING has been set. I am aware that today the SCSI core does not
set that flag. However, the dependency of the dependency of the
synchronize_rcu() call on BLK_MQ_F_BLOCKING not being set is nontrivial.

Thanks,

Bart.