Re: [PATCH v7 1/7] block: add a sanity check for non-write flush/fua bios

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 1/3/23 17:05, Niklas Cassel wrote:
> On Tue, Jan 03, 2023 at 02:19:18PM +0900, Damien Le Moal wrote:
>> From: Christoph Hellwig <hch@infradead.org>
>>
>> Check that the PREFUSH and FUA flags are only set on write bios,
>> given that the flush state machine expects that.
>>
>> [Damien] The check is also extended to REQ_OP_ZONE_APPEND operations as
>> these are data write operations used by btrfs and zonefs and may also
>> have the REQ_FUA bit set.
>>
>> Reported-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>  block/blk-core.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 9321767470dc..c644aac498ef 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -744,12 +744,16 @@ void submit_bio_noacct(struct bio *bio)
>> 	 * Filter flush bio's early so that bio based drivers without flush
>> 	 * support don't have to worry about them.
>> 	 */
>> -	if (op_is_flush(bio->bi_opf) &&
>> -	    !test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
>> -		bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
>> -		if (!bio_sectors(bio)) {
>> -			status = BLK_STS_OK;
>> +	if (op_is_flush(bio->bi_opf)) {
>> +		if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_WRITE &&
>> +				 bio_op(bio) != REQ_OP_ZONE_APPEND))
>> 			goto end_io;
>> +		if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
>> +			bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
>> +			if (!bio_sectors(bio)) {
>> +				status = BLK_STS_OK;
>> +				goto end_io;
>> +			}
>> 		}
>> 	}
> 
> Hello Damien,
> 
> In a previous email I wrote:
> 
>> It seems that you can have flag WC set, without having flag FUA set.
>>
>> So should perhaps the line:
>>
>>> +             if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
>>
>> instead be:
>>
>> if (!test_bit(QUEUE_FLAG_FUA, &q->queue_flags)) {
> 
> You replied with:
> "Need both. If there is no write cache or write cache is off, FUA is
> implied and is useless.">
> Did you change your mind since then?

I checked the flush machinery code again to be sure and we do not need to
check "if (!test_bit(QUEUE_FLAG_FUA, &q->queue_flags)) {" because this is
exactly what blk-flush.c code will handle: if the device support FUA, the
write is sent as is and if it does not, then the flush machinery sent a
regular write followed by a cache flush command. See the chain:

submit_bio_noacct() -> submit_bio_noacct_nocheck() ->
__submit_bio_noacct[_mq]() -> __submit_bio() -> blk_mq_submit_bio() ->
blk_insert_flush().

Then see blk_insert_flush() handling of the various cases based off the
device features and request.

So that QUEUE_FLAG_FUA test here does not make any sense.

Checking for the no write cache case does make sense though, as in that
case, all writes are FUA. So clearing the FUA & PREFLUSH flags for devices
that do not have write caching is the right thing to do.

> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research