Re: [RESEND PATCH v9 00/14] io-uring/xfs: support async buffered writes

Darrick J. Wong
2022-06-23T20:31:18Z

On Thu, Jun 23, 2022 at 10:51:43AM -0700, Stefan Roesch wrote:
> This patch series adds support for async buffered writes when using both
> xfs and io-uring. Currently io-uring only supports buffered writes in the
> slow path, by processing them in the io workers. With this patch series it is
> now possible to support buffered writes in the fast path. To be able to use
> the fast path the required pages must be in the page cache, the required locks
> in xfs can be granted immediately and no additional blocks need to be read
> form disk.
>
> Updating the inode can take time. An optimization has been implemented for
> the time update. Time updates will be processed in the slow path. While there
> is already a time update in process, other write requests for the same file,
> can skip the update of the modification time.
>
>
> Performance results:
> For fio the following results have been obtained with a queue depth of
> 1 and 4k block size (runtime 600 secs):
>
> sequential writes:
> without patch with patch libaio psync
> iops: 77k 209k 195K 233K
> bw: 314MB/s 854MB/s 790MB/s 953MB/s
> clat: 9600ns 120ns 540ns 3000ns

Hey, nice!

>
>
> For an io depth of 1, the new patch improves throughput by over three times
> (compared to the exiting behavior, where buffered writes are processed by an
> io-worker process) and also the latency is considerably reduced. To achieve the
> same or better performance with the exisiting code an io depth of 4 is required.
> Increasing the iodepth further does not lead to improvements.
>
> In addition the latency of buffered write operations is reduced considerably.
>
>
>
> Support for async buffered writes:
>
> To support async buffered writes the flag FMODE_BUF_WASYNC is introduced. In
> addition the check in generic_write_checks is modified to allow for async
> buffered writes that have this flag set.
>
> Changes to the iomap page create function to allow the caller to specify
> the gfp flags. Sets the IOMAP_NOWAIT flag in iomap if IOCB_NOWAIT has been set
> and specifies the requested gfp flags.
>
> Adds the iomap async buffered write support to the xfs iomap layer.
> Adds async buffered write support to the xfs iomap layer.
>
> Support for async buffered write support and inode time modification
>
> Splits the functions for checking if the file privileges need to be removed in
> two functions: check function and a function for the removal of file privileges.
> The same split is also done for the function to update the file modification time.
>
> Implement an optimization that while a file modification time is pending other
> requests for the same file don't need to wait for the file modification update.
> This avoids that a considerable number of buffered async write requests get
> punted.
>
> Take the ilock in nowait mode if async buffered writes are enabled and enable
> the async buffered writes optimization in io_uring.
>
> Support for write throttling of async buffered writes:
>
> Add a no_wait parameter to the exisiting balance_dirty_pages() function. The
> function will return -EAGAIN if the parameter is true and write throttling is
> required.
>
> Add a new function called balance_dirty_pages_ratelimited_async() that will be
> invoked from iomap_write_iter() if an async buffered write is requested.
>
> Enable async buffered write support in xfs
> This enables async buffered writes for xfs.
>
>
> Testing:
> This patch has been tested with xfstests, fsx, fio and individual test programs.

Good to hear. Will there be some new fstest coming/already merged?

<snip>

Hmm, well, vger and lore are still having stomach problems, so even the
resend didn't result in #5 ending up in my mailbox. :(

For the patches I haven't received, I'll just attach my replies as
comments /after/ each patch subject line. What a way to review code!

> Jan Kara (3):
> mm: Move starting of background writeback into the main balancing loop
> mm: Move updates of dirty_exceeded into one place
> mm: Add balance_dirty_pages_ratelimited_flags() function

(Yeah, I guess these changes make sense...)

> Stefan Roesch (11):
> iomap: Add flags parameter to iomap_page_create()
> iomap: Add async buffered write support

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

> iomap: Return -EAGAIN from iomap_write_iter()
> fs: Add check for async buffered writes to generic_write_checks
> fs: add __remove_file_privs() with flags parameter
> fs: Split off inode_needs_update_time and __file_update_time
> fs: Add async write file modification handling.

The commit message references a file_modified_async function, but all I
see is file_modified_flags? Assuming that's just a clerical error,

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

> io_uring: Add support for async buffered writes

Hm, ok, so the EAGAINs that we sprinkle everywhere get turned into short
writes at the end of iomap_file_buffered_write, and that's what this
picks up? If so, then...

> io_uring: Add tracepoint for short writes
> xfs: Specify lockmode when calling xfs_ilock_for_iomap()
> xfs: Add async buffered write support

...I guess I'm ok with signing off on the last patch:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>
> fs/inode.c | 168 +++++++++++++++++++++++---------
> fs/io_uring.c | 32 +++++-
> fs/iomap/buffered-io.c | 71 +++++++++++---
> fs/read_write.c | 4 +-
> fs/xfs/xfs_file.c | 11 +--
> fs/xfs/xfs_iomap.c | 11 ++-
> include/linux/fs.h | 4 +
> include/linux/writeback.h | 7 ++
> include/trace/events/io_uring.h | 25 +++++
> mm/page-writeback.c | 89 +++++++++++------
> 10 files changed, 314 insertions(+), 108 deletions(-)
>
>
> base-commit: b13baccc3850ca8b8cccbf8ed9912dbaa0fdf7f3
> --
> 2.30.2
>