[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAPjX3FdEZVHW-atYk3ufRj5=6TiiSvuL+=gNobv2biPJ0wuf=w@mail.gmail.com>
Date: Thu, 9 Jan 2025 09:41:51 +0100
From: Daniel Vacek <neelx@...e.com>
To: Johannes Thumshirn <Johannes.Thumshirn@....com>
Cc: Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>,
"linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] btrfs: keep `priv` struct on stack for sync reads in `btrfs_encoded_read_regular_fill_pages()`
On Wed, 8 Jan 2025 at 19:29, Johannes Thumshirn
<Johannes.Thumshirn@....com> wrote:
>
> On 08.01.25 16:25, Daniel Vacek wrote:
> > On Wed, 8 Jan 2025 at 13:42, Johannes Thumshirn
> > <Johannes.Thumshirn@....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.
Well in this case the scope is really limited just to those two
functions which you can fit on the screen. It looks quite readable to
me.
And the async/iouring case is already treated conditionally here.
In the end this patch only makes the sync case behave as it was before
introducing the async changes.
--nX
Powered by blists - more mailing lists