On 2022/11/02 16:09, Christoph Hellwig wrote:
> On Wed, Nov 02, 2022 at 07:05:35AM +0900, Damien Le Moal wrote:
>>>> + if (!op_is_write(rq->cmd_flags) && (rq->cmd_flags & REQ_FUA)) {
>>>> + blk_mq_end_request(rq, BLK_STS_NOTSUPP);
>>>
>>> How could this even happen? If we want a debug check, I think it
>>> should be in submit_bio and a WARN_ON_ONCE.
>>
>> I have not found any code that issues a FUA read. So I do not think this
>> can happen at all currently. The check is about making sure that it
>> *never* happens.
>>
>> I thought of having the check higher up in the submit path but I wanted to
>> avoid adding yet another check in the very hot path. But if you are OK
>> with that, I will move it.
>
> I'd do something like this:
>
> ---
> From 96847cce848938d1ee368e609ccb28a19854fba3 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@xxxxxx>
> Date: Wed, 2 Nov 2022 08:05:41 +0100
> Subject: block: add a sanity check for non-write flush/fua bios
>
> Check that the PREFUSH and FUA flags are only set on write bios,
> given that the flush state machine expects that.
>
> Reported-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> block/blk-core.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index e9e2bf15cd909..4e2b01a53c6ab 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -720,12 +720,15 @@ 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))
> 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;
> + }
> }
> }
>
Indeed looks nicer. I will send a v5 with this.
--
Damien Le Moal
Western Digital Research
