On 8/13/21 2:17 PM, Keith Busch wrote:
> On Thu, Aug 12, 2021 at 11:41:58AM -0600, Jens Axboe wrote:
>> Indeed. Wonder if we should make that a small helper, as any clear of
>> REQ_HIPRI should clear BIO_PERCPU_CACHE as well.
>>
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 7e852242f4cc..d2722ecd4d9b 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -821,11 +821,8 @@ static noinline_for_stack bool submit_bio_checks(struct bio *bio)
>> }
>> }
>>
>> - if (!test_bit(QUEUE_FLAG_POLL, &q->queue_flags)) {
>> - /* can't support alloc cache if we turn off polling */
>> - bio_clear_flag(bio, BIO_PERCPU_CACHE);
>> - bio->bi_opf &= ~REQ_HIPRI;
>> - }
>> + if (!test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
>> + bio_clear_hipri(bio);
>
> Since BIO_PERCPU_CACHE doesn't work without REQ_HIRPI, should this check
> look more like this?
>
> if (!test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
> bio->bi_opf &= ~REQ_HIPRI;
> if (!(bio->bi_opf & REQ_HIPRI))
> bio_clear_flag(bio, BIO_PERCPU_CACHE);
>
> I realise the only BIO_PERCPU_CACHE user in this series never sets it
> without REQ_HIPRI, but it looks like a problem waiting to happen if
> nothing enforces this pairing: someone could set the CACHE flag on a
> QUEUE_FLAG_POLL enabled queue without setting HIPRI and get the wrong
> bio_put() action.
I'd rather turn that into a WARN_ON or similar. But probably better to
do that on the freeing side, honestly. That'll be the most reliable way,
but a shame to add cycles to the hot path...
--
Jens Axboe
