Re: [PATCH v17 04/17] netfs: Add some tools for managing bvecq chains

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] netfs: mempool deadlock in bvecq_alloc_chain() when
  for_writeback is true
--

commit e5b9a085f1bcfd322d1044543745cbe0c1ae3076
Author: David Howells <dhowells@redhat.com>

netfs: Add some tools for managing bvecq chains

This commit adds helpers for allocating, populating, refcounting, and
freeing bvecq nodes and chains, along with an emergency mempool for
writeback allocations.

> diff --git a/fs/netfs/bvecq.c b/fs/netfs/bvecq.c
> new file mode 100644
> index 0000000000000..1d4da3de9b9d2
> --- /dev/null
> +++ b/fs/netfs/bvecq.c

[ ... ]

> +struct bvecq *bvecq_alloc_one(size_t nr_slots, gfp_t gfp, bool for_writeback)
> +{
> +	struct bvecq *bq;
> +	size_t size;
> +	bool from_pool = false;
> +
> +	/* Don't overrun ->max_slots or ->nr_slots. */
> +	if (nr_slots > BVECQ_MAX_INLINE_SLOTS)
> +		nr_slots = BVECQ_MAX_INLINE_SLOTS;
> +
> +	size = kmalloc_size_roundup(struct_size(bq, __bv, nr_slots));
> +	gfp &= ~(GFP_ZONEMASK | __GFP_THISNODE);
> +
> +	if (for_writeback) {
> +		if (size != BVECQ_POOL_SIZE) {
> +			gfp_t gfp_temp = gfp;
> +
> +			gfp_temp |= __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_NOWARN;
> +			gfp_temp &= ~(__GFP_DIRECT_RECLAIM | __GFP_IO);
> +			bq = kmalloc(size, gfp_temp);
> +			if (bq)
> +				goto success;
> +		}
> +
> +		bq = mempool_alloc(&netfs_bvecq_pool, gfp);
> +		if (!bq)
> +			return bq;
> +		from_pool = true;
> +		size = BVECQ_POOL_SIZE;
> +	} else {
> +		bq = kmalloc(size, gfp);
> +		if (!bq)
> +			return bq;
> +	}
> +
> +success:
> +	*bq = (struct bvecq) {
> +		.ref		= REFCOUNT_INIT(1),
> +		.bv		= bq->__bv,
> +		.inline_bv	= true,
> +		.max_slots	= min((size - sizeof(*bq)) / sizeof(bq->__bv[0]),
> +				      BVECQ_MAX_INLINE_SLOTS),
> +		.from_pool	= from_pool,
> +	};
> +	netfs_stat(&netfs_n_bvecq);
> +	return bq;
> +}
> +EXPORT_SYMBOL(bvecq_alloc_one);
> +
> +/**
> + * bvecq_alloc_chain - Allocate an unpopulated bvecq chain
> + * @nr_slots: Number of slots to allocate
> + * @gfp: The allocation constraints.
> + * @for_writeback: True if allocating for writeback
> + *
> + * Allocate a chain of bvecq nodes providing at least the requested cumulative
> + * number of slots.  Each node is a maximum of 4KiB in size.
> + *
> + * Return: The first node pointer or NULL on allocation failure.
> + */
> +struct bvecq *bvecq_alloc_chain(size_t nr_slots, gfp_t gfp, bool for_writeback)
> +{
> +	struct bvecq *head = NULL, *tail = NULL;
> +
> +	_enter("%zu", nr_slots);
> +
> +	for (;;) {
> +		struct bvecq *bq;
> +
> +		bq = bvecq_alloc_one(min(nr_slots, BVECQ_4KB_SLOTS), gfp, for_writeback);
> +		if (!bq)
> +			goto oom;
> +
> +		if (tail)
> +			bvecq_append(tail, bq);
> +		else
> +			head = bq;
> +		tail = bq;
> +		if (tail->max_slots >= nr_slots)
> +			break;
> +		nr_slots -= tail->max_slots;
> +	}
> +
> +	return head;
> +oom:
> +	bvecq_put(head);
> +	return NULL;
> +}

[Severity: High]
Can bvecq_alloc_chain() deadlock on netfs_bvecq_pool when for_writeback is
true and gfp permits direct reclaim?

Under memory pressure, bvecq_alloc_one() falls back to
mempool_alloc(&netfs_bvecq_pool, gfp), which yields a 512-byte node holding
BVECQ_POOL_SLOTS (29 slots on 64-bit).  bvecq_alloc_chain() loops calling
bvecq_alloc_one() while holding previously allocated pool elements in head.

Because netfs_bvecq_pool has 100 elements, a single chain requesting more
than 2900 slots (~11.3 MiB of 4 KiB pages) or concurrent writeback threads
building chains at the same time can exhaust the pool midway through the
loop.  Each thread then sleeps in mempool_alloc() waiting for elements to
be freed while pinning the elements already attached to head, before any
I/O can be submitted to release them.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111203034.2126314-1-dhowells@redhat.com?part=4