On Thu, Jan 19, 2023 at 10:33:26AM -0800, Christoph Hellwig wrote: > On Thu, Jan 19, 2023 at 12:03:34PM +1100, Dave Chinner wrote: > > The other place we can be called for a BMBT split without a > > preceeding allocation is __xfs_bunmapi() when punching out the > > center of an existing extent. We don't remove extents in the IO > > path, so these operations don't tend to be called with a lot of > > stack consumed. Hence we don't really need to ship the split off to > > a worker thread in these cases, either. > > So I agree with the fix. But the t_firstblock seems a bit opaque. > We do have a lot of comments, which is good but it still feels > a little fragile to me. Is there a good reason we can't do an > explicit flag to document the intent better? I don't see any point in adding flags to high level allocation APIs that expose deeply internal btree modification implementation details. Caller have no business knowing about this, have no idea how much stack they might need or consume, what context a btree split might occur in, etc. So I don't think there's any reason at all for exposing this btree split offload implementation detail to any other code. As for the "documentation" aspect of the change, see the patchset I just posted for "per-ag centric allocation". That fixes all the existing problems with tp->t_firstblock and also converts it to store an agno (i.e. tp->t_highest_agno) to make it clear that an AG access restriction has been placed on this transaction. That's exactly the situation that this deadlock avoidance needs to be aware of, so I don't see any real need to duplicate this information given the rework of the t-firstblock infrastructure that is out for review. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
