diff mbox series

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

Message ID 20230103051924.233796-2-damien.lemoal@opensource.wdc.com
State New
Headers show
Series Improve libata support for FUA | expand

Commit Message

Damien Le Moal Jan. 3, 2023, 5:19 a.m. UTC 
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(-)

Comments

Niklas Cassel Jan. 3, 2023, 8:05 a.m. UTC | #1 
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?


Kind regards,
Niklas
Damien Le Moal Jan. 3, 2023, 12:46 p.m. UTC | #2 
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
Niklas Cassel Jan. 3, 2023, 1:02 p.m. UTC | #3 
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;
> +			}
>  		}
>  	}
>  
> -- 
> 2.39.0
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
Ming Lei Jan. 4, 2023, 2:23 p.m. UTC | #4 
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.

flush state machine is only for request based driver, but FUA is
used by bio drivers(dm, md, ...) too, just wondering if bio drivers
are covered for this change? If yes, can you add words in the
commit log?


Thanks,
Ming
Damien Le Moal Jan. 5, 2023, 3:54 a.m. UTC | #5 
On 1/4/23 23:23, Ming Lei 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.
> 
> flush state machine is only for request based driver, but FUA is
> used by bio drivers(dm, md, ...) too, just wondering if bio drivers
> are covered for this change? If yes, can you add words in the
> commit log?

I think they are since it already was the responsibility of
dm_submit_bio() and md_submit_bio() to handle PREFLUSH and FUA.

> 
> 
> Thanks,
> Ming
>
diff mbox series

Patch

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;
+			}
 		}
 	}