On Sun, Mar 31, 2024 at 08:15:49AM +1100, Dave Chinner wrote: > On Fri, Mar 29, 2024 at 02:35:20PM -0700, Darrick J. Wong wrote: > > On Wed, Mar 27, 2024 at 04:35:12AM -0700, Christoph Hellwig wrote: > > > On Tue, Mar 26, 2024 at 07:07:58PM -0700, Darrick J. Wong wrote: > > > > periodically to allow other threads to do work. This implementation > > > > avoids the worst problems of the original code, though it lacks the > > > > desirable attribute of freeing the biggest chunks first. > > > > > > Do we really care much about freeing larger area first? I don't think > > > it really matters for FITRIM at all. > > > > > > In other words, I suspect we're better off with only the by-bno > > > implementation. > > > > Welll... you could argue that if the underlying "thin provisioning" is > > actually just an xfs file, that punching tons of tiny holes in that file > > could increase the height of the bmbt. In that case, you'd want to > > reduce the chances of the punch failing with ENOSPC by punching out > > larger ranges to free up more blocks. > > That's true, but it's a consideration for dm-thinp, too. I've always > considered FITRIM as an optimisation for dm-thinp systems (rather > than a hardware device optimisation), and so "large extents first" > make a whole lot more sense from that perspective. > > That said, there's another reason for by-size searches being the > default behaviour: FITRIM has a minimum length control parameter. > Hence for fragmented filesystems where there are few extents larger > than the minimum length, we needed to avoid doing an exhaustive > search of the by-bno tree to find extents longer than the minimum > length.... Ooooh, that's a /very/ good point. xfs_scrub also now constructs a histogram of free space extent lengths to see if it can reduce the runtime of phase 8 (FITRIM) by setting minlen to something large enough to reduce runtime by 4-5x while not missing more than a few percent of the free space. --D
