| Message ID | 20231114211804.1449162-1-bvanassche@acm.org |
|---|---|
| Headers | show |
| Series | Improve write performance for zoned UFS devices | expand |
As this keeps getting reposted: I still think it is a very bad idea to add this amount of complexity to the SCSI code, for a model that can't work for the general case and diverges from the established NVMe model. So I do not thing we should support this.
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, > };
On Mon, Nov 27, 2023 at 11:35:48AM -0800, Bart Van Assche wrote:
> Here is some additional background information:
I know the background. I also know that JEDEC did all this aginst
better judgement and knowing the situation. We should not give them
their carrot after they haven't even been interested in engaging.
On 11/28/23 04:53, Christoph Hellwig wrote: > I know the background. I also know that JEDEC did all this aginst > better judgement and knowing the situation. We should not give them > their carrot after they haven't even been interested in engaging. That statement is overly negative. The JEDEC Zoned Storage for UFS standard has been published last week [1]. It can be downloaded by anyone for free after having created a JEDEC account, which is also free. As one can see in this standard, nothing excludes using a zone append command. Once T10 standardizes a zone append command, it can be implemented by UFS vendors. However, I do not know whether T10 plans to standardize a zone append command. Bart. [1] https://www.jedec.org/system/files/docs/JESD220-5.pdf
On 11/27/23 17:45, Can Guo wrote: > 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. Hi Can, Thank you for having taken the time to run this test and also for having shared your findings. I have not yet had the chance to test this patch series myself on an MCQ test setup. I will try to locate such a test setup and test this patch series on an MCQ setup. Thanks, Bart.
Hi Martin, This patch series improves small write IOPS by a factor of four (+300%) for zoned UFS devices on my test setup with an UFSHCI 3.0 controller. Please consider this patch series for the next merge window. Thank you, Bart. Changes compared to v14: - Removed the drivers/scsi/Kconfig.kunit and drivers/scsi/Makefile.kunit files. Instead, modified drivers/scsi/Kconfig and added #include "*_test.c" directives in the appropriate .c files. Removed the EXPORT_SYMBOL() directives that were added to make the unit tests link. - Fixed a double free in a unit test. Changes compared to v13: - Reworked patch "block: Preserve the order of requeued zoned writes". - Addressed a performance concern by removing the eh_needs_prepare_resubmit SCSI driver callback and by introducing the SCSI host template flag .needs_prepare_resubmit instead. - Added a patch that adds a 'host' argument to scsi_eh_flush_done_q(). - Made the code in unit tests less repetitive. Changes compared to v12: - Added two new patches: "block: Preserve the order of requeued zoned writes" and "scsi: sd: Add a unit test for sd_cmp_sector()" - Restricted the number of zoned write retries. To my surprise I had to add "&& scmd->retries <= scmd->allowed" in the SCSI error handler to limit the number of retries. - In patch "scsi: ufs: Inform the block layer about write ordering", only set ELEVATOR_F_ZBD_SEQ_WRITE for zoned block devices. Changes compared to v11: - Fixed a NULL pointer dereference that happened when booting from an ATA device by adding an scmd->device != NULL check in scsi_needs_preparation(). - Updated Reviewed-by tags. Changes compared to v10: - Dropped the UFS MediaTek and HiSilicon patches because these are not correct and because it is safe to drop these patches. - Updated Acked-by / Reviewed-by tags. Changes compared to v9: - Introduced an additional scsi_driver callback: .eh_needs_prepare_resubmit(). - Renamed the scsi_debug kernel module parameter 'no_zone_write_lock' into 'preserves_write_order'. - Fixed an out-of-bounds access in the unit scsi_call_prepare_resubmit() unit test. - Wrapped ufshcd_auto_hibern8_update() calls in UFS host drivers with WARN_ON_ONCE() such that a kernel stack appears in case an error code is returned. - Elaborated a comment in the UFSHCI driver. Changes compared to v8: - Fixed handling of 'driver_preserves_write_order' and 'use_zone_write_lock' in blk_stack_limits(). - Added a comment in disk_set_zoned(). - Modified blk_req_needs_zone_write_lock() such that it returns false if q->limits.use_zone_write_lock is false. - Modified disk_clear_zone_settings() such that it clears q->limits.use_zone_write_lock. - Left out one change from the mq-deadline patch that became superfluous due to the blk_req_needs_zone_write_lock() change. - Modified scsi_call_prepare_resubmit() such that it only calls list_sort() if zoned writes have to be resubmitted for which zone write locking is disabled. - Added an additional unit test for scsi_call_prepare_resubmit(). - Modified the sorting code in the sd driver such that only those SCSI commands are sorted for which write locking is disabled. - Modified sd_zbc.c such that ELEVATOR_F_ZBD_SEQ_WRITE is only set if the write order is not preserved. - Included three patches for UFS host drivers that rework code that wrote directly to the auto-hibernation controller register. - Modified the UFS driver such that enabling auto-hibernation is not allowed if a zoned logical unit is present and if the controller operates in legacy mode. - Also in the UFS driver, simplified ufshcd_auto_hibern8_update(). Changes compared to v7: - Split the queue_limits member variable `use_zone_write_lock' into two member variables: `use_zone_write_lock' (set by disk_set_zoned()) and `driver_preserves_write_order' (set by the block driver or SCSI LLD). This should clear up the confusion about the purpose of this variable. - Moved the code for sorting SCSI commands by LBA from the SCSI error handler into the SCSI disk (sd) driver as requested by Christoph. Changes compared to v6: - Removed QUEUE_FLAG_NO_ZONE_WRITE_LOCK and instead introduced a flag in the request queue limits data structure. Changes compared to v5: - Renamed scsi_cmp_lba() into scsi_cmp_sector(). - Improved several source code comments. Changes compared to v4: - Dropped the patch that introduces the REQ_NO_ZONE_WRITE_LOCK flag. - Dropped the null_blk patch and added two scsi_debug patches instead. - Dropped the f2fs patch. - Split the patch for the UFS driver into two patches. - Modified several patch descriptions and source code comments. - Renamed dd_use_write_locking() into dd_use_zone_write_locking(). - Moved the list_sort() call from scsi_unjam_host() into scsi_eh_flush_done_q() such that sorting happens just before reinserting. - Removed the scsi_cmd_retry_allowed() call from scsi_check_sense() to make sure that the retry counter is adjusted once per retry instead of twice. Changes compared to v3: - Restored the patch that introduces QUEUE_FLAG_NO_ZONE_WRITE_LOCK. That patch had accidentally been left out from v2. - In patch "block: Introduce the flag REQ_NO_ZONE_WRITE_LOCK", improved the patch description and added the function blk_no_zone_write_lock(). - In patch "block/mq-deadline: Only use zone locking if necessary", moved the blk_queue_is_zoned() call into dd_use_write_locking(). - In patch "fs/f2fs: Disable zone write locking", set REQ_NO_ZONE_WRITE_LOCK from inside __bio_alloc() instead of in f2fs_submit_write_bio(). Changes compared to v2: - Renamed the request queue flag for disabling zone write locking. - Introduced a new request flag for disabling zone write locking. - Modified the mq-deadline scheduler such that zone write locking is only disabled if both flags are set. - Added an F2FS patch that sets the request flag for disabling zone write locking. - Only disable zone write locking in the UFS driver if auto-hibernation is disabled. Changes compared to v1: - Left out the patches that are already upstream. - Switched the approach in patch "scsi: Retry unaligned zoned writes" from retrying immediately to sending unaligned write commands to the SCSI error handler. Bart Van Assche (19): block: Introduce more member variables related to zone write locking block: Only use write locking if necessary block: Preserve the order of requeued zoned writes block/mq-deadline: Only use zone locking if necessary scsi: Pass SCSI host pointer to scsi_eh_flush_done_q() scsi: core: Introduce a mechanism for reordering requests in the error handler scsi: core: Add unit tests for scsi_call_prepare_resubmit() scsi: sd: Support sorting commands by LBA before resubmitting scsi: sd: Add a unit test for sd_cmp_sector() scsi: core: Retry unaligned zoned writes scsi: sd_zbc: Only require an I/O scheduler if needed scsi: scsi_debug: Add the preserves_write_order module parameter scsi: scsi_debug: Support injecting unaligned write errors scsi: ufs: hisi: Rework the code that disables auto-hibernation scsi: ufs: Rename ufshcd_auto_hibern8_enable() and make it static scsi: ufs: Change the return type of ufshcd_auto_hibern8_update() scsi: ufs: Simplify ufshcd_auto_hibern8_update() scsi: ufs: Forbid auto-hibernation without I/O scheduler scsi: ufs: Inform the block layer about write ordering block/blk-mq.c | 6 +- block/blk-settings.c | 15 ++ block/blk-zoned.c | 10 +- block/mq-deadline.c | 11 +- drivers/ata/libata-eh.c | 2 +- drivers/scsi/Kconfig | 10 ++ drivers/scsi/constants.c | 1 + drivers/scsi/libsas/sas_scsi_host.c | 2 +- drivers/scsi/scsi_debug.c | 21 ++- drivers/scsi/scsi_error.c | 77 ++++++++- drivers/scsi/scsi_error_test.c | 233 ++++++++++++++++++++++++++++ drivers/scsi/scsi_lib.c | 1 + drivers/scsi/sd.c | 44 ++++++ drivers/scsi/sd_test.c | 86 ++++++++++ drivers/scsi/sd_zbc.c | 4 +- drivers/ufs/core/ufs-sysfs.c | 2 +- drivers/ufs/core/ufshcd-priv.h | 1 - drivers/ufs/core/ufshcd.c | 119 +++++++++++--- drivers/ufs/host/ufs-hisi.c | 5 +- include/linux/blkdev.h | 10 ++ include/scsi/scsi.h | 1 + include/scsi/scsi_driver.h | 1 + include/scsi/scsi_eh.h | 3 +- include/scsi/scsi_host.h | 6 + include/ufs/ufshcd.h | 3 +- 25 files changed, 628 insertions(+), 46 deletions(-) create mode 100644 drivers/scsi/scsi_error_test.c create mode 100644 drivers/scsi/sd_test.c