Re: [PATCH] btrfs: keep `priv` struct on stack for sync reads in `btrfs_encoded_read_regular_fill_pages()`

[Johannes Thumshirn <Johannes.Thumshirn@wdc.com>]

On 08.01.25 16:25, Daniel Vacek wrote:
> On Wed, 8 Jan 2025 at 13:42, Johannes Thumshirn
> <Johannes.Thumshirn@wdc.com> wrote:
>>
>> On 08.01.25 12:44, Daniel Vacek wrote:
>>> Only allocate the `priv` struct from slab for asynchronous mode.
>>>
>>> There's no need to allocate an object from slab in the synchronous mode. In
>>> such a case stack can be happily used as it used to be before 68d3b27e05c7
>>> ("btrfs: move priv off stack in btrfs_encoded_read_regular_fill_pages()")
>>> which was a preparation for the async mode.
>>>
>>> While at it, fix the comment to reflect the atomic => refcount change in
>>> d29662695ed7 ("btrfs: fix use-after-free waiting for encoded read endios").
>>
>>
>> Generally I'm not a huge fan of conditional allocation/freeing. It just
>> complicates matters. I get it in case of the bio's bi_inline_vecs where
>> it's a optimization, but I fail to see why it would make a difference in
>> this case.
>>
>> If we're really going down that route, there should at least be a
>> justification other than "no need" to.
> 
> Well the main motivation was not to needlessly exercise the slab
> allocator when IO uring is not used. It is a bit of an overhead,
> though the object is not really big so I guess it's not a big deal
> after all (the slab should manage just fine even under low memory
> conditions).
> 
> 68d3b27e05c7 added the allocation for the async mode but also changed
> the original behavior of the sync mode which was using stack before.
> The async mode indeed requires the allocation as the object's lifetime
> extends over the function's one. The sync mode is perfectly contained
> within as it always was.
> 
> Simply, I tend not to do any allocations which are not strictly
> needed. If you prefer to simply allocate the object unconditionally,
> we can just drop this patch.

At the end of the day it's David's call, he's the maintainer. I'm just 
not sure if skipping the allocator for a small short lived object is 
worth the special casing. Especially as I got bitten by this in the past 
when hunting down kmemleak reports. Conditional allocation is like 
conditional locking, sometimes OK but it raises suspicion.