diff mbox series

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

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

Commit Message

Damien Le Moal Jan. 10, 2023, 1:27 p.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>
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 block/blk-core.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Damien Le Moal Jan. 10, 2023, 9:58 p.m. UTC | #1 
On 1/10/23 22:27, 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>
> Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Christoph, Jens,

Are you OK with this patch ?

I am going to queue up patches 2 to 6 in libata tree as we have another
series on top of these patches (CDL series rebased). Jens, if you could
take this one in the block tree, that would be great.

Thanks !

> ---
>  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;
> +			}
>  		}
>  	}
>
Christoph Hellwig Jan. 11, 2023, 4:27 a.m. UTC | #2 
Yes, I'm ok with the updates to it.
Jens Axboe Jan. 11, 2023, 4:32 a.m. UTC | #3 
On Jan 10, 2023, at 2:58 PM, Damien Le Moal <damien.lemoal@opensource.wdc.com> wrote:
> 
> On 1/10/23 22:27, 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>
>> Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Christoph, Jens,
> 
> Are you OK with this patch ?

I already acked a previous version, you just didn’t pick it up. 

— 
Jens Axboe
Damien Le Moal Jan. 11, 2023, 6:09 a.m. UTC | #4 
On 1/11/23 13:32, Jens Axboe wrote:
> On Jan 10, 2023, at 2:58 PM, Damien Le Moal <damien.lemoal@opensource.wdc.com> wrote:
>>
>> On 1/10/23 22:27, 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>
>>> Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
>>
>> Christoph, Jens,
>>
>> Are you OK with this patch ?
> 
> I already acked a previous version, you just didn’t pick it up. 

I noticed your ack. But since I changed the patch, I wanted confirmation
again. Do you want me to pickup this version through the ATA tree ?
You can take it through the block tree as the block patch can go in
separately. There are no conflicts/dependencies for compile with the ATA
part. Whichever is fine with me.

> 
> — 
> Jens Axboe
>
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;
+			}
 		}
 	}