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@xxxxxxxxxxxxx
