Re: [RFC PATCH v4 4/4] scsi: ufs-qcom: add Inline Crypto Engine support

From: Eric Biggers <ebiggers@kernel.org>
To: Thara Gopinath <thara.gopinath@linaro.org>
Cc: linux-scsi@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-block@vger.kernel.org, linux-fscrypt@vger.kernel.org,
	Alim Akhtar <alim.akhtar@samsung.com>,
	Andy Gross <agross@kernel.org>, Avri Altman <avri.altman@wdc.com>,
	Barani Muthukumaran <bmuthuku@qti.qualcomm.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Can Guo <cang@codeaurora.org>,
	Elliot Berman <eberman@codeaurora.org>,
	John Stultz <john.stultz@linaro.org>,
	Satya Tangirala <satyat@google.com>
Subject: Re: [RFC PATCH v4 4/4] scsi: ufs-qcom: add Inline Crypto Engine support
Date: Fri, 29 May 2020 14:38:43 -0700	[thread overview]
Message-ID: <20200529213843.GA220669@gmail.com> (raw)
In-Reply-To: <676394c6-4250-8998-d959-68cd218991e5@linaro.org>

On Fri, May 29, 2020 at 05:25:47PM -0400, Thara Gopinath wrote:
> > > Hi Eric,
> > > 
> > > I tested this manually on db845c, sm8150-mtp and sm8250-mtp.(I added the dts
> > > file entries for 8150 and 8250).
> > > 
> > > I also ran OsBench test case createfiles[1] on the above platforms.
> > > Following are the results on a non encrypted and encrypted directory on the
> > > same file system(lower the number better)
> > > 
> > > 			8250-MTP	8150-MTP	DB845
> > > 
> > > nonencrypt_dir(us) 	55.3108954	26.8323124    69.5709552
> > > encrypt_dir(us) 	70.0214426	37.5411254    92.3818296
> > > 
> > > 
> > > 
> > > 1. https://github.com/mbitsnbites/osbench/blob/master/README.md
> > > 
> > 
> > Great, thanks for testing.
> > 
> > Note that the benchmark you ran (creating many small files, then deleting them)
> > mostly tests the performance of filenames encryption and directory operations,
> > not file contents encryption.  Inline encryption is only used for file contents.
> > 
> > In fact, since that benchmark doesn't sync the files before deleting them, there
> > is no guarantee that any file contents are actually written to disk, and hence
> > no guarantee that inline encryption got used at all.
> 
> Hi Eric,
> 
> The results are particularly interesting if you think a sync is not
> happening. 

You can check the source code
(https://github.com/mbitsnbites/osbench/blob/master/src/create_files.c).
There's no sync.

> There should not be any performance regression in this case
> between the two directories. 

That's not true; the filenames still need to be encrypted.  Filenames encryption
happens right away, not later when the pages are written to disk.  Contents
encryption works differently.

> I can try some reading/writing performance tests rather than creating files
> testing.
> > 
> > It would be more relevant to test the performance of reading/writing file data.
> > 
> > Also, did you try doing any correctness tests?  (See what I suggested earlier.)
> 
> I did correctness test as part of manual tests by diffing the content of the
> copied files and verifying them. I did not run any other tests you
> mentioned. Feel free to add my Tested-by in the next version you send out.
> 

Okay, thanks!

- Eric