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>
 <CAPDyKFq8JXO6q+_JFhSGWoVfSUEtowUX85kJpsVMa0jmEEjOiQ@mail.gmail.com>
 <CAPDyKFqg5GuJ=ChPgtVPnhJiNT3=XHXR_q3vwLUFj4ZXoQK5-A@mail.gmail.com>
 <CAPDyKFrtF32AvBKCbMESP7oFT2aBhbce0KA2zzXq+szQnShbww@mail.gmail.com> <CA+Da2qzwKf6Nbz7YFj6i1Kj_3H8XKreapOHTBo+snNzMg2eW+g@mail.gmail.com>
In-Reply-To: <CA+Da2qzwKf6Nbz7YFj6i1Kj_3H8XKreapOHTBo+snNzMg2eW+g@mail.gmail.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Wed, 15 Feb 2023 16:13:15 +0100
Message-ID: <CAPDyKFohV6kzUgF=3-Zm_V3pWpj17b9LfTxcQ2mARrRh7RM9=A@mail.gmail.com>
Subject: Re: [PATCH V2 0/2] mmc: block: Support Host to control FUA
To:     Wenchao Chen <wenchao.chen666@gmail.com>
Cc:     avri.altman@wdc.com, Adrian Hunter <adrian.hunter@intel.com>,
        orsonzhai@gmail.com, baolin.wang@linux.alibaba.com,
        zhang.lyra@gmail.com, axboe@kernel.dk, 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"
Precedence: bulk

On Mon, 13 Feb 2023 at 09:21, Wenchao Chen <wenchao.chen666@gmail.com> wrote:
>
> On Thu, Feb 9, 2023 at 10:51 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Fri, 11 Nov 2022 at 13:04, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > [...]
> > >
> > > > >
> > > > > 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.
> > > >
> > >
> > > Below is a simple patch that does the above. We may not want to enable
> > > this for *all* eMMC/SD cards, but it works fine for testing and to
> > > continue the discussions here.
> > >
> > >
> > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > Date: Fri, 11 Nov 2022 12:48:02 +0100
> > > Subject: [PATCH] mmc: core: Disable REQ_FUA if the card supports an internal
> > >  cache
> > >
> > > !!!! This is not for merge, but only for test and discussions!!!
> > >
> > > It has been reported that REQ_FUA can be costly on some eMMC devices. A
> > > potential option that could mitigate this problem, is to rely solely on
> > > REQ_OP_FLUSH instead, but that requires the eMMC/SD to support an internal
> > > cache. This is an attempt to try this out to see how it behaves.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/mmc/core/block.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > > index db6d8a099910..197e9f6cdaad 100644
> > > --- a/drivers/mmc/core/block.c
> > > +++ b/drivers/mmc/core/block.c
> > > @@ -2494,15 +2494,15 @@ static struct mmc_blk_data
> > > *mmc_blk_alloc_req(struct mmc_card *card,
> > >                         md->flags |= MMC_BLK_CMD23;
> > >         }
> > >
> > > -       if (md->flags & MMC_BLK_CMD23 &&
> > > -           ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
> > > -            card->ext_csd.rel_sectors)) {
> > > +       if (mmc_cache_enabled(card->host)) {
> > > +               cache_enabled  = true;
> > > +       } else if (md->flags & MMC_BLK_CMD23 &&
> > > +                 (card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN ||
> > > +                  card->ext_csd.rel_sectors)) {
> > >                 md->flags |= MMC_BLK_REL_WR;
> > >                 fua_enabled = true;
> > >                 cache_enabled = true;
> > >         }
> > > -       if (mmc_cache_enabled(card->host))
> > > -               cache_enabled  = true;
> > >
> > >         blk_queue_write_cache(md->queue.queue, cache_enabled, fua_enabled);
> > >
> > > --
> > > 2.34.1
> >
> > Wenchao,
> >
> > Did you manage to try the above patch to see if that could improve the
> > situation?
> >
>
> Hi Uffe,
> Yes, it can solve my problem. Thank you very much.

Okay, that's very interesting news. I will prepare a formal patch and
make a new submission soon. Let's continue the discussion then.

Kind regards
Uffe