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

[David Sterba <dsterba@suse.cz>]

On Wed, Jan 08, 2025 at 06:29:24PM +0000, Johannes Thumshirn wrote:
> 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.

Yeah, in this case it's the uring that makes the allocation/freeing
times different. The "normal" ioctl case does not need it so I think
it's keeping the scope clear while the uring has it's own specialities
like returing to user space with inode lock held
(22d2e48e318564f8c9b09faf03ecb4f03fb44dd5).