[PATCH v2 0/2] Use SYNCHRONIZE CACHE instead of FUA for UFS devices

[PATCH v2 0/2] Use SYNCHRONIZE CACHE instead of FUA for UFS devices

[Bart Van Assche]

Hi Martin,

Measurements have shown that UFS devices perform better when using SYNCHRONIZE
CACHE instead of FUA. Hence this patch series that makes the SCSI core submit
a SYNCHRONIZE CACHE command instead of setting the FUA bit for UFS
devices. Please consider this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v1:
- Updated __BLIST_LAST_USED.
- Added #include <scsi/scsi_devinfo.h> to fix a build error reported by the
  kernel robot.

Asutosh Das (1):
  scsi: ufs: Use SYNCHRONIZE CACHE instead of FUA

Bart Van Assche (1):
  scsi: core: Introduce the BLIST_BROKEN_FUA flag

 drivers/scsi/scsi_scan.c    | 3 +++
 drivers/ufs/core/ufshcd.c   | 4 ++++
 include/scsi/scsi_devinfo.h | 4 +++-
 3 files changed, 10 insertions(+), 1 deletion(-)

[PATCH v2 1/2] scsi: core: Introduce the BLIST_BROKEN_FUA flag

[Bart Van Assche]

UFS devices perform better when using SYNCHRONIZE CACHE command instead of
the FUA flag. Introduce the BLIST_BROKEN_FUA flag for using SYNCHRONIZE
CACHE instead of FUA.

The BLIST_BROKEN_FUA flag can be set by using one of the following
approaches:
* From user space, by writing into /proc/scsi/device_info.
* From the kernel, by setting sdev_bflags from the slave_alloc callback.

For a prior version of this patch, see also
https://lore.kernel.org/lkml/YpecaXfIxZBHIcfj@google.com/T/

Cc: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: Asutosh Das <quic_asutoshd@quicinc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_scan.c    | 3 +++
 include/scsi/scsi_devinfo.h | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index a62925355c2c..c1eb6bfe7ed2 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1020,6 +1020,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	if (*bflags & BLIST_NO_RSOC)
 		sdev->no_report_opcodes = 1;
 
+	if (*bflags & BLIST_BROKEN_FUA)
+		sdev->broken_fua = 1;
+
 	/* set the device running here so that slave configure
 	 * may do I/O */
 	mutex_lock(&sdev->state_mutex);
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 5d14adae21c7..783451dfa46e 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -68,8 +68,10 @@
 #define BLIST_RETRY_ITF		((__force blist_flags_t)(1ULL << 32))
 /* Always retry ABORTED_COMMAND with ASC 0xc1 */
 #define BLIST_RETRY_ASC_C1	((__force blist_flags_t)(1ULL << 33))
+/* Use SYNCHRONIZE CACHE instead of FUA */
+#define BLIST_BROKEN_FUA	((__force blist_flags_t)(1ULL << 34))
 
-#define __BLIST_LAST_USED BLIST_RETRY_ASC_C1
+#define __BLIST_LAST_USED BLIST_BROKEN_FUA
 
 #define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
 			       (__force blist_flags_t) \

Re: [PATCH v2 1/2] scsi: core: Introduce the BLIST_BROKEN_FUA flag

[Bart Van Assche]

On 2/2/23 14:00, Bart Van Assche wrote:
> UFS devices perform better when using SYNCHRONIZE CACHE command instead of
> the FUA flag. Introduce the BLIST_BROKEN_FUA flag for using SYNCHRONIZE
> CACHE instead of FUA.
> 
> The BLIST_BROKEN_FUA flag can be set by using one of the following
> approaches:
> * From user space, by writing into /proc/scsi/device_info.
> * From the kernel, by setting sdev_bflags from the slave_alloc callback.
> 
> For a prior version of this patch, see also
> https://lore.kernel.org/lkml/YpecaXfIxZBHIcfj@google.com/T/
> 
> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
> Cc: Asutosh Das <quic_asutoshd@quicinc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/scsi_scan.c    | 3 +++
>   include/scsi/scsi_devinfo.h | 4 +++-
>   2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index a62925355c2c..c1eb6bfe7ed2 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1020,6 +1020,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>   	if (*bflags & BLIST_NO_RSOC)
>   		sdev->no_report_opcodes = 1;
>   
> +	if (*bflags & BLIST_BROKEN_FUA)
> +		sdev->broken_fua = 1;
> +
>   	/* set the device running here so that slave configure
>   	 * may do I/O */
>   	mutex_lock(&sdev->state_mutex);
> diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
> index 5d14adae21c7..783451dfa46e 100644
> --- a/include/scsi/scsi_devinfo.h
> +++ b/include/scsi/scsi_devinfo.h
> @@ -68,8 +68,10 @@
>   #define BLIST_RETRY_ITF		((__force blist_flags_t)(1ULL << 32))
>   /* Always retry ABORTED_COMMAND with ASC 0xc1 */
>   #define BLIST_RETRY_ASC_C1	((__force blist_flags_t)(1ULL << 33))
> +/* Use SYNCHRONIZE CACHE instead of FUA */
> +#define BLIST_BROKEN_FUA	((__force blist_flags_t)(1ULL << 34))
>   
> -#define __BLIST_LAST_USED BLIST_RETRY_ASC_C1
> +#define __BLIST_LAST_USED BLIST_BROKEN_FUA
>   
>   #define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
>   			       (__force blist_flags_t) \

(replying to an email from two months ago)

I plan to repost the above patch because this patch will allow us to set the
broken_fua flag from user space. Please let me know if there are any objections
or concerns.

Thanks,

Bart.

[PATCH v2 2/2] scsi: ufs: Use SYNCHRONIZE CACHE instead of FUA

[Bart Van Assche]

From: Asutosh Das <quic_asutoshd@quicinc.com>

UFS devices perform better when using SYNCHRONIZE CACHE command
instead of the FUA flag. Hence this patch.

Signed-off-by: Asutosh Das <quic_asutoshd@quicinc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
[ bvanassche: modified a source code comment ]
---
 drivers/ufs/core/ufshcd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index bf3cb12ef02f..a8d42d9308da 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -24,6 +24,7 @@
 #include <linux/sched/clock.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_dbg.h>
+#include <scsi/scsi_devinfo.h>
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_eh.h>
 #include "ufshcd-priv.h"
@@ -5056,6 +5057,9 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev)
 	/* WRITE_SAME command is not supported */
 	sdev->no_write_same = 1;
 
+	/* Use SYNCHRONIZE CACHE instead of FUA to improve performance */
+	sdev->sdev_bflags = BLIST_BROKEN_FUA;
+
 	ufshcd_lu_init(hba, sdev);
 
 	ufshcd_setup_links(hba, sdev);

Re: [PATCH v2 0/2] Use SYNCHRONIZE CACHE instead of FUA for UFS devices

[Christoph Hellwig]

On Thu, Feb 02, 2023 at 02:00:39PM -0800, Bart Van Assche wrote:
> Hi Martin,
> 
> Measurements have shown that UFS devices perform better when using SYNCHRONIZE
> CACHE instead of FUA. Hence this patch series that makes the SCSI core submit
> a SYNCHRONIZE CACHE command instead of setting the FUA bit for UFS
> devices. Please consider this patch series for the next merge window.

NAK.  This is a policy decision that might make sense for current UFS
devices.  If you want to do it use the sysfs files from udev to quirk
it up for them.  But there is nothing inherent in the UFS transport
that speaks against using FUA.

And please lobby your suppliers to either don't claim FUA support or
implement it in a useful way in the future.  Unlikely most of us you
and your employer actually have that power in the market.

Re: [PATCH v2 0/2] Use SYNCHRONIZE CACHE instead of FUA for UFS devices

[Bart Van Assche]

On 2/2/23 22:30, Christoph Hellwig wrote:
> On Thu, Feb 02, 2023 at 02:00:39PM -0800, Bart Van Assche wrote:
>> Hi Martin,
>>
>> Measurements have shown that UFS devices perform better when using SYNCHRONIZE
>> CACHE instead of FUA. Hence this patch series that makes the SCSI core submit
>> a SYNCHRONIZE CACHE command instead of setting the FUA bit for UFS
>> devices. Please consider this patch series for the next merge window.
> 
> NAK.  This is a policy decision that might make sense for current UFS
> devices.  If you want to do it use the sysfs files from udev to quirk
> it up for them.  But there is nothing inherent in the UFS transport
> that speaks against using FUA.
> 
> And please lobby your suppliers to either don't claim FUA support or
> implement it in a useful way in the future.  Unlikely most of us you
> and your employer actually have that power in the market.

Hi Christoph,

We can ask our suppliers politely to not claim FUA support in future 
devices. However we still need patch 1/2 for existing UFS devices.

Bart.

Re: [PATCH v2 0/2] Use SYNCHRONIZE CACHE instead of FUA for UFS devices

[Christoph Hellwig]

On Fri, Feb 03, 2023 at 09:54:24AM -0800, Bart Van Assche wrote:
> We can ask our suppliers politely to not claim FUA support in future
> devices. However we still need patch 1/2 for existing UFS devices.

Please add quirks for the actually affected devices, and do not
block fua for an entire transport.

Re: [PATCH v2 0/2] Use SYNCHRONIZE CACHE instead of FUA for UFS devices

[Martin K. Petersen]

Christoph,

>> We can ask our suppliers politely to not claim FUA support in future
>> devices. However we still need patch 1/2 for existing UFS devices.
>
> Please add quirks for the actually affected devices, and do not
> block fua for an entire transport.

Yeah, I agree. Let's not make assumptions about implementation
deficiencies based on transport. If there are specific devices that
perform better with SYNCHRONIZE CACHE, then we should quirk them.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 0/2] Use SYNCHRONIZE CACHE instead of FUA for UFS devices

[Bean Huo]

Hi Bart,

To this series patch:


Reviewed-by: Bean Huo <beanhuo@micron.com>


Thanks,

Bean