On Sun, Sep 26, 2021 at 04:10:43AM +0100, Matthew Wilcox wrote: > On Sun, Sep 26, 2021 at 09:42:43AM +1000, Dave Chinner wrote: > > 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()? > > Hi Dave, > > Thanks for bringing up all these points. I think they all deserve to go > into the documentation as "things to consider" for people implementing > ->swap_rw for their filesystem. > > Something I don't think David perhaps made sufficiently clear is that > regular DIO from userspace gets handled by ->read_iter and ->write_iter. > This ->swap_rw op is used exclusive for, as the name suggests, swap DIO. > So filesystems don't have to handle swap DIO and regular DIO the same > way, and can split the allocation work between ->swap_activate and the > iomap callback as they see fit (as long as they can guarantee the lack > of deadlocks under memory pressure). I understand this completely. The point is that the implementation of ->swap_rw is to call iomap_dio_rw() with the same ops as the normal DIO read/write path uses. IOWs, apart from the IOCB_SWAP flag, there is no practical difference between the "swap DIO" and "normal DIO" I/O paths. > There are several advantages to using the DIO infrastructure for > swap: > > - unify block & net swap paths > - allow filesystems to _see_ swap IOs instead of being bypassed > - get rid of the swap extent rbtree > - allow writing compound pages to swap files instead of splitting > them > - allow ->readpage to be synchronous for better error reporting > - remove page_file_mapping() and page_file_offset() > > I suspect there are several problems with this patchset, but I'm not > likely to have a chance to read it closely for a few days. If you > have time to give the XFS parts a good look, that would be fantastic. That's what I've already done, and all the questions I've raised are from asking a simple question: what happens if a transaction is required to complete the iomap_dio_rw() swap write operation? I mean, this is similar to the problems with IOCB_NOWAIT - we're supposed to return -EAGAIN if we might block during IO submission, and one of those situations we have to consider is "do we need to run a transaction". If we get it wrong (and we do!), then the worst thing that happens is that there is a long latency for IO submission. It's a minor performance issue, not the end of the world. The difference with IOCB_SWAP is that "don't do transactions during iomap_dio_rw()" is a _hard requirement_ on both IO submission and completion. That means, from now and forever, we will have to guarantee a path through iomap_dio_rw() that will never run transactions on an IO. That requirement needs to be enforced in every block mapping callback into each filesystem, as this is something the iomap infrastructure cannot enforce. Hence we'll have to plumb IOCB_SWAP into a new IOMAP_SWAP iterator flag to pass to the ->iomap_begin() DIO methods to ensure they do the right thing. And then the question becomes: what happens if the filesystem cannot do the right thing? Can the swap code handle an error? e.g. the first thing that xfs_direct_write_iomap_begin() and xfs_read_iomap_begin() do is check if the filesystem is shut down and returns -EIO in that case. IOWs, we've now got normal filesystem "reject all IO" corruption protection mechanisms in play. Using iomap_dio_rw() as it stands means that _all swapfile IO will fail_ if the filesystem shuts down. Right now the swap file IO can keep going blissfully unaware of the filesystem failure status. The open swapfile will prevent the filesystem from being unmounted. Hence to unmount the shutdown filesystem to correct the problem, first the swap file has to be turned off, which means we have a fail-safe behaviour. Using the iomap_dio_rw() path means that swapfile IO _can and will fail_. AFAICT, swap IO errors are pretty much thrown away by the mm code; the swap_writepage() return value is ignored or placed on the swap cache address space and ignored. And it looks like the new read path just sets PageError() and leaves it to callers to detect and deal with a swapin failure because swap_readpage() is now void... So it seems like there's a whole new set of failure cases using the DIO path introduces into the swap IO path that haven't been considered here. I can't see why we wouldn't be able to solve them, but these considerations lead me to think that use of the DIO is based on an incorrect assumption - DIO is not a "simple low level IO" interface. Hence I suspect that we'd be much better off with a new iomap_swap_rw() implementation that just does what swap needs without any of the complexity of the DIO API. Internally iomap can share what it needs to share with the DIO path, but at this point I'm not sure we should be overloading the iomap_dio_rw() path with the semantics required by swap. e.g. we limit iomap_swap_rw() to only accept written or unwritten block mappings within file size on inodes with clean metadata (i.e. pure overwrite to guarantee no modification transactions), and then the fs provided ->iomap_begin callback can ignore shutdown state, elide inode level locking, do read-only mappings, etc without adding extra overhead to the existing DIO code path... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
