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

From: Justin Tobler <jltobler@gmail.com>
Date: Sep 16, 2025, 02:55 UTC
Message-ID: <pk2cpihxk4j4ywgq3dtknybyzjeon7ajgmwq4yhknojjsfiqo2@q5dsygszdkar>
In-Reply-To: <aMijGE2CveYcQaWc@nand.local>

On 25/09/15 07:36PM, Taylor Blau wrote:

Show 30 quoted lines

> 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.

Yes, well said. {begin,end}_odbtransaction() operations on inner transactions are effectively a noop. They simple manage an internal counter to know when a new transaction should be started/finished.

Show 5 quoted lines

> 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.

Yes, this patch removes the logic that manages the internal nested transaction counter in favor of requiring callers to check if a transaction has already been started or not.

> (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 [...]").

Yes, you are correct. Apologies for the poor wording.

Show 13 quoted lines

> 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've revisted this patch and I agree there is probably a better/safer way to accoplish this.

The ultimate goal of this patch is make it so invoking end_odb_transaction() on a transaction guarantees it is flushed. With this guarantee we would no longer need flush_odb_transaction() and the transaction interface eventually can be simplified to just {begin,end}_odb_transaction(). This also avoids a potential class of errors where a caller _thinks_ they have committed a transaction and that the objects should be visible, but they actually are not because it was a nested transaction.

The nice thing about the current implementation though is that nested transactions are automatically treated as noops and the caller doesn't have to check if there is already a pending transaction. This is safer and less error-prone.

Thinking about this some more though, we should be able to continue to have {begin,end}_odb_transaction() function as noops when there is already a pending transaction and also be able to drop the internal transaction nesting mechanism at the same time. To do this, instead of erroring out in begin_odb_transaction() when there is already a transaction, we can simply return NULL. If a caller wants to know if a new transaction was actually started, they can just check the return value afterwords. This removes the burden from the caller to explicitly check if there is a pending transaction beforehand. Furthermore, since an ODB only allows a single transaction at a time, it probably makes sense for operations at the ODB layer to guard against this anyway.

This change would pair nicely with the change made to end_odb_transaction() in version 2 of this series. In this version, when end_odb_transaction() is provided a NULL transaction, it now functions as a noop as well. If a transaction _is_ provided though, it is guaranteed to be flushed with addresses the main goal on this patch.

Show 5 quoted lines

> 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 for the thoughtful feedback. Much appreciated. :)

I'll plan on sending a followup version tomorrow that hopefully addresses most of these concerns.

-Justin