On Fri, Sep 24, 2021 at 06:17:52PM +0100, David Howells wrote: > > Hi Willy, Trond, Christoph, > > Here's v3 of a change to make reads and writes from the swapfile use async > DIO, adding a new ->swap_rw() address_space method, rather than readpage() > or direct_IO(), as requested by Willy. This allows NFS to bypass the write > checks that prevent swapfiles from working, plus a bunch of other checks > that may or may not be necessary. > > Whilst trying to make this work, I found that NFS's support for swapfiles > seems to have been non-functional since Aug 2019 (I think), so the first > patch fixes that. Question is: do we actually *want* to keep this > functionality, given that it seems that no one's tested it with an upstream > kernel in the last couple of years? > > There are additional patches to get rid of noop_direct_IO and replace it > with a feature bitmask, to make btrfs, ext4, xfs and raw blockdevs use the > new ->swap_rw method and thence remove the direct BIO submission paths from > swap. > > I kept the IOCB_SWAP flag, using it to enable REQ_SWAP. I'm not sure if > that's necessary, but it seems accounting related. > > The synchronous DIO I/O code on NFS, raw blockdev, ext4 swapfile and xfs > swapfile all seem to work fine. Btrfs refuses to swapon because the file > might be CoW'd. I've tried doing "chattr +C", but that didn't help. Ok, so if the filesystem is doing block mapping in the IO path now, why does the swap file still need to map the file into a private block mapping now? i.e all the work that iomap_swapfile_activate() does for filesystems like XFS and ext4 - it's this completely redundant now that we are doing block mapping during swap file IO via iomap_dio_rw()? Actually, that path does all the "can we use this file as a swap file" checking. So the extent iteration can't go away, just the swap file mapping part (iomap_swapfile_add_extent()). This is necessary to ensure there aren't any holes in the file, and we still need that because the DIO write path will allocate into holes, which leads me to my main concern here. Using the DIO path opens up the possibility that the filesystem could want to run transactions are part of the DIO. Right now we support unwritten extents for swap files (so they don't have to be written to allocate the backing store before activation) and that means we'll be doing DIO to unwritten extents. IO completion of a DIO write to an unwritten extent will run a transaction to convert that extent to written. A similar problem with sparse files exists, because allocation of blocks can be done from the DIO path, and that requires transactions. File extension is another potential transaction path we open up by using DIO writes dor swap. The problem is that a transaction run in swap IO context will will deadlock the filesystem. Either through the unbound memory demand of metadata modification, or from needing log space that can't be freed up because the metadata IO that will free the log space is waiting on memory allocation that is waiting on swap IO... I think some more thought needs to be put into controlling the behaviour/semantics of the DIO path so that it can be safely used by swap IO, because it's not a direct 1:1 behavioural mapping with existing DIO and there are potential deadlock vectors we need to avoid. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
