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

[Dave Chinner <david@fromorbit.com>]

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

> > > To address this problem, update the shift preparation code to
> > > stabilize the start boundary along with the full range of the
> > > insert. Also update the existing corruption check to fail if any
> > > extent is shifted with a start offset behind the target offset of
> > > the insert range. This prevents insert from racing with COW
> > > writeback completion and fails loudly in the event of an unexpected
> > > extent shift.
> > 
> > It looks ok to avoid this particular symptom (backportable point
> > fix), but I really think we should convert insert/collapse to be
> > atomic w.r.t other extent list modifications....
> > 
> 
> Ok, I think that approach is reasonable so long as we do it in two
> phases as such to minimize backport churn and separate bug fix from
> behavior change.
> 
> Unless there is other feedback on this patch, is there any objection to
> getting this one reviewed/merged independently?

Not here. Seems like the right approach to me. SO for the original
patch:

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com