[PATCH] mmc: core: Allow to avoid REQ_FUA if the eMMC supports an internal cache

[PATCH] mmc: core: Allow to avoid REQ_FUA if the eMMC supports an internal cache

[Ulf Hansson]

REQ_FUA translates into so called "reliable writes" (atomic writes) for
eMMC cards, which is generally supported as it was introduced as a
mandatory feature already in the v4.3 (2007) of the eMMC spec. To fully
support the reliable writes (thus REQ_FUA), the mmc host driver needs to
support the CMD23 (MMC_CAP_CMD23) too, which is rather common nowadays.

File systems typically uses REQ_FUA for writing their meta-data and other
important information. Ideally it should provide an increased protection
against data-corruption, during sudden power-failures. This said, file
systems have other ways to handle sudden power-failures too, like using
checksums to detect partly-written data, for example.

It has been reported that the reliable writes are costly for some eMMCs,
leading to performance degradations. Exactly why, is in the implementation
details of the internals of the eMMC.

Moreover, in the v4.5 (2011) of the eMMC spec, the cache-control was
introduced as an optional feature. It allows the host to trigger a flush of
the eMMC's internal write-cache. In the past, before the cache-control
feature was added, the reliable write acted as trigger for the eMMC, to
also flush its internal write-cache, even if that too remains as an
implementation detail of the eMMC.

In a way to try to improve the situation with costly reliable writes and
REQ_FUA, let's add a new card quirk MMC_QUIRK_AVOID_REL_WRITE, which may be
set to avoid announcing the support for it. However, as mentioned above,
due to the specific relationship with the cache-control feature, we must
keep REQ_FUA unless that is supported too.

Reported-by: Wenchao Chen <wenchao.chen666@gmail.com>
Acked-by: Bean Huo <beanhuo@micron.com>
Acked-by: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Updated since the RFC:
	Added a card quirk to maintain the current behaviour. The quirk isn't
	set for any cards yet, which is needed (a patch on top) to move forward
	with this.

---
 drivers/mmc/core/block.c | 16 ++++++++++++----
 drivers/mmc/core/card.h  |  5 +++++
 include/linux/mmc/card.h |  1 +
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 672ab90c4b2d..35292e36a1fb 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2409,8 +2409,7 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 	struct mmc_blk_data *md;
 	int devidx, ret;
 	char cap_str[10];
-	bool cache_enabled = false;
-	bool fua_enabled = false;
+	bool cache_enabled, avoid_fua, fua_enabled = false;
 
 	devidx = ida_simple_get(&mmc_blk_ida, 0, max_devices, GFP_KERNEL);
 	if (devidx < 0) {
@@ -2494,11 +2493,20 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 	    ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
 	     card->ext_csd.rel_sectors)) {
 		md->flags |= MMC_BLK_REL_WR;
+	}
+
+	/*
+	 * REQ_FUA is supported through eMMC reliable writes, which has been
+	 * reported to be a bit costly for some eMMCs. In these cases, let's
+	 * rely on the flush requests (REQ_OP_FLUSH) instead, if we can use the
+	 * cache-control feature too.
+	 */
+	cache_enabled = mmc_cache_enabled(card->host);
+	avoid_fua = cache_enabled && mmc_card_avoid_rel_write(card);
+	if (md->flags & MMC_BLK_REL_WR && !avoid_fua) {
 		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);
 
diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
index cfdd1ff40b86..2ab448fa2841 100644
--- a/drivers/mmc/core/card.h
+++ b/drivers/mmc/core/card.h
@@ -229,6 +229,11 @@ static inline int mmc_blksz_for_byte_mode(const struct mmc_card *c)
 	return c->quirks & MMC_QUIRK_BLKSZ_FOR_BYTE_MODE;
 }
 
+static inline int mmc_card_avoid_rel_write(const struct mmc_card *c)
+{
+	return c->quirks & MMC_QUIRK_AVOID_REL_WRITE;
+}
+
 static inline int mmc_card_disable_cd(const struct mmc_card *c)
 {
 	return c->quirks & MMC_QUIRK_DISABLE_CD;
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index c726ea781255..4d297d565c83 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -282,6 +282,7 @@ struct mmc_card {
 						/* for byte mode */
 #define MMC_QUIRK_NONSTD_SDIO	(1<<2)		/* non-standard SDIO card attached */
 						/* (missing CIA registers) */
+#define MMC_QUIRK_AVOID_REL_WRITE (1<<3)	/* Avoid rel-write (REQ_FUA) */
 #define MMC_QUIRK_NONSTD_FUNC_IF (1<<4)		/* SDIO card has nonstd function interfaces */
 #define MMC_QUIRK_DISABLE_CD	(1<<5)		/* disconnect CD/DAT[3] resistor */
 #define MMC_QUIRK_INAND_CMD38	(1<<6)		/* iNAND devices have broken CMD38 */
-- 
2.34.1

Re: [PATCH] mmc: core: Allow to avoid REQ_FUA if the eMMC supports an internal cache

[Christoph Hellwig]

On Thu, Mar 16, 2023 at 05:45:14PM +0100, Ulf Hansson wrote:
> File systems typically uses REQ_FUA for writing their meta-data and other
> important information. Ideally it should provide an increased protection
> against data-corruption, during sudden power-failures. This said, file
> systems have other ways to handle sudden power-failures too, like using
> checksums to detect partly-written data, for example.

Sorry, but this is completely wrong and dangerous, if not intentionally
misleading bullshit.

The only way to provide data integrity is to ensure data is written to
the media and not a cache.  That can be done by a full blown cache
flush, or with a FUA-like optimization.

Re: [PATCH] mmc: core: Allow to avoid REQ_FUA if the eMMC supports an internal cache

[Adrian Hunter]

On 16/03/23 18:49, Christoph Hellwig wrote:
> On Thu, Mar 16, 2023 at 05:45:14PM +0100, Ulf Hansson wrote:
>> File systems typically uses REQ_FUA for writing their meta-data and other
>> important information. Ideally it should provide an increased protection
>> against data-corruption, during sudden power-failures. This said, file
>> systems have other ways to handle sudden power-failures too, like using
>> checksums to detect partly-written data, for example.
> 
> Sorry, but this is completely wrong and dangerous, if not intentionally
> misleading bullshit.

There is some context missing.

Historically file systems have assumed that sectors are updated
atomically i.e. there is never a sector with a mixture of
old and new data. The eMMC spec does not guarantee that,
except for the eMMC "reliable write" operation. So the paragraph
above is informing the potential benefit of reliable write instead
of cache flush.

Note, it is not that eMMC cannot avoid torn sectors, it is that
the specification does not mandate that they do.

However, the issue has been raised that reliable write is not
needed to provide sufficient assurance of data integrity, and that
in fact, cache flush can be used instead and perform better.

This patch adds a card quirk MMC_QUIRK_AVOID_REL_WRITE
that can be set for cards where cache flush outperforms
reliable write.  We would expect acks from someone in the
manufacturer's organization on patches setting the quirk, effectively
assuring fitness-for-purpose, and implying that torn sectors are
not especially more likely than any other sort of data integrity
failure.

> 
> The only way to provide data integrity is to ensure data is written to
> the media and not a cache.  That can be done by a full blown cache
> flush, or with a FUA-like optimization.

The patch is just reporting (via blk_queue_write_cache())
that FUA is not supported, so a flush will be done instead.

Re: [PATCH] mmc: core: Allow to avoid REQ_FUA if the eMMC supports an internal cache

[Christoph Hellwig]

On Thu, Mar 16, 2023 at 09:12:35PM +0200, Adrian Hunter wrote:
> Historically file systems have assumed that sectors are updated
> atomically i.e. there is never a sector with a mixture of
> old and new data.

Yes.  Not just file systems, but also all kinds of applications.

> The eMMC spec does not guarantee that,
> except for the eMMC "reliable write" operation.

Neither to ATA or SCSI, but applications and file systems always very
much expected it, so withou it storage devices would be considered
fault.  Only NVMe actually finally made it part of the standard.

> So the paragraph
> above is informing the potential benefit of reliable write instead
> of cache flush.

But these are completely separate issue.  Torn writes are completely
unrelated to cache flushes.  You can indeed work around torn writes
by checksums, but not the lack of cache flushes or vice versa.


> Note, it is not that eMMC cannot avoid torn sectors, it is that
> the specification does not mandate that they do.

If devices tear writes it will break not only various file systems,
but more importantly applications, at least on file systems without
data checksum (aka all except for btrfs, and even that has a nodatacsum
option).

> However, the issue has been raised that reliable write is not
> needed to provide sufficient assurance of data integrity, and that
> in fact, cache flush can be used instead and perform better.

It does not.

Re: [PATCH] mmc: core: Allow to avoid REQ_FUA if the eMMC supports an internal cache

[Ulf Hansson]

On Mon, 20 Mar 2023 at 07:25, Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, Mar 16, 2023 at 09:12:35PM +0200, Adrian Hunter wrote:
> > Historically file systems have assumed that sectors are updated
> > atomically i.e. there is never a sector with a mixture of
> > old and new data.
>
> Yes.  Not just file systems, but also all kinds of applications.
>
> > The eMMC spec does not guarantee that,
> > except for the eMMC "reliable write" operation.
>
> Neither to ATA or SCSI, but applications and file systems always very
> much expected it, so withou it storage devices would be considered
> fault.  Only NVMe actually finally made it part of the standard.

Even if the standard doesn't say, it's perfectly possible that the
storage device implements it.

Hence, $subject patch isn't changing anything in regards to REQ_FUA,
unless the eMMC device/vendor has agreed to this (via the MMC card
quirks).

>
> > So the paragraph
> > above is informing the potential benefit of reliable write instead
> > of cache flush.
>
> But these are completely separate issue.  Torn writes are completely
> unrelated to cache flushes.  You can indeed work around torn writes
> by checksums, but not the lack of cache flushes or vice versa.

It's not a separate issue for eMMC. Please read the complete commit
message for further clarifications in this regard.

>
>
> > Note, it is not that eMMC cannot avoid torn sectors, it is that
> > the specification does not mandate that they do.
>
> If devices tear writes it will break not only various file systems,
> but more importantly applications, at least on file systems without
> data checksum (aka all except for btrfs, and even that has a nodatacsum
> option).

Yes, you are correct. Again, the card quirk (as suggested in $subject
patch) helps us to manage eMMC cards in different ways. We should not
avoid REQ_FUA (reliable writes) for an eMMC that actually needs it.

>
> > However, the issue has been raised that reliable write is not
> > needed to provide sufficient assurance of data integrity, and that
> > in fact, cache flush can be used instead and perform better.
>
> It does not.

Can you please elaborate on this?

The tests we have done so far indicate that performance differs based
upon what eMMC we are using, for example.

Kind regards
Uffe

Re: [PATCH] mmc: core: Allow to avoid REQ_FUA if the eMMC supports an internal cache

[Adrian Hunter]

On 16/03/23 18:45, Ulf Hansson wrote:
> REQ_FUA translates into so called "reliable writes" (atomic writes) for
> eMMC cards, which is generally supported as it was introduced as a
> mandatory feature already in the v4.3 (2007) of the eMMC spec. To fully
> support the reliable writes (thus REQ_FUA), the mmc host driver needs to
> support the CMD23 (MMC_CAP_CMD23) too, which is rather common nowadays.
> 
> File systems typically uses REQ_FUA for writing their meta-data and other
> important information. Ideally it should provide an increased protection
> against data-corruption, during sudden power-failures. This said, file
> systems have other ways to handle sudden power-failures too, like using
> checksums to detect partly-written data, for example.
> 
> It has been reported that the reliable writes are costly for some eMMCs,
> leading to performance degradations. Exactly why, is in the implementation
> details of the internals of the eMMC.
> 
> Moreover, in the v4.5 (2011) of the eMMC spec, the cache-control was
> introduced as an optional feature. It allows the host to trigger a flush of
> the eMMC's internal write-cache. In the past, before the cache-control
> feature was added, the reliable write acted as trigger for the eMMC, to
> also flush its internal write-cache, even if that too remains as an
> implementation detail of the eMMC.
> 
> In a way to try to improve the situation with costly reliable writes and
> REQ_FUA, let's add a new card quirk MMC_QUIRK_AVOID_REL_WRITE, which may be
> set to avoid announcing the support for it. However, as mentioned above,
> due to the specific relationship with the cache-control feature, we must
> keep REQ_FUA unless that is supported too.
> 
> Reported-by: Wenchao Chen <wenchao.chen666@gmail.com>
> Acked-by: Bean Huo <beanhuo@micron.com>
> Acked-by: Avri Altman <avri.altman@wdc.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Minor cosmetic suggestion below, but nevertheless:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> 
> Updated since the RFC:
> 	Added a card quirk to maintain the current behaviour. The quirk isn't
> 	set for any cards yet, which is needed (a patch on top) to move forward
> 	with this.
> 
> ---
>  drivers/mmc/core/block.c | 16 ++++++++++++----
>  drivers/mmc/core/card.h  |  5 +++++
>  include/linux/mmc/card.h |  1 +
>  3 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 672ab90c4b2d..35292e36a1fb 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -2409,8 +2409,7 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
>  	struct mmc_blk_data *md;
>  	int devidx, ret;
>  	char cap_str[10];
> -	bool cache_enabled = false;
> -	bool fua_enabled = false;
> +	bool cache_enabled, avoid_fua, fua_enabled = false;
>  
>  	devidx = ida_simple_get(&mmc_blk_ida, 0, max_devices, GFP_KERNEL);
>  	if (devidx < 0) {
> @@ -2494,11 +2493,20 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
>  	    ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
>  	     card->ext_csd.rel_sectors)) {
>  		md->flags |= MMC_BLK_REL_WR;
> +	}
> +
> +	/*
> +	 * REQ_FUA is supported through eMMC reliable writes, which has been
> +	 * reported to be a bit costly for some eMMCs. In these cases, let's
> +	 * rely on the flush requests (REQ_OP_FLUSH) instead, if we can use the
> +	 * cache-control feature too.
> +	 */
> +	cache_enabled = mmc_cache_enabled(card->host);
> +	avoid_fua = cache_enabled && mmc_card_avoid_rel_write(card);
> +	if (md->flags & MMC_BLK_REL_WR && !avoid_fua) {
>  		fua_enabled = true;
>  		cache_enabled = true;
>  	}

looks like this could be just:

	fua_enabled = (md->flags & MMC_BLK_REL_WR) && !avoid_fua;

with fua_enabled no longer needing initialization

> -	if (mmc_cache_enabled(card->host))
> -		cache_enabled  = true;
>  
>  	blk_queue_write_cache(md->queue.queue, cache_enabled, fua_enabled);
>  
> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> index cfdd1ff40b86..2ab448fa2841 100644
> --- a/drivers/mmc/core/card.h
> +++ b/drivers/mmc/core/card.h
> @@ -229,6 +229,11 @@ static inline int mmc_blksz_for_byte_mode(const struct mmc_card *c)
>  	return c->quirks & MMC_QUIRK_BLKSZ_FOR_BYTE_MODE;
>  }
>  
> +static inline int mmc_card_avoid_rel_write(const struct mmc_card *c)
> +{
> +	return c->quirks & MMC_QUIRK_AVOID_REL_WRITE;
> +}
> +
>  static inline int mmc_card_disable_cd(const struct mmc_card *c)
>  {
>  	return c->quirks & MMC_QUIRK_DISABLE_CD;
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index c726ea781255..4d297d565c83 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -282,6 +282,7 @@ struct mmc_card {
>  						/* for byte mode */
>  #define MMC_QUIRK_NONSTD_SDIO	(1<<2)		/* non-standard SDIO card attached */
>  						/* (missing CIA registers) */
> +#define MMC_QUIRK_AVOID_REL_WRITE (1<<3)	/* Avoid rel-write (REQ_FUA) */
>  #define MMC_QUIRK_NONSTD_FUNC_IF (1<<4)		/* SDIO card has nonstd function interfaces */
>  #define MMC_QUIRK_DISABLE_CD	(1<<5)		/* disconnect CD/DAT[3] resistor */
>  #define MMC_QUIRK_INAND_CMD38	(1<<6)		/* iNAND devices have broken CMD38 */

Re: [PATCH] mmc: core: Allow to avoid REQ_FUA if the eMMC supports an internal cache

[Ulf Hansson]

On Tue, 21 Mar 2023 at 11:36, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 16/03/23 18:45, Ulf Hansson wrote:
> > REQ_FUA translates into so called "reliable writes" (atomic writes) for
> > eMMC cards, which is generally supported as it was introduced as a
> > mandatory feature already in the v4.3 (2007) of the eMMC spec. To fully
> > support the reliable writes (thus REQ_FUA), the mmc host driver needs to
> > support the CMD23 (MMC_CAP_CMD23) too, which is rather common nowadays.
> >
> > File systems typically uses REQ_FUA for writing their meta-data and other
> > important information. Ideally it should provide an increased protection
> > against data-corruption, during sudden power-failures. This said, file
> > systems have other ways to handle sudden power-failures too, like using
> > checksums to detect partly-written data, for example.
> >
> > It has been reported that the reliable writes are costly for some eMMCs,
> > leading to performance degradations. Exactly why, is in the implementation
> > details of the internals of the eMMC.
> >
> > Moreover, in the v4.5 (2011) of the eMMC spec, the cache-control was
> > introduced as an optional feature. It allows the host to trigger a flush of
> > the eMMC's internal write-cache. In the past, before the cache-control
> > feature was added, the reliable write acted as trigger for the eMMC, to
> > also flush its internal write-cache, even if that too remains as an
> > implementation detail of the eMMC.
> >
> > In a way to try to improve the situation with costly reliable writes and
> > REQ_FUA, let's add a new card quirk MMC_QUIRK_AVOID_REL_WRITE, which may be
> > set to avoid announcing the support for it. However, as mentioned above,
> > due to the specific relationship with the cache-control feature, we must
> > keep REQ_FUA unless that is supported too.
> >
> > Reported-by: Wenchao Chen <wenchao.chen666@gmail.com>
> > Acked-by: Bean Huo <beanhuo@micron.com>
> > Acked-by: Avri Altman <avri.altman@wdc.com>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Minor cosmetic suggestion below, but nevertheless:
>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Thanks!

>
> > ---
> >
> > Updated since the RFC:
> >       Added a card quirk to maintain the current behaviour. The quirk isn't
> >       set for any cards yet, which is needed (a patch on top) to move forward
> >       with this.
> >
> > ---
> >  drivers/mmc/core/block.c | 16 ++++++++++++----
> >  drivers/mmc/core/card.h  |  5 +++++
> >  include/linux/mmc/card.h |  1 +
> >  3 files changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > index 672ab90c4b2d..35292e36a1fb 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -2409,8 +2409,7 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
> >       struct mmc_blk_data *md;
> >       int devidx, ret;
> >       char cap_str[10];
> > -     bool cache_enabled = false;
> > -     bool fua_enabled = false;
> > +     bool cache_enabled, avoid_fua, fua_enabled = false;
> >
> >       devidx = ida_simple_get(&mmc_blk_ida, 0, max_devices, GFP_KERNEL);
> >       if (devidx < 0) {
> > @@ -2494,11 +2493,20 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
> >           ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
> >            card->ext_csd.rel_sectors)) {
> >               md->flags |= MMC_BLK_REL_WR;
> > +     }
> > +
> > +     /*
> > +      * REQ_FUA is supported through eMMC reliable writes, which has been
> > +      * reported to be a bit costly for some eMMCs. In these cases, let's
> > +      * rely on the flush requests (REQ_OP_FLUSH) instead, if we can use the
> > +      * cache-control feature too.
> > +      */
> > +     cache_enabled = mmc_cache_enabled(card->host);
> > +     avoid_fua = cache_enabled && mmc_card_avoid_rel_write(card);
> > +     if (md->flags & MMC_BLK_REL_WR && !avoid_fua) {
> >               fua_enabled = true;
> >               cache_enabled = true;
> >       }
>
> looks like this could be just:
>
>         fua_enabled = (md->flags & MMC_BLK_REL_WR) && !avoid_fua;
>
> with fua_enabled no longer needing initialization

Unless I misunderstand your point, that would work for fua_enabled,
but would not be sufficient for cache_enabled.

cache_enabled should be set if fua_enabled is set - and no matter
whether mmc_cache_enabled() returns true or not.

Did that make sense?

[...]

Kind regards
Uffe

Re: [PATCH] mmc: core: Allow to avoid REQ_FUA if the eMMC supports an internal cache

[Adrian Hunter]

On 21/03/23 13:03, Ulf Hansson wrote:
> On Tue, 21 Mar 2023 at 11:36, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 16/03/23 18:45, Ulf Hansson wrote:
>>> REQ_FUA translates into so called "reliable writes" (atomic writes) for
>>> eMMC cards, which is generally supported as it was introduced as a
>>> mandatory feature already in the v4.3 (2007) of the eMMC spec. To fully
>>> support the reliable writes (thus REQ_FUA), the mmc host driver needs to
>>> support the CMD23 (MMC_CAP_CMD23) too, which is rather common nowadays.
>>>
>>> File systems typically uses REQ_FUA for writing their meta-data and other
>>> important information. Ideally it should provide an increased protection
>>> against data-corruption, during sudden power-failures. This said, file
>>> systems have other ways to handle sudden power-failures too, like using
>>> checksums to detect partly-written data, for example.
>>>
>>> It has been reported that the reliable writes are costly for some eMMCs,
>>> leading to performance degradations. Exactly why, is in the implementation
>>> details of the internals of the eMMC.
>>>
>>> Moreover, in the v4.5 (2011) of the eMMC spec, the cache-control was
>>> introduced as an optional feature. It allows the host to trigger a flush of
>>> the eMMC's internal write-cache. In the past, before the cache-control
>>> feature was added, the reliable write acted as trigger for the eMMC, to
>>> also flush its internal write-cache, even if that too remains as an
>>> implementation detail of the eMMC.
>>>
>>> In a way to try to improve the situation with costly reliable writes and
>>> REQ_FUA, let's add a new card quirk MMC_QUIRK_AVOID_REL_WRITE, which may be
>>> set to avoid announcing the support for it. However, as mentioned above,
>>> due to the specific relationship with the cache-control feature, we must
>>> keep REQ_FUA unless that is supported too.
>>>
>>> Reported-by: Wenchao Chen <wenchao.chen666@gmail.com>
>>> Acked-by: Bean Huo <beanhuo@micron.com>
>>> Acked-by: Avri Altman <avri.altman@wdc.com>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Minor cosmetic suggestion below, but nevertheless:
>>
>> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> 
> Thanks!
> 
>>
>>> ---
>>>
>>> Updated since the RFC:
>>>       Added a card quirk to maintain the current behaviour. The quirk isn't
>>>       set for any cards yet, which is needed (a patch on top) to move forward
>>>       with this.
>>>
>>> ---
>>>  drivers/mmc/core/block.c | 16 ++++++++++++----
>>>  drivers/mmc/core/card.h  |  5 +++++
>>>  include/linux/mmc/card.h |  1 +
>>>  3 files changed, 18 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>>> index 672ab90c4b2d..35292e36a1fb 100644
>>> --- a/drivers/mmc/core/block.c
>>> +++ b/drivers/mmc/core/block.c
>>> @@ -2409,8 +2409,7 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
>>>       struct mmc_blk_data *md;
>>>       int devidx, ret;
>>>       char cap_str[10];
>>> -     bool cache_enabled = false;
>>> -     bool fua_enabled = false;
>>> +     bool cache_enabled, avoid_fua, fua_enabled = false;
>>>
>>>       devidx = ida_simple_get(&mmc_blk_ida, 0, max_devices, GFP_KERNEL);
>>>       if (devidx < 0) {
>>> @@ -2494,11 +2493,20 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
>>>           ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
>>>            card->ext_csd.rel_sectors)) {
>>>               md->flags |= MMC_BLK_REL_WR;
>>> +     }
>>> +
>>> +     /*
>>> +      * REQ_FUA is supported through eMMC reliable writes, which has been
>>> +      * reported to be a bit costly for some eMMCs. In these cases, let's
>>> +      * rely on the flush requests (REQ_OP_FLUSH) instead, if we can use the
>>> +      * cache-control feature too.
>>> +      */
>>> +     cache_enabled = mmc_cache_enabled(card->host);
>>> +     avoid_fua = cache_enabled && mmc_card_avoid_rel_write(card);
>>> +     if (md->flags & MMC_BLK_REL_WR && !avoid_fua) {
>>>               fua_enabled = true;
>>>               cache_enabled = true;
>>>       }
>>
>> looks like this could be just:
>>
>>         fua_enabled = (md->flags & MMC_BLK_REL_WR) && !avoid_fua;
>>
>> with fua_enabled no longer needing initialization
> 
> Unless I misunderstand your point, that would work for fua_enabled,
> but would not be sufficient for cache_enabled.
> 
> cache_enabled should be set if fua_enabled is set - and no matter
> whether mmc_cache_enabled() returns true or not.
> 
> Did that make sense?

Yes, you are right, sorry!

> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH] mmc: core: Allow to avoid REQ_FUA if the eMMC supports an internal cache

[Christoph Hellwig]

On Mon, Mar 20, 2023 at 04:24:36PM +0100, Ulf Hansson wrote:
> > Neither to ATA or SCSI, but applications and file systems always very
> > much expected it, so withou it storage devices would be considered
> > fault.  Only NVMe actually finally made it part of the standard.
> 
> Even if the standard doesn't say, it's perfectly possible that the
> storage device implements it.

That's exactly what I'm saying above.

> > But these are completely separate issue.  Torn writes are completely
> > unrelated to cache flushes.  You can indeed work around torn writes
> > by checksums, but not the lack of cache flushes or vice versa.
> 
> It's not a separate issue for eMMC. Please read the complete commit
> message for further clarifications in this regard.

The commit message claims that checksums replace cache flushes.  Which
is dangerously wrong.  So please don't refer me to it again - this
dangerously incorrect commit message is wht alerted me to reply to the
patch.

> > > However, the issue has been raised that reliable write is not
> > > needed to provide sufficient assurance of data integrity, and that
> > > in fact, cache flush can be used instead and perform better.
> >
> > It does not.
> 
> Can you please elaborate on this?

Flushing caches does not replace the invariant of not tearing subsector
writes.  And if you need to use reliable writes for (some) devices to
not tear sectors, no amount of cache flushing is going to paper over
the problem.

Re: [PATCH] mmc: core: Allow to avoid REQ_FUA if the eMMC supports an internal cache

[Ulf Hansson]

On Tue, 21 Mar 2023 at 13:37, Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Mar 20, 2023 at 04:24:36PM +0100, Ulf Hansson wrote:
> > > Neither to ATA or SCSI, but applications and file systems always very
> > > much expected it, so withou it storage devices would be considered
> > > fault.  Only NVMe actually finally made it part of the standard.
> >
> > Even if the standard doesn't say, it's perfectly possible that the
> > storage device implements it.
>
> That's exactly what I'm saying above.
>
> > > But these are completely separate issue.  Torn writes are completely
> > > unrelated to cache flushes.  You can indeed work around torn writes
> > > by checksums, but not the lack of cache flushes or vice versa.
> >
> > It's not a separate issue for eMMC. Please read the complete commit
> > message for further clarifications in this regard.
>
> The commit message claims that checksums replace cache flushes.  Which
> is dangerously wrong.  So please don't refer me to it again - this
> dangerously incorrect commit message is wht alerted me to reply to the
> patch.

That was not the intent, but rather to state that REQ_FUA isn't the
only thing a filesystem can rely on, there are other things too. If
it's striving towards being more tolerant to sudden power failures, I
mean.

Anyway, thanks for your advice, I will drop these parts from the
commit message to make sure it doesn't cause confusion.

>
> > > > However, the issue has been raised that reliable write is not
> > > > needed to provide sufficient assurance of data integrity, and that
> > > > in fact, cache flush can be used instead and perform better.
> > >
> > > It does not.
> >
> > Can you please elaborate on this?
>
> Flushing caches does not replace the invariant of not tearing subsector
> writes.  And if you need to use reliable writes for (some) devices to
> not tear sectors, no amount of cache flushing is going to paper over
> the problem.

Of course, then I get your point!

I think the confusing part here is that the internals of the eMMC
treats a "reliable write" as a cache flush too. At least this is the
case for earlier eMMC devices, where the write-cache couldn't be
explicitly controlled by the host.

Kind regards
Uffe