Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Michael Wu <michael@allwinnertech.com>]

On 25/03/2022 18:13, Ulf Hansson wrote:
> On Fri, 25 Mar 2022 at 06:46, Michael Wu <michael@allwinnertech.com> wrote:
>>
>> On 24/03/2022 19:27, Ulf Hansson wrote:
>>> On Thu, 17 Mar 2022 at 10:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>
>>>> On Wed, 16 Mar 2022 at 17:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>
>>>>> On 16.3.2022 16.46, Christian Löhle wrote:
>>>>>>> So we are not going to let the block layer know about SD cache?
>>>>>>> Or is it a separate change?
>>>>>>
>>>>>> I have some code for this laying around, but as it requires reading, parsing and writing Function Registers,
>>>>>> in particular PEH, it's a lot of boilerplate code to get the functionality, but I'll clean it up and send a patch in the coming weeks.
>>>>>>
>>>>>
>>>>> We have the sd cache flush.  We would presumably just need to call blk_queue_write_cache()
>>>>> for the !mmc_card_mmc(card) case e.g.
>>>>>
>>>>>           if (mmc_has_reliable_write(card)) {
>>>>>                   md->flags |= MMC_BLK_REL_WR;
>>>>>                   enable_fua = true;
>>>>>           }
>>>>>
>>>>>           if (mmc_cache_enabled(card->host))
>>>>>                   enable_cache = true;
>>>>>
>>>>>           blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
>>>>
>>>> To me, this seems like the most reasonable thing to do.
>>>>
>>>> However, I have to admit that it's not clear to me, if there was a
>>>> good reason to why commit f4c5522b0a88 ("mmc: Reliable write
>>>> support.") also added support for REQ_FLUSH (write back cache) and why
>>>> not only REQ_FUA. I assumed this was wrong too, right?
>>>>
>>
>> Hi Ulf,
>>
>> 1. I've found the reason. If we only enable REQ_FUA, there won't be any
>> effect -- The block layer won't send any request with FUA flag to the
>> driver.
>> If we want REQ_FUA to take effect, we must enable REQ_FLUSH. But on the
>> contrary, REQ_FLUSH does not rely on REQ_FUA.
>> In the previous patch(commit f4c5522b0a88 ("mmc: Reliable write
>> support.")), REQ_FLUSH was added to make REQ_FUA effective. I've done
>> experiments to prove this.
> 
> Thanks for doing the research and for confirming.
> 
> Note that this is also pretty well documented in
> Documentation/block/writeback_cache_control.rst.

Thanks for reminding. I'm clear now.

> 
>>
>> 2. Why block layer requires REQ_FLUSH to make REQ_FUA effective? I did
>> not find the reason. Does anyone know about this? Thank you.
> 
> The REQ_FLUSH indicates that the storage device has a write back
> cache, which also can be flushed in some device specific way.
> 
> The REQ_FUA (Force Unit Access), tells that the data can be written to
> the storage device, in a way that when the I/O request is completed,
> the data is fully written to the device (the data must not be left in
> the write back cache). In other words, REQ_FUA doesn't make sense
> unless REQ_FLUSH is supported too.
> 

Thank you for your answer.

> $subject patch should also conform to this pattern.

I'm not sure if I understood this in a right way... Did you mean I 
should modify the subject of this mail/patch?

> 
> However, it's still questionable to me whether we want to support
> REQ_FUA through the eMMC reliable write command - in case we also have
> support for managing the eMMC cache separately. It looks to me that
> the reason why we currently support REQ_FUA, is because back in the
> days when there was only the eMMC reliable write command available, it
> was simply the best we could do. But it was never really a good fit.
> 
> I am starting to think that we may consider dropping REQ_FUA, if we
> have the option to manage the eMMC cache separately - no matter
> whether the eMMC reliable write command is supported or not. In this
> case, REQ_FLUSH is sufficient and also a better match to what we
> really can support.

Hi Ulf,
As to dropping REQ_FUA, I don't know if it is a good idea, but generally 
we are facing three possible situations:

1. If both cache and reliable-write are available, both REQ_FUA and 
REQ_FLUSH can be supported at the same time. In this case, with 
available cache, the behavior of reliable-write is to write eMMC while 
skipping cache, which is consistent with the current kernel's definition 
of REQ_FUA. What's more, most eMMCs now support both cache and 
reliable-write command.
2. If only reliable-write is available, REQ_FUA should not be supported, 
which is consistent with the current standard in another way. But I 
don't think eMMCs that only support reliable-write can be easily found 
nowadays.
3. If only cache is available, we just use REQ_FLUSH. It is not in 
conflict with keeping REQ_FUA.

Maybe, is it more reasonable to reserve FUA and use if/else to pick it 
up or down, considering the compatibility? I mean, in most cases, FUA 
and FLUSH are complementary. So it seems more feasible with branch to 
choose.

>>
>>>> When it comes to patches for stable kernels. mmc_cache_enabled() was
>>>> introduced quite recently in v5.13, so for older kernels that call
>>>> needs to be replaced with something else.
>>>>
>>>> In any case, the relevant commits that can be considered as needs to
>>>> be fixed seems like these:
>>>> commit f4c5522b0a88 ("mmc: Reliable write support.")
>>>> commit 881d1c25f765 ("mmc: core: Add cache control for eMMC4.5 device")
>>>> commit 130206a615a9 ("mmc: core: Add support for cache ctrl for SD cards")
>>>>
>>>> [...]
>>>
>>> Michael, are you planning to send a v2 for this? Or are there any
>>> parts that are still unclear to you?
>>
>> Dear Ulf, Sorry for the delay. I was trying to figure out the SD cache
>> stuff, so a few day was taken...
> 
> No problem, I have been busy too. :-)
> 
> Kind regards
> Uffe

-- 
Best Regards,
Michael Wu