Re: [PATCH v15 19/19] scsi: ufs: Inform the block layer about write ordering

Can Guo
2023-11-28T01:45:43Z

Hi Bart,

On 11/15/2023 5:16 AM, Bart Van Assche wrote:
> From the UFSHCI 4.0 specification, about the legacy (single queue) mode:
> "The host controller always process transfer requests in-order according
> to the order submitted to the list. In case of multiple commands with
> single doorbell register ringing (batch mode), The dispatch order for
> these transfer requests by host controller will base on their index in
> the List. A transfer request with lower index value will be executed
> before a transfer request with higher index value."
>
> From the UFSHCI 4.0 specification, about the MCQ mode:
> "Command Submission
> 1. Host SW writes an Entry to SQ
> 2. Host SW updates SQ doorbell tail pointer
>
> Command Processing
> 3. After fetching the Entry, Host Controller updates SQ doorbell head
> pointer
> 4. Host controller sends COMMAND UPIU to UFS device"
>
> In other words, for both legacy and MCQ mode, UFS controllers are
> required to forward commands to the UFS device in the order these
> commands have been received from the host.
>
> Notes:
> - For legacy mode this is only correct if the host submits one
> command at a time. The UFS driver does this.
> - Also in legacy mode, the command order is not preserved if
> auto-hibernation is enabled in the UFS controller. Hence, enable
> zone write locking if auto-hibernation is enabled.
>
> This patch improves performance as follows on my test setup:
> - With the mq-deadline scheduler: 2.5x more IOPS for small writes.
> - When not using an I/O scheduler compared to using mq-deadline with
> zone locking: 4x more IOPS for small writes.
>
> Reviewed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
> Reviewed-by: Can Guo <quic_cang@quicinc.com>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Avri Altman <avri.altman@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/ufs/core/ufshcd.c | 25 +++++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 732509289165..e78954cda3ae 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4421,6 +4421,20 @@ static int ufshcd_update_preserves_write_order(struct ufs_hba *hba,
> return -EPERM;
> }
> }
> + shost_for_each_device(sdev, hba->host)
> + blk_freeze_queue_start(sdev->request_queue);
> + shost_for_each_device(sdev, hba->host) {
> + struct request_queue *q = sdev->request_queue;
> +
> + blk_mq_freeze_queue_wait(q);
> + q->limits.driver_preserves_write_order = preserves_write_order;
> + blk_queue_required_elevator_features(q,
> + !preserves_write_order && blk_queue_is_zoned(q) ?
> + ELEVATOR_F_ZBD_SEQ_WRITE : 0);
> + if (q->disk)
> + disk_set_zoned(q->disk, q->limits.zoned);
> + blk_mq_unfreeze_queue(q);
> + }
>
> return 0;
> }
> @@ -4463,7 +4477,8 @@ int ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit)
>
> if (!is_mcq_enabled(hba) && !prev_state && new_state) {
> /*
> - * Auto-hibernation will be enabled for legacy UFSHCI mode.
> + * Auto-hibernation will be enabled for legacy UFSHCI mode. Tell
> + * the block layer that write requests may be reordered.
> */
> ret = ufshcd_update_preserves_write_order(hba, false);
> if (ret)
> @@ -4479,7 +4494,8 @@ int ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit)
> }
> if (!is_mcq_enabled(hba) && prev_state && !new_state) {
> /*
> - * Auto-hibernation has been disabled.
> + * Auto-hibernation has been disabled. Tell the block layer that
> + * the order of write requests is preserved.
> */
> ret = ufshcd_update_preserves_write_order(hba, true);
> WARN_ON_ONCE(ret);
> @@ -5247,6 +5263,10 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
> struct ufs_hba *hba = shost_priv(sdev->host);
> struct request_queue *q = sdev->request_queue;
>
> + q->limits.driver_preserves_write_order =
> + !ufshcd_is_auto_hibern8_supported(hba) ||
> + FIELD_GET(UFSHCI_AHIBERN8_TIMER_MASK, hba->ahit) == 0;
> +

I got some time testing these changes on SM8650 with MCQ enabled. I
found that with these changes in place (with AH8 disabled). Even we can
make sure UFS driver does not re-order requests in MCQ mode, the reorder
is still happening while running FIO and can be seen from ftrace logs. I
think it is related with below logic in blk-mq-sched.c, please correct
me if I am wrong.

static int __blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
{
...
} else if (multi_hctxs) {
/*
* Requests from different hctx may be dequeued from some
* schedulers, such as bfq and deadline.
*
* Sort the requests in the list according to their hctx,
* dispatch batching requests from same hctx at a time.
*/
list_sort(NULL, &rq_list, sched_rq_cmp);
...
}

Thanks,
Can Guo.

> blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
>
> /*
> @@ -9026,6 +9046,7 @@ static const struct scsi_host_template ufshcd_driver_template = {
> .max_host_blocked = 1,
> .track_queue_depth = 1,
> .skip_settle_delay = 1,
> + .needs_prepare_resubmit = 1,
> .sdev_groups = ufshcd_driver_groups,
> .rpm_autosuspend_delay = RPM_AUTOSUSPEND_DELAY_MS,
> };