On Mon, Jan 13, 2025 at 12:00:24PM +0000, Filipe Manana wrote: > On Wed, Dec 18, 2024 at 8:19 PM Qu Wenruo <quwenruo.btrfs@xxxxxxx> wrote: > > > > > > > > 在 2024/12/19 06:39, Darrick J. Wong 写道: > > > On Wed, Dec 18, 2024 at 09:07:26AM +1030, Qu Wenruo wrote: > > >> > > >> > > >> 在 2024/12/18 03:52, Darrick J. Wong 写道: > > >>> On Tue, Dec 17, 2024 at 08:26:33AM +0000, Filipe Manana wrote: > > >>>> On Tue, Dec 17, 2024 at 8:14 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > > >>>>> > > >>>>> On Wed, Dec 11, 2024 at 03:09:40PM +0000, fdmanana@xxxxxxxxxx wrote: > > >>>>>> The test also fails sporadically on xfs and the bug was already reported > > >>>>>> to the xfs mailing list: > > >>>>>> > > >>>>>> https://lore.kernel.org/linux-xfs/CAL3q7H7cURmnkJfUUx44HM3q=xKmqHb80eRdisErD_x8rU4+0Q@xxxxxxxxxxxxxx/ > > >>>>>> > > >>>>> > > >>>>> This version still doesn't seem to have the fs freeze/unfreeze that Darrick > > >>>>> asked for in that thread. > > >>>> > > >>>> I don't get it, what's the freeze/unfreeze for? Where should they be placed? > > >>>> Is it some way to get around the bug on xfs? > > >>> > > >>> freeze kicks the background inode gc thread so that the unlinked clones > > >>> actually get freed before the swapon call. A less bighammer idea might > > >>> be to call XFS_IOC_FREE_EOFBLOCKS which also kicks the garbage > > >>> collectors. > > >> > > >> I'm wondering why this GC things can not be done inside XFS' swapon call? > > >> > > >> So that we don't need some per-fs workaround in a generic test case. > > > > > > I suppose one could call xfs_inodegc_flush from within swapon with the > > > swap file's i_rwsem held, but now we're blocking swapon while we wait > > > for some unbounded number of probably unrelated unlinked inodes to be > > > freed on the off chance that one of them shared blocks. > > > > > > A better answer might be to run FALLOC_FL_UNSHARE on the file, but now > > > we're making swapon more complex and potentially issuing a lot of IO to > > > make that happen. If you can convince the fsdevel/mm folks that swapon > > > is supposed to try to correct things it doesn't like in the file mapping > > > (instead of returning EINVAL or whatever it does now) then we could add > > > that to the syscall definition. > > So how do we proceed from here? > > The btrfs fix was merged into Linus' tree sometime ago (v6.13-rc5), > and I would hate to lose test coverage in fstests. Sure, let's move on :) > > Since the bug seems to be hard to fix on XFS, shall I make the test > _notun on xfs, or move it tests/btrfs? Add "_supported_fs ^xfs" and some comments to explain why skiping XFS. Let's cover the original btrfs bug at first (if there's not a good solution from xfs for now), then deal with the xfs problem later. Thanks, Zorro > Zorro? > > Thanks. > > > > > Sorry that I'm no familiar with XFS to provide any help, but the swapon > > call on btrfs is already very complex. > > > > It needs to verify every extent of that file is not shared, and block > > the subvolume from being snapshotted. > > (The extent shareness check iteslf may already cause quite some IO) > > > > So at least to me, a little more extra logic and IO shouldn't be a huge > > blockage, since we're already doing exactly that since the btrfs > > swapfile support. > > > > Thanks, > > Qu > > > > > > --D > > > > > >> Thanks, > > >> Qu > > >>> > > >>> --D > > >>> > > >> > > >> > > > > > >
