On 12/30/22 20:54, Niklas Cassel wrote:
> On Tue, Nov 08, 2022 at 02:55:38PM +0900, Damien Le Moal wrote:
>> From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
>>
>> 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>
>> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
>> Reviewed-by: Chaitanya Kulkarni <kch@xxxxxxxxxx>
>> Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
>> ---
>> 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 17667159482e..d3446d38ba77 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -730,12 +730,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)) {
>
> Considering that blk_queue_write_cache() looks like this:
>
> void blk_queue_write_cache(struct request_queue *q, bool wc, bool fua)
> {
> if (wc)
> blk_queue_flag_set(QUEUE_FLAG_WC, q);
> else
> blk_queue_flag_clear(QUEUE_FLAG_WC, q);
> if (fua)
> blk_queue_flag_set(QUEUE_FLAG_FUA, q);
> else
> blk_queue_flag_clear(QUEUE_FLAG_FUA, q);
>
> wbt_set_write_cache(q, test_bit(QUEUE_FLAG_WC, &q->queue_flags));
> }
>
> 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)) {
Need both. If there is no write cache or write cache is off, FUA is
implied and is useless.
>
>
> Kind regards,
> Niklas
--
Damien Le Moal
Western Digital Research
