Re: [PATCH] xfs: stabilize insert range start boundary to avoid COW writeback race

[Dave Chinner <david@fromorbit.com>]

On Thu, Dec 12, 2019 at 09:16:34AM -0500, Brian Foster wrote:
> On Thu, Dec 12, 2019 at 07:52:30AM +1100, Dave Chinner wrote:
> > On Wed, Dec 11, 2019 at 07:47:12AM -0500, Brian Foster wrote:
> > > On Wed, Dec 11, 2019 at 08:41:00AM +1100, Dave Chinner wrote:
> > > > On Tue, Dec 10, 2019 at 08:23:40AM -0500, Brian Foster wrote:
> > > > I think insert/collapse need to be converted to work like a
> > > > truncate operation instead of a series on individual write
> > > > operations. That is, they are a permanent transaction that locks the
> > > > inode once and is rolled repeatedly until the entire extent listi
> > > > modification is done and then the inode is unlocked.
> > > > 
> > > 
> > > Note that I don't think it's sufficient to hold the inode locked only
> > > across the shift. For the insert case, I think we'd need to grab it
> > > before the extent split at the target offset and roll from there.
> > > Otherwise the same problem could be reintroduced if we eventually
> > > replaced the xfs_prepare_shift() tweak made by this patch. Of course,
> > > that doesn't look like a big problem. The locking is already elevated
> > > and split and shift even use the same transaction type, so it's mostly a
> > > refactor from a complexity standpoint. 
> > 
> > *nod*
> > 
> > > For the collapse case, we do have a per-shift quota reservation for some
> > > reason. If that is required, we'd have to somehow replace it with a
> > > worst case calculation. That said, it's not clear to me why that
> > > reservation even exists.
> > 
> > I'm not 100% sure, either, but....
> > 
> > > The pre-shift hole punch is already a separate
> > > transaction with its own such reservation. The shift can merge extents
> > > after that point (though most likely only on the first shift), but that
> > > would only ever remove extent records. Any thoughts or objections if I
> > > just killed that off?
> > 
> > Yeah, I suspect that it is the xfs_bmse_merge() case freeing blocks
> > the reservation is for, and I agree that it should only happen on
> > the first shift because all the others that are moved are identical
> > in size and shape and would have otherwise been merged at creation.
> > 
> > Hence I think we can probably kill the xfs_bmse_merge() case,
> > though it might be wrth checking first how often it gets called...
> > 
> 
> Ok, but do we need an up-front quota reservation for freeing blocks out
> of the bmapbt? ISTM the reservation could be removed regardless of the
> merging behavior. This is what my current patch does, at least, so we'll
> see if anything explodes. :P

xfs_itruncate_extents() doesn't require an up front block
reservation for quotas or transaction allocation, so I don't really
see how the collapse would require it, even in the merge case...

> I agree on the xfs_bmse_merge() bit. I was planning to leave that as is
> however because IIRC, even though it is quite rare, I thought we had a
> few corner cases where it was possible for physically and logically
> contiguous extents to track separately in a mapping tree. Max sized
> extents that are subsequently punched out or truncated might be one
> example. I thought we had others, but I can't quite put my finger on it
> atm..

True, but is it common enough that we really need to care about it?
If it's bad, just run xfs_fsr on the file/filesystem....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com