Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221021073025.18831-1-wenchao.chen666@gmail.com>
 <22499ab9-340d-7059-b3ff-45342b0810cf@intel.com> <CA+Da2qyx7McYStMysPdK-DV9qiOnb9rvXYqqugxsbVgqqX59VQ@mail.gmail.com>
In-Reply-To: <CA+Da2qyx7McYStMysPdK-DV9qiOnb9rvXYqqugxsbVgqqX59VQ@mail.gmail.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Fri, 11 Nov 2022 12:34:34 +0100
Message-ID: <CAPDyKFq8JXO6q+_JFhSGWoVfSUEtowUX85kJpsVMa0jmEEjOiQ@mail.gmail.com>
Subject: Re: [PATCH V2 0/2] mmc: block: Support Host to control FUA
To:     Wenchao Chen <wenchao.chen666@gmail.com>,
        Adrian Hunter <adrian.hunter@intel.com>
Cc:     orsonzhai@gmail.com, baolin.wang@linux.alibaba.com,
        zhang.lyra@gmail.com, axboe@kernel.dk, avri.altman@wdc.com,
        kch@nvidia.com, CLoehle@hyperstone.com,
        vincent.whitchurch@axis.com, bigeasy@linutronix.de,
        s.shtylyov@omp.ru, michael@allwinnertech.com,
        linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
        megoo.tang@gmail.com, lzx.stg@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, 11 Nov 2022 at 08:59, Wenchao Chen <wenchao.chen666@gmail.com> wrot=
e:
>
> Hi Hunter
> Thank you for your review!
> I'm sorry to reply you so late because I've been too busy lately.
>
> On Fri, Oct 21, 2022 at 11:50 PM Adrian Hunter <adrian.hunter@intel.com> =
wrote:
> >
> > On 21/10/22 10:30, Wenchao Chen wrote:
> > > From: Wenchao Chen <wenchao.chen@unisoc.com>
> > >
> > > Summary
> > > =3D=3D=3D=3D=3D=3D=3D
> > > These patches[1] supports the host to turn off FUA.
> > >
> > > About FUA, roughly deal with the following two parts:
> > > 1) FUA(Forced Unit Access):
> > > - The REQ_FUA flag can be OR ed into the r/w flags of a bio submitted=
 from the
> > >   filesystem and will make sure that I/O completion for this request =
is only
> > >   signaled after the data has been committed to non-volatile storage.
> > >
> > > 2) In emmc, FUA is represented as Reliable write. code show as below:
> > > static void mmc_blk_data_prep(struct mmc_queue *mq, struct mmc_queue_=
req *mqrq,
> > >               int recovery_mode, bool *do_rel_wr_p, bool *do_data_tag=
_p)
> > > {
> > >       ...
> > >       /*
> > >        * Reliable writes are used to implement Forced Unit Access and
> > >        * are supported only on MMCs.
> > >        */
> > >       do_rel_wr =3D (req->cmd_flags & REQ_FUA) &&
> > >                       rq_data_dir(req) =3D=3D WRITE &&
> > >                       (md->flags & MMC_BLK_REL_WR);
> > >       ...
> > > }
> > >
> > > Patch structure
> > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > > patch#1:  for block
> > > patch#2:  for sdhci-sprd
> > >
> > > Tests
> > > =3D=3D=3D=3D=3D
> > > Ran 'AndroBench' to evaluate the performance:
> >
> > It would be good to have more details e.g.
> > What file system? What block size?  What journal size?
> > What file size? What record size?
> >
>
> What file system?
> F2FS
> What block size?
> Sequential: 32768KB, Random: 4KB
> What file size?
> 64MB
>
> > > 1. fua_disable =3D 1
> > > /sys/block/mmcblk0/queue # cat fua 0
> > > I tested 5 times for each case and output a average speed.
> > >
> > > 1) Sequential read:
> > > Speed: 266.8MiB/s, 265.1MiB/s, 262.9MiB/s, 268.7MiB/s, 265.2MiB/s
> > > Average speed: 265.74MiB/s
> > >
> > > 2) Random read:
> > > Speed: 98.75MiB/s, 98.7MiB/s, 98.5MiB/s, 99.4MiB/s, 98.7MiB/s
> > > Average speed: 98.81MiB/s
> > >
> > > 3) Sequential write:
> > > Speed: 199.94MiB/s, 199.1MiB/s, 205.5MiB/s, 206.5MiB/s, 191.5MiB/s
> > > Average speed: 200.5MiB/s
> > >
> > > 4) Random write:
> > > Speed: 68.6MiB/s, 71.8MiB/s, 77.1MiB/s, 64.8MiB/s, 69.3MiB/s
> > > Average speed: 70.32MiB/s
> > >
> > > 2. fua_disable =3D 0 (default 0)
> > > /sys/block/mmcblk0/queue # cat fua 1
> > > I tested 5 times for each case and output a average speed.
> > >
> > > 1) Sequential read:
> > > Speed: 259.3MiB/s, 258.8MiB/s, 258.2MiB/s, 259.5MiB/s, 253.5MiB/s
> > > Average speed: 257.86MiB/s
> > >
> > > 2) Random read:
> > > Speed: 98.9MiB/s, 101MiB/s, 101MiB/s, 99MiB/s, 101.1MiB/s
> > > Average speed: 100.2MiB/s
> > >
> > > 3) Sequential write:
> > > Speed: 153.7MiB/s, 146.2MiB/s, 151.2MiB/s, 148.8MiB/s, 147.5MiB/s
> > > Average speed: 149.48MiB/s
> > >
> > > 4) Random write:
> > > Speed: 12.9MiB/s, 12.3MiB/s, 12.6MiB/s, 12.8MiB/s, 12.8MiB/s
> > > Average speed: 12.68MiB/s
> >
> > Is every write being sync'ed of just sync at the end?
> >
>
> /*
> * Reliable writes are used to implement Forced Unit Access and
> * are supported only on MMCs.
> */
> do_rel_wr =3D (req->cmd_flags & REQ_FUA) &&
>     rq_data_dir(req) =3D=3D WRITE &&
>     (md->flags & MMC_BLK_REL_WR);
>
> A Reliable Write access shall force the data to be written to the
> nonvolatile storage=E3=80=82
> It will consume more time.

My apologies for side-stepping the discussion.

Yes, REQ_FUA is per definition a write and a flush of the
write-back-cache to non-volatile storage. So, this is indeed the
correct behaviour, even if it consumes more time to complete the
operation.

>
> > >
> > > According to the above data, disable FUA (fua_disable =3D 1) improves=
 the
> > > performance:
> > > 1)Sequential read improved by 3%.
> > > 2)Random read were down 1%.
> >
> > FUA should not affect reads.  If it is, you may want to investigate how=
.
> >
> > > 3)Sequential write improved by 34%.
> > > 4)Random write improved by 454%.
> > > Therefore, it is recommended to support the host to control FUA.
> > >
> > > Reference
> > > =3D=3D=3D=3D=3D=3D=3D=3D=3D
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.gi=
t/tree/Documentation/block/writeback_cache_control.rst
> > > [2] Embedded Multi-Media Card (e=E2=80=A2MMC) Electrical Standard (5.=
1)''
> >
> > You do not seem to have considered data integrity.
> >
> > Regular disks are assumed to provide atomic sector writes.  That is, a =
sector has either the old data or the new data, but not some corrupt mixtur=
e.
> >
> > mmc does not have that assumption, which is presumably why Reliable Wri=
te has been used instead.  Although that idea appears to have been thrown a=
way for devices with no cache by commit 08ebf903af57 ("mmc: core: Fixup sup=
port for writeback-cache for eMMC and SD").
> >
> > File systems can use FUA to mark a successful journal flush.  Whether o=
r not getting a torn sector at that point will corrupt the file system reco=
very is presumably file system specific, and maybe specific to file system =
options e.g. the use of checksums.
> >
> > It may well be that a file system can survive a torn sector at that poi=
nt, or that user space would prefer to take the risk in order to get better=
 performance.  In either of those cases, it is not really a decision for th=
e host controller driver.
> >
>
> Considering the data integrity, we did a random power-down test, and
> the experimental results were good.
>
> FUA can only reduce data loss under abnormal conditions, but cannot
> prevent data loss under abnormal conditions.
>
> I think there should be a balance between FUA and NO FUA, but
> filesystems seem to favor FUA.
>
> FUA brings a drop in random write performance. If enough tests are
> done, NO FUA is acceptable.

Testing this isn't entirely easy. It requires you to hook up
electrical switches to allow you to automate the powering on/off of
the platform(s). Then at each cycle, really make sure to stress test
the data integrity of the flash memory. Is that what the tests did -
or can you elaborate a bit on what was really tested?

In any case, the performance impact boils down to how each eMMC/SD
card internally manages reliable writes vs regular writes. Some
vendors may treat them very similarly, while others do not.

That said, trying to disable REQ_FUA from an mmc host driver is the
wrong approach, as also pointed out by Adrian above. These types of
decisions belong solely in the mmc core layer.

Instead of what the $subject series proposes, I would rather suggest
we discuss (and test) whether it could make sense to disable REQ_FUA -
*if* the eMMC/SD card supports a write-back-cache (REQ_OP_FLUSH) too.
Hence, the mmc core could then announce only REQ_OP_FLUSH.

Kind regards
Uffe