diff mbox series

[4/6] block: clear BIO_PERCPU_CACHE flag if polling isn't supported

Message ID 20210812154149.1061502-5-axboe@kernel.dk (mailing list archive)
State New, archived
Headers show
Series Enable bio recycling for polled IO | expand

Commit Message

Jens Axboe Aug. 12, 2021, 3:41 p.m. UTC 
The bio alloc cache relies on the fact that a polled bio will complete
in process context, clear the cacheable flag if we disable polling
for a given bio.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Christoph Hellwig Aug. 12, 2021, 4:33 p.m. UTC | #1 
On Thu, Aug 12, 2021 at 09:41:47AM -0600, Jens Axboe wrote:
> The bio alloc cache relies on the fact that a polled bio will complete
> in process context, clear the cacheable flag if we disable polling
> for a given bio.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Keith Busch Aug. 12, 2021, 5:31 p.m. UTC | #2 
On Thu, Aug 12, 2021 at 09:41:47AM -0600, Jens Axboe wrote:
> -	if (!test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
> +	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;
> +	}

It looks like you should also clear BIO_PERCPU_CACHE if this bio gets
split in blk_bio_segment_split().
Jens Axboe Aug. 12, 2021, 5:41 p.m. UTC | #3 
On 8/12/21 11:31 AM, Keith Busch wrote:
> On Thu, Aug 12, 2021 at 09:41:47AM -0600, Jens Axboe wrote:
>> -	if (!test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
>> +	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;
>> +	}
> 
> It looks like you should also clear BIO_PERCPU_CACHE if this bio gets
> split in blk_bio_segment_split().

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);
 
 	switch (bio_op(bio)) {
 	case REQ_OP_DISCARD:
diff --git a/block/blk-merge.c b/block/blk-merge.c
index f8707ff7e2fc..985ca1116c32 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -285,7 +285,7 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
 	 * iopoll in direct IO routine. Given performance gain of iopoll for
 	 * big IO can be trival, disable iopoll when split needed.
 	 */
-	bio->bi_opf &= ~REQ_HIPRI;
+	bio_clear_hipri(bio);
 
 	return bio_split(bio, sectors, GFP_NOIO, bs);
 }
diff --git a/block/blk.h b/block/blk.h
index db6f82bbb683..7dba254b45f2 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -367,4 +367,11 @@ extern struct device_attribute dev_attr_events;
 extern struct device_attribute dev_attr_events_async;
 extern struct device_attribute dev_attr_events_poll_msecs;
 
+static inline void bio_clear_hipri(struct bio *bio)
+{
+	/* can't support alloc cache if we turn off polling */
+	bio_clear_flag(bio, BIO_PERCPU_CACHE);
+	bio->bi_opf &= ~REQ_HIPRI;
+}
+
 #endif /* BLK_INTERNAL_H */
Keith Busch Aug. 13, 2021, 8:17 p.m. UTC | #4 
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.
Jens Axboe Aug. 13, 2021, 8:19 p.m. UTC | #5 
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...
Keith Busch Aug. 13, 2021, 8:31 p.m. UTC | #6 
On Fri, Aug 13, 2021 at 02:19:11PM -0600, Jens Axboe wrote:
> 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...

Yeah, it is a coding error if that happened, so a WARN sounds okay. I
also don't like adding these kinds of checks, so please feel free to not
include it if you think the usage is clear enough.
Jens Axboe Aug. 13, 2021, 8:33 p.m. UTC | #7 
On 8/13/21 2:31 PM, Keith Busch wrote:
> On Fri, Aug 13, 2021 at 02:19:11PM -0600, Jens Axboe wrote:
>> 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...
> 
> Yeah, it is a coding error if that happened, so a WARN sounds okay. I
> also don't like adding these kinds of checks, so please feel free to not
> include it if you think the usage is clear enough.

Just have to watch for new additions of IOCB_ALLOC_CACHE, which
thankfully shouldn't be too bad.
diff mbox series

Patch

diff --git a/block/blk-core.c b/block/blk-core.c
index 04477697ee4b..c130206e9961 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -833,8 +833,11 @@  static noinline_for_stack bool submit_bio_checks(struct bio *bio)
 		}
 	}
 
-	if (!test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
+	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;
+	}
 
 	switch (bio_op(bio)) {
 	case REQ_OP_DISCARD: