[PATCH for-6.1 0/2] iopoll bio pcpu cache fix

[PATCH for-6.1 0/2] iopoll bio pcpu cache fix

[Pavel Begunkov]

There are ways to deprive bioset mempool of requests using pcpu caches
and never return them back, which breaks forward progress guarantees bioset
tried to provide. Fix it.

Pavel Begunkov (2):
  mempool: introduce mempool_is_saturated
  bio: don't rob bios from starving bioset

 block/bio.c             | 2 ++
 include/linux/mempool.h | 5 +++++
 2 files changed, 7 insertions(+)

-- 
2.38.0

[PATCH for-6.1 1/2] mempool: introduce mempool_is_saturated

[Pavel Begunkov]

Introduce a helper mempool_is_saturated(), which tells if the mempool is
under-filled or not. We need it to figure out whether it should be
freed right into the mempool or could be cached with top level caches.

Cc: <stable@vger.kernel.org> # 5.15+
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 include/linux/mempool.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/mempool.h b/include/linux/mempool.h
index 0c964ac107c2..4aae6c06c5f2 100644
--- a/include/linux/mempool.h
+++ b/include/linux/mempool.h
@@ -30,6 +30,11 @@ static inline bool mempool_initialized(mempool_t *pool)
 	return pool->elements != NULL;
 }
 
+static inline bool mempool_is_saturated(mempool_t *pool)
+{
+	return READ_ONCE(pool->curr_nr) >= pool->min_nr;
+}
+
 void mempool_exit(mempool_t *pool);
 int mempool_init_node(mempool_t *pool, int min_nr, mempool_alloc_t *alloc_fn,
 		      mempool_free_t *free_fn, void *pool_data,
-- 
2.38.0

[PATCH for-6.1 2/2] bio: don't rob bios from starving bioset

[Pavel Begunkov]

Biosets keep a mempool, so as long as requests complete we can always
can allocate and have forward progress. Percpu bio caches break that
assumptions as we may complete into the cache of one CPU and after try
and fail to allocate with another CPU. We also can't grab from another
CPU's cache without tricky sync.

If we're allocating with a bio while the mempool is undersaturated,
remove REQ_ALLOC_CACHE flag, so on put it will go straight to mempool.
It might try to free into mempool more requests than required, but
assuming than there is no memory starvation in the system it'll
stabilise and never hit that path.

Fixes: be4d234d7aebb ("bio: add allocation cache abstraction")
Cc: <stable@vger.kernel.org> # 5.15+
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/bio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/bio.c b/block/bio.c
index 0a14af923738..e00c93be368a 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -526,6 +526,8 @@ struct bio *bio_alloc_bioset(struct block_device *bdev, unsigned short nr_vecs,
 	}
 	if (unlikely(!p))
 		return NULL;
+	if (!mempool_is_saturated(&bs->bio_pool))
+		opf &= ~REQ_ALLOC_CACHE;
 
 	bio = p + bs->front_pad;
 	if (nr_vecs > BIO_INLINE_VECS) {
-- 
2.38.0

Re: [PATCH for-6.1 0/2] iopoll bio pcpu cache fix

[Jens Axboe]

On 10/27/22 4:14 PM, Pavel Begunkov wrote:
> There are ways to deprive bioset mempool of requests using pcpu caches
> and never return them back, which breaks forward progress guarantees bioset
> tried to provide. Fix it.
> 
> Pavel Begunkov (2):
>   mempool: introduce mempool_is_saturated
>   bio: don't rob bios from starving bioset
> 
>  block/bio.c             | 2 ++
>  include/linux/mempool.h | 5 +++++
>  2 files changed, 7 insertions(+)
> 

This isn't really a concern for 6.1 and earlier, because the caching is
just for polled IO. Polled IO will not be grabbing any of the reserved
inflight units on the mempool side, which is what guarantees the forward
progress.

It'll be a concern for the 6.2 irq based general caching, so it would
need to be handled there. So perhaps this can be a pre-series for a
reposting of that patchset.

-- 
Jens Axboe

Re: [PATCH for-6.1 0/2] iopoll bio pcpu cache fix

[Jens Axboe]

On 10/27/22 5:27 PM, Jens Axboe wrote:
> On 10/27/22 4:14 PM, Pavel Begunkov wrote:
>> There are ways to deprive bioset mempool of requests using pcpu caches
>> and never return them back, which breaks forward progress guarantees bioset
>> tried to provide. Fix it.
>>
>> Pavel Begunkov (2):
>>   mempool: introduce mempool_is_saturated
>>   bio: don't rob bios from starving bioset
>>
>>  block/bio.c             | 2 ++
>>  include/linux/mempool.h | 5 +++++
>>  2 files changed, 7 insertions(+)
>>
> 
> This isn't really a concern for 6.1 and earlier, because the caching is
> just for polled IO. Polled IO will not be grabbing any of the reserved
> inflight units on the mempool side, which is what guarantees the forward
> progress.
> 
> It'll be a concern for the 6.2 irq based general caching, so it would
> need to be handled there. So perhaps this can be a pre-series for a
> reposting of that patchset.

Just a followup, since we had some out-of-band discussion. This is
a potential concern on the bio side, though not on the request side.
But this approach is racy, we'll figure something else out.

-- 
Jens Axboe

Re: [PATCH for-6.1 0/2] iopoll bio pcpu cache fix

[Pavel Begunkov]

On 10/28/22 00:55, Jens Axboe wrote:
> On 10/27/22 5:27 PM, Jens Axboe wrote:
>> On 10/27/22 4:14 PM, Pavel Begunkov wrote:
>>> There are ways to deprive bioset mempool of requests using pcpu caches
>>> and never return them back, which breaks forward progress guarantees bioset
>>> tried to provide. Fix it.
>>>
>>> Pavel Begunkov (2):
>>>    mempool: introduce mempool_is_saturated
>>>    bio: don't rob bios from starving bioset
>>>
>>>   block/bio.c             | 2 ++
>>>   include/linux/mempool.h | 5 +++++
>>>   2 files changed, 7 insertions(+)
>>>
>>
>> This isn't really a concern for 6.1 and earlier, because the caching is
>> just for polled IO. Polled IO will not be grabbing any of the reserved
>> inflight units on the mempool side, which is what guarantees the forward
>> progress.

And after looking it up, apparently it can allocate from reserves.


>> It'll be a concern for the 6.2 irq based general caching, so it would
>> need to be handled there. So perhaps this can be a pre-series for a
>> reposting of that patchset.
> 
> Just a followup, since we had some out-of-band discussion. This is
> a potential concern on the bio side, though not on the request side.
> But this approach is racy, we'll figure something else out.

I agree that it _may_ be, but I can't think of an counter example
for arches w/ strong ordering and would need to think more if there
is an issue in the looser ephemeral world.

-- 
Pavel Begunkov

Re: [PATCH for-6.1 0/2] iopoll bio pcpu cache fix

[Pavel Begunkov]

On 10/28/22 01:13, Pavel Begunkov wrote:
> On 10/28/22 00:55, Jens Axboe wrote:
>> On 10/27/22 5:27 PM, Jens Axboe wrote:
>>> On 10/27/22 4:14 PM, Pavel Begunkov wrote:
>>>> There are ways to deprive bioset mempool of requests using pcpu caches
>>>> and never return them back, which breaks forward progress guarantees bioset
>>>> tried to provide. Fix it.
>>>>
>>>> Pavel Begunkov (2):
>>>>    mempool: introduce mempool_is_saturated
>>>>    bio: don't rob bios from starving bioset
>>>>
>>>>   block/bio.c             | 2 ++
>>>>   include/linux/mempool.h | 5 +++++
>>>>   2 files changed, 7 insertions(+)
>>>>
>>>
>>> This isn't really a concern for 6.1 and earlier, because the caching is
>>> just for polled IO. Polled IO will not be grabbing any of the reserved
>>> inflight units on the mempool side, which is what guarantees the forward
>>> progress.
> 
> And after looking it up, apparently it can allocate from reserves.
> 
> 
>>> It'll be a concern for the 6.2 irq based general caching, so it would
>>> need to be handled there. So perhaps this can be a pre-series for a
>>> reposting of that patchset.
>>
>> Just a followup, since we had some out-of-band discussion. This is
>> a potential concern on the bio side, though not on the request side.
>> But this approach is racy, we'll figure something else out.
> 
> I agree that it _may_ be, but I can't think of an counter example
> for arches w/ strong ordering and would need to think more if there
> is an issue in the looser ephemeral world.

Let me try to prove it's not a problem:

It's not interesting if didn't allocate from reserves as it won't affect
mempool. Neither we care about falsely avoiding pcpu cache, i.e. trying
to return into the mempool more than needed. So, the only potential race
problem that requires explanation is when the bio is from reserves but
we see that the mempool is full and it's not. Because of spinlocks
around ->curr_nr modifications and the fact that we won't see random
invented values, it might have only happened if there were both bio
allocations and bio puts strictly after our allocation, after
spin_unlock to be more specific. But then those puts and allocs are
serialised by the mutex, we can take a bio alloc that happened right
after a put that filled the mempool full and recurse all the reasoning.

I'm rusty on formal proves and it can be more formal, but would be
interesting to see if it's flawed and/or get a counter example.
In any case, it only returns more bios into mempool, which won't hurt
the issue.

-- 
Pavel Begunkov