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

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

[Bart Van Assche]

Hi Martin,

Apparently 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.

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   | 3 +++
 include/scsi/scsi_devinfo.h | 2 ++
 3 files changed, 8 insertions(+)

[PATCH 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 <asutoshd@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_scan.c    | 3 +++
 include/scsi/scsi_devinfo.h | 2 ++
 2 files changed, 5 insertions(+)

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..f89e8d1a9d64 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -68,6 +68,8 @@
 #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
 

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

[Bart Van Assche]

From: Asutosh Das <asutoshd@codeaurora.org>

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

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index bf3cb12ef02f..461aa51cfccc 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5056,6 +5056,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 2/2] scsi: ufs: Use SYNCHRONIZE CACHE instead of FUA

[Daejun Park]

Hi Bart,
> From: Asutosh Das <asutoshd@codeaurora.org>
> 
> UFS devices perform better when using SYNCHRONIZE CACHE command
> instead of the FUA flag. Hence this patch.

If you have, could you share the result when using SYNCHRONIZE CACHE command?

Thanks,
Daejun

> 
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> [ bvanassche: modified a source code comment ]
> ---
>  drivers/ufs/core/ufshcd.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index bf3cb12ef02f..461aa51cfccc 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5056,6 +5056,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 2/2] scsi: ufs: Use SYNCHRONIZE CACHE instead of FUA

[kernel test robot]

Hi Bart,

I love your patch! Yet something to improve:

[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on jejb-scsi/for-next linus/master v6.2-rc6 next-20230202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bart-Van-Assche/scsi-core-Introduce-the-BLIST_BROKEN_FUA-flag/20230202-021019
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20230201180637.2102556-3-bvanassche%40acm.org
patch subject: [PATCH 2/2] scsi: ufs: Use SYNCHRONIZE CACHE instead of FUA
config: arm-randconfig-r046-20230130 (https://download.01.org/0day-ci/archive/20230202/202302021202.mM5iFxSO-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/4edde0173805f04faa8e79aab4de3e929ea4b7c0
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bart-Van-Assche/scsi-core-Introduce-the-BLIST_BROKEN_FUA-flag/20230202-021019
        git checkout 4edde0173805f04faa8e79aab4de3e929ea4b7c0
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/ufs/core/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/ufs/core/ufshcd.c: In function 'ufshcd_slave_alloc':
>> drivers/ufs/core/ufshcd.c:5060:29: error: 'BLIST_BROKEN_FUA' undeclared (first use in this function)
    5060 |         sdev->sdev_bflags = BLIST_BROKEN_FUA;
         |                             ^~~~~~~~~~~~~~~~
   drivers/ufs/core/ufshcd.c:5060:29: note: each undeclared identifier is reported only once for each function it appears in


vim +/BLIST_BROKEN_FUA +5060 drivers/ufs/core/ufshcd.c

  5031	
  5032	/**
  5033	 * ufshcd_slave_alloc - handle initial SCSI device configurations
  5034	 * @sdev: pointer to SCSI device
  5035	 *
  5036	 * Returns success
  5037	 */
  5038	static int ufshcd_slave_alloc(struct scsi_device *sdev)
  5039	{
  5040		struct ufs_hba *hba;
  5041	
  5042		hba = shost_priv(sdev->host);
  5043	
  5044		/* Mode sense(6) is not supported by UFS, so use Mode sense(10) */
  5045		sdev->use_10_for_ms = 1;
  5046	
  5047		/* DBD field should be set to 1 in mode sense(10) */
  5048		sdev->set_dbd_for_ms = 1;
  5049	
  5050		/* allow SCSI layer to restart the device in case of errors */
  5051		sdev->allow_restart = 1;
  5052	
  5053		/* REPORT SUPPORTED OPERATION CODES is not supported */
  5054		sdev->no_report_opcodes = 1;
  5055	
  5056		/* WRITE_SAME command is not supported */
  5057		sdev->no_write_same = 1;
  5058	
  5059		/* Use SYNCHRONIZE CACHE instead of FUA to improve performance */
> 5060		sdev->sdev_bflags = BLIST_BROKEN_FUA;
  5061	
  5062		ufshcd_lu_init(hba, sdev);
  5063	
  5064		ufshcd_setup_links(hba, sdev);
  5065	
  5066		return 0;
  5067	}
  5068	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Adrian Hunter]

On 1/02/23 20:06, Bart Van Assche wrote:
> From: Asutosh Das <asutoshd@codeaurora.org>
> 
> UFS devices perform better when using SYNCHRONIZE CACHE command
> instead of the FUA flag. Hence this patch.

Hi

It would be nice to get some clarification on what is
going on for this case.

This includes with Data Reliability enabled?

In theory, WRITE+FUA should be at least as fast as
WRITE+SYNCHRONIZE CACHE, right?

Do we have any explanation for why that would not
be true?

In particular, is SYNCHRONIZE CACHE faster because
it is not, in fact, providing Reliable Writes?

> 
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> [ bvanassche: modified a source code comment ]
> ---
>  drivers/ufs/core/ufshcd.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index bf3cb12ef02f..461aa51cfccc 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5056,6 +5056,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 2/2] scsi: ufs: Use SYNCHRONIZE CACHE instead of FUA

[Bart Van Assche]

On 2/1/23 23:52, Adrian Hunter wrote:
> On 1/02/23 20:06, Bart Van Assche wrote:
>> UFS devices perform better when using SYNCHRONIZE CACHE command
>> instead of the FUA flag. Hence this patch.
> 
> It would be nice to get some clarification on what is
> going on for this case.
> 
> This includes with Data Reliability enabled?
> 
> In theory, WRITE+FUA should be at least as fast as
> WRITE+SYNCHRONIZE CACHE, right?
> 
> Do we have any explanation for why that would not
> be true?
> 
> In particular, is SYNCHRONIZE CACHE faster because
> it is not, in fact, providing Reliable Writes?
  Hi Adrian,

Setting the FUA bit in a WRITE command is functionally equivalent to 
submitting a WRITE command without FUA and submitting a SYNCHRONIZE 
CACHE command afterwards. For both sequences the storage device has to 
guarantee that the written data will survive a sudden power loss event.

It is not clear to me why WRITE + SYNCHRONIZE CACHE is faster than WRITE 
+ FUA. All I know is that this behavior has been observed for multiple 
UFS devices from multiple vendors. I hope that one of the UFS vendors 
can provide more information.

Bart.

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

[James Bottomley]

On Thu, 2023-02-02 at 10:09 -0800, Bart Van Assche wrote:
> On 2/1/23 23:52, Adrian Hunter wrote:
> > On 1/02/23 20:06, Bart Van Assche wrote:
> > > UFS devices perform better when using SYNCHRONIZE CACHE command
> > > instead of the FUA flag. Hence this patch.
> > 
> > It would be nice to get some clarification on what is
> > going on for this case.
> > 
> > This includes with Data Reliability enabled?
> > 
> > In theory, WRITE+FUA should be at least as fast as
> > WRITE+SYNCHRONIZE CACHE, right?
> > 
> > Do we have any explanation for why that would not
> > be true?
> > 
> > In particular, is SYNCHRONIZE CACHE faster because
> > it is not, in fact, providing Reliable Writes?
>   Hi Adrian,
> 
> Setting the FUA bit in a WRITE command is functionally equivalent to 
> submitting a WRITE command without FUA and submitting a SYNCHRONIZE 
> CACHE command afterwards. For both sequences the storage device has
> to guarantee that the written data will survive a sudden power loss
> event.

Well, that may not be true in all situations.  Semantically FUA is a
barrier: it can be implemented such that it destages only the current
write plus the cache writes that occurred before the write with the
FUA.  It could also be implemented as you suggest above, which simply
destages the entire cache, but it doesn't have to be.  One of the
reasons for FUA to exist is the potential difference between the two.

James

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

[Bart Van Assche]

On 2/2/23 10:46, James Bottomley wrote:
> Well, that may not be true in all situations.  Semantically FUA is a
> barrier: it can be implemented such that it destages only the current
> write plus the cache writes that occurred before the write with the
> FUA.  It could also be implemented as you suggest above, which simply
> destages the entire cache, but it doesn't have to be.  One of the
> reasons for FUA to exist is the potential difference between the two.

Hi James,

Although support for the barrier concept has been removed from the block 
layer, would it be possible to tell me in which T10 document I can find 
more information about the barrier semantics? All I found in the latest 
SBC-5 draft (revision 4; 2023-01-24) about FUA is the following (section 
5.40 WRITE (10)):

"A force unit access (FUA) bit set to one specifies that the device 
server shall write the logical blocks to:
a) the non-volatile cache, if any; or
b) the medium.
An FUA bit set to zero specifies that the device server shall write the 
logical blocks to:
a) volatile cache, if any;
b) non-volatile cache, if any; or
c) the medium."

To me the description of FUA in the SBC-3 draft from 11 November 2013 
seems identical to the above text.

Thanks,

Bart.

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

[James Bottomley]

On Thu, 2023-02-02 at 11:00 -0800, Bart Van Assche wrote:
> On 2/2/23 10:46, James Bottomley wrote:
> > Well, that may not be true in all situations.  Semantically FUA is
> > a barrier: it can be implemented such that it destages only the
> > current write plus the cache writes that occurred before the write
> > with the FUA.  It could also be implemented as you suggest above,
> > which simply destages the entire cache, but it doesn't have to be. 
> > One of the reasons for FUA to exist is the potential difference
> > between the two.
> 
> Hi James,
> 
> Although support for the barrier concept has been removed from the
> block layer, would it be possible to tell me in which T10 document I
> can find  more information about the barrier semantics? All I found
> in the latest  SBC-5 draft (revision 4; 2023-01-24) about FUA is the
> following (section 5.40 WRITE (10)):

I have only a vague recollection of manufacturers implementing this
semantic but ...

> "A force unit access (FUA) bit set to one specifies that the device 
> server shall write the logical blocks to:
> a) the non-volatile cache, if any; or
> b) the medium.
> An FUA bit set to zero specifies that the device server shall write
> the logical blocks to:
> a) volatile cache, if any;
> b) non-volatile cache, if any; or
> c) the medium."
> 
> To me the description of FUA in the SBC-3 draft from 11 November 2013
> seems identical to the above text.

So what that says is the FUA write writes to the medium and *doesn't*
flush the volatile cache (so any writeback data stays there).  I assume
this style is for metadata reservations, so we guarantee fs tree
consistency but not necessarily file data consistency.  However, that
makes flushing everything a way bigger hammer than this behaviour.

James

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

[kernel test robot]

Hi Bart,

I love your patch! Yet something to improve:

[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on jejb-scsi/for-next linus/master v6.2-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bart-Van-Assche/scsi-core-Introduce-the-BLIST_BROKEN_FUA-flag/20230202-021019
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20230201180637.2102556-3-bvanassche%40acm.org
patch subject: [PATCH 2/2] scsi: ufs: Use SYNCHRONIZE CACHE instead of FUA
config: x86_64-randconfig-a012-20230130 (https://download.01.org/0day-ci/archive/20230202/202302021626.GfHCwXIh-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/4edde0173805f04faa8e79aab4de3e929ea4b7c0
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bart-Van-Assche/scsi-core-Introduce-the-BLIST_BROKEN_FUA-flag/20230202-021019
        git checkout 4edde0173805f04faa8e79aab4de3e929ea4b7c0
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/ufs/core/ufshcd.c:5060:22: error: use of undeclared identifier 'BLIST_BROKEN_FUA'
           sdev->sdev_bflags = BLIST_BROKEN_FUA;
                               ^
   drivers/ufs/core/ufshcd.c:9988:44: warning: shift count >= width of type [-Wshift-count-overflow]
                   if (!dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(64)))
                                                            ^~~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
   #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
                                                        ^ ~~~
   1 warning and 1 error generated.


vim +/BLIST_BROKEN_FUA +5060 drivers/ufs/core/ufshcd.c

  5031	
  5032	/**
  5033	 * ufshcd_slave_alloc - handle initial SCSI device configurations
  5034	 * @sdev: pointer to SCSI device
  5035	 *
  5036	 * Returns success
  5037	 */
  5038	static int ufshcd_slave_alloc(struct scsi_device *sdev)
  5039	{
  5040		struct ufs_hba *hba;
  5041	
  5042		hba = shost_priv(sdev->host);
  5043	
  5044		/* Mode sense(6) is not supported by UFS, so use Mode sense(10) */
  5045		sdev->use_10_for_ms = 1;
  5046	
  5047		/* DBD field should be set to 1 in mode sense(10) */
  5048		sdev->set_dbd_for_ms = 1;
  5049	
  5050		/* allow SCSI layer to restart the device in case of errors */
  5051		sdev->allow_restart = 1;
  5052	
  5053		/* REPORT SUPPORTED OPERATION CODES is not supported */
  5054		sdev->no_report_opcodes = 1;
  5055	
  5056		/* WRITE_SAME command is not supported */
  5057		sdev->no_write_same = 1;
  5058	
  5059		/* Use SYNCHRONIZE CACHE instead of FUA to improve performance */
> 5060		sdev->sdev_bflags = BLIST_BROKEN_FUA;
  5061	
  5062		ufshcd_lu_init(hba, sdev);
  5063	
  5064		ufshcd_setup_links(hba, sdev);
  5065	
  5066		return 0;
  5067	}
  5068	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests