Re: [PATCH v9 17/17] scsi: ufs: Inform the block layer about write ordering

Bao D. Nguyen
2023-08-17T19:01:23Z

On 8/16/2023 12:53 PM, 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.
>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Can Guo <quic_cang@quicinc.com>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/ufs/core/ufshcd.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 37d430d20939..7f5049a66cca 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4356,6 +4356,19 @@ 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 ? 0 : ELEVATOR_F_ZBD_SEQ_WRITE);
> + if (q->disk)
> + disk_set_zoned(q->disk, q->limits.zoned);
> + blk_mq_unfreeze_queue(q);
> + }
>
> return 0;
> }
> @@ -4393,7 +4406,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)
> @@ -4409,7 +4423,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);
> @@ -5187,6 +5202,9 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
>
> ufshcd_hpb_configure(hba, sdev);
>
> + q->limits.driver_preserves_write_order =
> + !ufshcd_is_auto_hibern8_supported(hba) ||
> + FIELD_GET(UFSHCI_AHIBERN8_TIMER_MASK, hba->ahit) == 0;
In legacy SDB mode with auto-hibernation enabled, the default setting
for the driver_preserves_write_order = false.

Using the default setting, it may be missing this check that is part of
the ufshcd_auto_hibern8_update()->ufshcd_update_preserves_write_order().

Auto-hibernation should not be enabled by default unless these
conditions are met?
if (blk_queue_is_zoned(q) && !q->elevator)
return -EPERM;

> blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
> if (hba->quirks & UFSHCD_QUIRK_4KB_DMA_ALIGNMENT)
> blk_queue_update_dma_alignment(q, SZ_4K - 1);