From: Taylor Blau Date: Mon, 15 Sep 2025 23:36:56 GMT Subject: Re: [PATCH 1/6] bulk-checkin: remove ODB transaction nesting Message-ID: In-Reply-To: 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/