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

[Taylor Blau <me@ttaylorr.com>]

On Thu, Sep 11, 2025 at 08:40:35AM +0200, Patrick Steinhardt wrote:
> 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/