Re: [PATCH 1/6] bulk-checkin: remove ODB transaction nesting

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 15, 2025, 23:36 UTC
Message-ID: <aMijGE2CveYcQaWc@nand.local>
In-Reply-To: <aMJu4yoO5-Xp52oJ@pks.im>

On Thu, Sep 11, 2025 at 08:40:35AM +0200, Patrick Steinhardt wrote:

Show 16 quoted lines

> On Tue, Sep 09, 2025 at 02:11:29PM -0500, Justin Tobler wrote:
> > ODB transactions support being nested. Only the outermost
> > {begin,end}_odb_transaction() start and finish a transaction. This is
> > done so that certain object write codepaths that occur internally can be
> > optimized via ODB transactions without having to worry if a transaction
> > has already been started or not. This can make the interface a bit
> > awkward to use, as calling {begin,end}_odb_transaction() does not
> > guarantee that a transaction is actually started or ended.
> >
> > Instead, be more explicit and require callers who use ODB transactions
> > internally to ensure there is not already a pending transaction before
> > beginning or ending a transaction.
>
> I think one bit missing in the commit message is to explain what this
> buys us. Does it for example enable subsequent changes? Or is this
> really only done to have clean ownership semantics for the transaction?

In addition, it would be useful to hear from the commit message *why* this is safe to do. Justin's message suggests that nested transactions are noops, so doing something like:

  begin_odb_transaction();
    begin_odb_transaction();
      write_object();
    end_odb_transaction(); <- object not yet added to the main ODB
  end_odb_transaction();   <- now it is

only results in the object being added to the main ODB when the final end_odb_transaction() is called.

Instead it looks like this patch pushes us towards having callers check whether or not there is a transaction in progress before starting a new one. So it seems like this is safe to do only for callers that check whether or not there is an ongoing transaction before beginning a new one.

(I think this is what the second paragraph of the quoted part is trying to say, but I think it may be clearer to say "To preserve the same semantics, callers MUST ensure there is not [...]").

That's more work for callers, and at first blush feels a little more error-prone.

Specifically, if some new piece of code is written that does not first check whether there is an ongoing transaction, it could result in a BUG() either at the time it is written, or worse, later on when that function is called in the context of an outer transaction.

So I am not sure whether this patch is making things simpler or safer. Certainly the bulk-checkin API is a little simpler, since we no longer have to keep track of the nesting level within an odb_transaction. But I think it pushes more burden onto the callers in a way that I worry could create the potential for BUG()s later on.

I think that takes us back to Patrick's question: what do we gain by simplifying the internals of the bulk-checkin API, and how does (or doesn't) that justify the added burden on callers? Looking at the newer version of this patch in [1], I see that you addressed what we gain, but I am still curious about how we justify the added cost.

Thanks, Taylor

[1]: https://lore.kernel.org/git/20250915202956.3784935-2-jltobler@gmail.com/