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 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.. > > > > 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@xxxxxxxxxx> > Thanks. FWIW, I whipped up a few patches to hold ilock across insert and collapse yesterday that survive cursory testing. I'll get them on the list after more thorough testing.. Brian > -- > Dave Chinner > david@xxxxxxxxxxxxx >
