Skip to contentSkip to conversation

io-uring/xfs: support async buffered writes

PATCH RESEND Applied Stefan Roesch 8 participants 33 messages Jun 23, 2022 – Mar 6, 2023

Subsystems (7)
Memory management VFS Tracing XFS filesystem +3 more subsystems
lore
Share & tools
lore

Stefan Roesch

author
#0 lore raw

v9 cover io-uring/xfs: support async buffered writes

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

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.

Changes:
V9:
- Added comment for function balance_dirty_pages_ratelimited_flags()
- checking return code for iop allocation in iomap_page_create()

V8:
- Reverted back changes to iomap_write_iter and used Mathew Wilcox code review recommendation with an additional change to revert the iterator.
- Removed patch "fs: Optimization for concurrent file time updates"
- Setting flag value in file_modified_flags()
- Removed additional spaces in comment in file_update_time()
- Run fsx with 1 billion ops against the changes (Run passed)

V7:
- Change definition and if clause in " iomap: Add flags parameter to iomap_page_create()"
- Added patch "iomap: Return error code from iomap_write_iter()" to address the problem Dave Chinner brought up: retrying memory allocation a second time when we are under memory pressure.
- Removed patch "xfs: Change function signature of xfs_ilock_iocb()"
- Merged patch "xfs: Enable async buffered write support" with previous patch

V6:
- Pass in iter->flags to calls in iomap_page_create()

V5:
- Refreshed to 5.19-rc1
- Merged patch 3 and patch 4
"mm: Prepare balance_dirty_pages() for async buffered writes" and "mm: Add balance_dirty_pages_ratelimited_flags() function"
- Reformatting long file in iomap_page_create()
- Replacing gfp parameter with flags parameter in iomap_page_create() This makes sure that the gfp setting is done in one location.
- Moved variable definition outside of loop in iomap_write_iter()
- Merged patch 7 with patch 6.
- Introduced __file_remove_privs() that get the iocb_flags passed in as an additional parameter
- Removed file_needs_remove_privs() function
- Renamed file_needs_update_time() inode_needs_update_time()
- inode_needs_update_time() no longer passes the file pointer
- Renamed file_modified_async() to file_modified_flags()
- Made file_modified_flags() an internal function
- Removed extern keyword in file_modified_async definition
- Added kiocb_modified function.
- Separate patch for changes to xfs_ilock_for_iomap()
- Separate patch for changes to xfs_ilock_inode()
- Renamed xfs_ilock_xfs_inode()n back to xfs_ilock_iocb()
- Renamed flags parameter to iocb_flags in function xfs_ilock_iocb()
- Used inode_set_flags() to manipulate inode flags in the function file_modified_flags()

V4:
- Reformat new code in generic_write_checks_count().
- Removed patch that introduced new function iomap_page_create_gfp().
- Add gfp parameter to iomap_page_create() and change all callers All users will enforce the number of blocks check
- Removed confusing statement in iomap async buffer support patch
- Replace no_wait variable in __iomap_write_begin with check of IOMAP_NOWAIT for easier readability.
- Moved else if clause in __iomap_write_begin into else clause for easier readability
- Removed the balance_dirty_pages_ratelimited_async() function and reverted back to the earlier version that used the function balance_dirty_pages_ratelimited_flags()
- Introduced the flag BDP_ASYNC.
- Renamed variable in iomap_write_iter from i_mapping to mapping.
- Directly call balance_dirty_pages_ratelimited_flags() in the function

    iomap_write_iter().

- Re-ordered the patches.

V3:
- Reformat new code in generic_write_checks_count() to line lengthof 80.
- Remove if condition in __iomap_write_begin to maintain current behavior.
- use GFP_NOWAIT flag in __iomap_write_begin
- rename need_file_remove_privs() function to file_needs_remove_privs()
- rename do_file_remove_privs to __file_remove_privs()
- add kernel documentation to file_remove_privs() function
- rework else if branch in file_remove_privs() function
- add kernel documentation to file_modified() function
- add kernel documentation to file_modified_async() function
- rename err variable in file_update_time to ret
- rename function need_file_update_time() to file_needs_update_time()
- rename function do_file_update_time() to __file_update_time()
- don't move check for FMODE_NOCMTIME in generic helper
- reformat __file_update_time for easier reading
- add kernel documentation to file_update_time() function
- fix if in file_update_time from < to <=
- move modification of inode flags from do_file_update_time to file_modified() When this function is called, the caller must hold the inode lock.
- 3 new patches from Jan to add new no_wait flag to balance_dirty_pages(), remove patch 12 from previous series
- Make balance_dirty_pages_ratelimited_flags() a static function
- Add new balance_dirty_pages_ratelimited_async() function

V2:
- Remove atomic allocation
- Use direct write in xfs_buffered_write_iomap_begin()
- Use xfs_ilock_for_iomap() in xfs_buffered_write_iomap_begin()
- Remove no_wait check at the end of xfs_buffered_write_iomap_begin() for the COW path.
- Pass xfs_inode pointer to xfs_ilock_iocb and rename function to xfs_lock_xfs_inode
- Replace existing uses of xfs_ilock_iocb with xfs_ilock_xfs_inode
- Use xfs_ilock_xfs_inode in xfs_file_buffered_write()
- Callers of xfs_ilock_for_iomap need to initialize lock mode. This is required so writes use an exclusive lock
- Split of _balance_dirty_pages() from balance_dirty_pages() and return sleep time
- Call _balance_dirty_pages() in balance_dirty_pages_ratelimited_flags()
- Move call to balance_dirty_pages_ratelimited_flags() in iomap_write_iter() to the beginning of the loop

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

Stefan Roesch (11):
iomap: Add flags parameter to iomap_page_create()
iomap: Add async buffered write support
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.
io_uring: Add support for async buffered writes
io_uring: Add tracepoint for short writes
xfs: Specify lockmode when calling xfs_ilock_for_iomap()
xfs: Add async buffered write support

 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

Signature · 1 line
2.30.2
author
#1 lore raw

PATCH RESEND v9 1/14 mm: Move starting of background writeback into the main balancing loop

From: Jan Kara <[email protected]>

We start background writeback if we are over background threshold after exiting the main loop in balance_dirty_pages(). This may result in basing the decision on already stale values (we may have slept for significant amount of time) and it is also inconvenient for refactoring needed for async dirty throttling. Move the check into the main waiting loop.

Signed-off-by Jan Kara <[email protected]>
Signed-off-by Stefan Roesch <[email protected]>
---
 mm/page-writeback.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)
mm/page-writeback.c +14 -17
index 55c2776ae699..e59c523aed1a 100644
@@ -1627,6 +1627,19 @@ static void balance_dirty_pages(struct bdi_writeback *wb,
 			}
 		}
 
+		/*
+		 * In laptop mode, we wait until hitting the higher threshold
+		 * before starting background writeout, and then write out all
+		 * the way down to the lower threshold.  So slow writers cause
+		 * minimal disk activity.
+		 *
+		 * In normal mode, we start background writeout at the lower
+		 * background_thresh, to keep the amount of dirty memory low.
+		 */
+		if (!laptop_mode && nr_reclaimable > gdtc->bg_thresh &&
+		    !writeback_in_progress(wb))
+			wb_start_background_writeback(wb);
+
 		/*
 		 * Throttle it only when the background writeback cannot
 		 * catch-up. This avoids (excessively) small writeouts

@@ -1657,6 +1670,7 @@ static void balance_dirty_pages(struct bdi_writeback *wb,
 			break;
 		}
 
+		/* Start writeback even when in laptop mode */
 		if (unlikely(!writeback_in_progress(wb)))
 			wb_start_background_writeback(wb);
 

@@ -1823,23 +1837,6 @@ static void balance_dirty_pages(struct bdi_writeback *wb,
 
 	if (!dirty_exceeded && wb->dirty_exceeded)
 		wb->dirty_exceeded = 0;
-
-	if (writeback_in_progress(wb))
-		return;
-
-	/*
-	 * In laptop mode, we wait until hitting the higher threshold before
-	 * starting background writeout, and then write out all the way down
-	 * to the lower threshold.  So slow writers cause minimal disk activity.
-	 *
-	 * In normal mode, we start background writeout at the lower
-	 * background_thresh, to keep the amount of dirty memory low.
-	 */
-	if (laptop_mode)
-		return;
-
-	if (nr_reclaimable > gdtc->bg_thresh)
-		wb_start_background_writeback(wb);
 }
 
 static DEFINE_PER_CPU(int, bdp_ratelimits);
Signature · 1 line
2.30.2
author
#2 lore raw

PATCH RESEND v9 2/14 mm: Move updates of dirty_exceeded into one place

In reply to Stefan Roesch

From: Jan Kara <[email protected]>

Transition of wb->dirty_exceeded from 0 to 1 happens before we go to sleep in balance_dirty_pages() while transition from 1 to 0 happens when exiting from balance_dirty_pages(), possibly based on old values. This does not make a lot of sense since wb->dirty_exceeded should simply reflect whether wb is over dirty limit and so we should ratelimit entering to balance_dirty_pages() less. Move the two updates together.

Signed-off-by Jan Kara <[email protected]>
Signed-off-by Stefan Roesch <[email protected]>
---
 mm/page-writeback.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
mm/page-writeback.c +2 -5
index e59c523aed1a..90b1998c16a1 100644
@@ -1729,8 +1729,8 @@ static void balance_dirty_pages(struct bdi_writeback *wb,
 				sdtc = mdtc;
 		}
 
-		if (dirty_exceeded && !wb->dirty_exceeded)
-			wb->dirty_exceeded = 1;
+		if (dirty_exceeded != wb->dirty_exceeded)
+			wb->dirty_exceeded = dirty_exceeded;
 
 		if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
 					   BANDWIDTH_INTERVAL))

@@ -1834,9 +1834,6 @@ static void balance_dirty_pages(struct bdi_writeback *wb,
 		if (fatal_signal_pending(current))
 			break;
 	}
-
-	if (!dirty_exceeded && wb->dirty_exceeded)
-		wb->dirty_exceeded = 0;
 }
 
 static DEFINE_PER_CPU(int, bdp_ratelimits);
Signature · 1 line
2.30.2
author
#3 lore raw

PATCH RESEND v9 3/14 mm: Add balance_dirty_pages_ratelimited_flags() function

In reply to Stefan Roesch

From: Jan Kara <[email protected]>

This adds the helper function balance_dirty_pages_ratelimited_flags(). It adds the parameter flags to balance_dirty_pages_ratelimited(). The flags parameter is passed to balance_dirty_pages(). For async buffered writes the flag value will be BDP_ASYNC.

If balance_dirty_pages() gets called for async buffered write, we don't want to wait. Instead we need to indicate to the caller that throttling is needed so that it can stop writing and offload the rest of the write to a context that can block.

The new helper function is also used by balance_dirty_pages_ratelimited().

Signed-off-by Jan Kara <[email protected]>
Signed-off-by Stefan Roesch <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 include/linux/writeback.h |  7 ++++++
 mm/page-writeback.c       | 51 +++++++++++++++++++++++++++++++--------
 2 files changed, 48 insertions(+), 10 deletions(-)
include/linux/writeback.h +7 -0
index da21d63f70e2..b8c9610c2313 100644
@@ -364,7 +364,14 @@ void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty);
 unsigned long wb_calc_thresh(struct bdi_writeback *wb, unsigned long thresh);
 
 void wb_update_bandwidth(struct bdi_writeback *wb);
+
+/* Invoke balance dirty pages in async mode. */
+#define BDP_ASYNC 0x0001
+
 void balance_dirty_pages_ratelimited(struct address_space *mapping);
+int balance_dirty_pages_ratelimited_flags(struct address_space *mapping,
+		unsigned int flags);
+
 bool wb_over_bg_thresh(struct bdi_writeback *wb);
 
 typedef int (*writepage_t)(struct page *page, struct writeback_control *wbc,
mm/page-writeback.c +41 -10

103 lines not on this page · raw message

Signature · 1 line
2.30.2
author
#4 lore raw

PATCH RESEND v9 4/14 iomap: Add flags parameter to iomap_page_create()

In reply to Stefan Roesch

Add the kiocb flags parameter to the function iomap_page_create(). Depending on the value of the flags parameter it enables different gfp flags.

No intended functional changes in this patch.

Signed-off-by Stefan Roesch <[email protected]>
Reviewed-by Jan Kara <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
Reviewed-by Darrick J. Wong <[email protected]>
---
 fs/iomap/buffered-io.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)
fs/iomap/buffered-io.c +20 -10
index d2a9f699e17e..3c97b713f831 100644
@@ -44,20 +44,28 @@ static inline struct iomap_page *to_iomap_page(struct folio *folio)
 static struct bio_set iomap_ioend_bioset;
 
 static struct iomap_page *
-iomap_page_create(struct inode *inode, struct folio *folio)
+iomap_page_create(struct inode *inode, struct folio *folio, unsigned int flags)
 {
 	struct iomap_page *iop = to_iomap_page(folio);
 	unsigned int nr_blocks = i_blocks_per_folio(inode, folio);
+	gfp_t gfp;
 
 	if (iop || nr_blocks <= 1)
 		return iop;
 
+	if (flags & IOMAP_NOWAIT)
+		gfp = GFP_NOWAIT;
+	else
+		gfp = GFP_NOFS | __GFP_NOFAIL;
+
 	iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)),
-			GFP_NOFS | __GFP_NOFAIL);
-	spin_lock_init(&iop->uptodate_lock);
-	if (folio_test_uptodate(folio))
-		bitmap_fill(iop->uptodate, nr_blocks);
-	folio_attach_private(folio, iop);
+		      gfp);
+	if (iop) {
+		spin_lock_init(&iop->uptodate_lock);
+		if (folio_test_uptodate(folio))
+			bitmap_fill(iop->uptodate, nr_blocks);
+		folio_attach_private(folio, iop);
+	}
 	return iop;
 }
 

@@ -226,7 +234,7 @@ static int iomap_read_inline_data(const struct iomap_iter *iter,
 	if (WARN_ON_ONCE(size > iomap->length))
 		return -EIO;
 	if (offset > 0)
-		iop = iomap_page_create(iter->inode, folio);
+		iop = iomap_page_create(iter->inode, folio, iter->flags);
 	else
 		iop = to_iomap_page(folio);
 

@@ -264,7 +272,7 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter,
 		return iomap_read_inline_data(iter, folio);
 
 	/* zero post-eof blocks as the page may be mapped */
-	iop = iomap_page_create(iter->inode, folio);
+	iop = iomap_page_create(iter->inode, folio, iter->flags);
 	iomap_adjust_read_range(iter->inode, folio, &pos, length, &poff, &plen);
 	if (plen == 0)
 		goto done;

@@ -547,7 +555,7 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 		size_t len, struct folio *folio)
 {
 	const struct iomap *srcmap = iomap_iter_srcmap(iter);
-	struct iomap_page *iop = iomap_page_create(iter->inode, folio);
+	struct iomap_page *iop;
 	loff_t block_size = i_blocksize(iter->inode);
 	loff_t block_start = round_down(pos, block_size);
 	loff_t block_end = round_up(pos + len, block_size);

@@ -558,6 +566,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 		return 0;
 	folio_clear_error(folio);
 
+	iop = iomap_page_create(iter->inode, folio, iter->flags);
+
 	do {
 		iomap_adjust_read_range(iter->inode, folio, &block_start,
 				block_end - block_start, &poff, &plen);

@@ -1329,7 +1339,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 		struct writeback_control *wbc, struct inode *inode,
 		struct folio *folio, u64 end_pos)
 {
-	struct iomap_page *iop = iomap_page_create(inode, folio);
+	struct iomap_page *iop = iomap_page_create(inode, folio, 0);
 	struct iomap_ioend *ioend, *next;
 	unsigned len = i_blocksize(inode);
 	unsigned nblocks = i_blocks_per_folio(inode, folio);
Signature · 1 line
2.30.2
maintainer
#5 lore raw

Re: [RESEND PATCH v9 04/14] iomap: Add flags parameter to iomap_page_create()

Quoted 5 lines · Stefan Roesch

Add the kiocb flags parameter to the function iomap_page_create(). Depending on the value of the flags parameter it enables different gfp flags.

No intended functional changes in this patch.

[...]

fs/iomap/buffered-io.c static int iomap_read_inline_data(const struct iomap_iter *iter, · line 239

Quoted 8 lines
fs/iomap/buffered-io.c +1 -1
@@ -226,7 +234,7 @@ static int iomap_read_inline_data(const struct iomap_iter *iter,
 	if (WARN_ON_ONCE(size > iomap->length))
 		return -EIO;
 	if (offset > 0)
-		iop = iomap_page_create(iter->inode, folio);
+		iop = iomap_page_create(iter->inode, folio, iter->flags);
 	else
 		iop = to_iomap_page(folio);

I really don't like what this change has done to this file. I'm modifying this function, and I start thinking "Well, hang on, if flags has IOMAP_NOWAIT set, then GFP_NOWAIT can fail, and iop will be NULL, so we'll end up marking the entire folio uptodate when really we should only be marking some blocks uptodate, so we should really be failing the entire read if the allocation failed, but maybe it's OK because IOMAP_NOWAIT is never set in this path".

I don't know how we fix this. Maybe return ERR_PTR(-ENOMEM) or -EAGAIN if the memory allocation fails (leaving the NULL return for "we don't need an iop"). Thoughts?

#6 lore raw

Re: [RESEND PATCH v9 04/14] iomap: Add flags parameter to iomap_page_create()

Quoted 27 lines · Matthew Wilcox
Quoted 5 lines · Stefan Roesch

Add the kiocb flags parameter to the function iomap_page_create(). Depending on the value of the flags parameter it enables different gfp flags.

No intended functional changes in this patch.

[...]

fs/iomap/buffered-io.c static int iomap_read_inline_data(const struct iomap_iter *iter, · line 239

Quoted 8 lines
fs/iomap/buffered-io.c +1 -1
@@ -226,7 +234,7 @@ static int iomap_read_inline_data(const struct iomap_iter *iter,
 	if (WARN_ON_ONCE(size > iomap->length))
 		return -EIO;
 	if (offset > 0)
-		iop = iomap_page_create(iter->inode, folio);
+		iop = iomap_page_create(iter->inode, folio, iter->flags);
 	else
 		iop = to_iomap_page(folio);

I really don't like what this change has done to this file. I'm modifying this function, and I start thinking "Well, hang on, if flags has IOMAP_NOWAIT set, then GFP_NOWAIT can fail, and iop will be NULL, so we'll end up marking the entire folio uptodate when really we should only be marking some blocks uptodate, so we should really be failing the entire read if the allocation failed, but maybe it's OK because IOMAP_NOWAIT is never set in this path".

Quoted 3 lines

I don't know how we fix this. Maybe return ERR_PTR(-ENOMEM) or -EAGAIN if the memory allocation fails (leaving the NULL return for "we don't need an iop"). Thoughts?

I don't see any problem with that, aside from being pre-coffee and on vacation for the rest of today. ;)

--D

#7 lore raw

Re: [RESEND PATCH v9 04/14] iomap: Add flags parameter to iomap_page_create()

Quoted 29 lines · Darrick J. Wong
Quoted 1 line
Quoted 27 lines · Matthew Wilcox
Quoted 5 lines · Stefan Roesch

Add the kiocb flags parameter to the function iomap_page_create(). Depending on the value of the flags parameter it enables different gfp flags.

No intended functional changes in this patch.

[...]

fs/iomap/buffered-io.c static int iomap_read_inline_data(const struct iomap_iter *iter, · line 239

Quoted 8 lines
fs/iomap/buffered-io.c +1 -1
@@ -226,7 +234,7 @@ static int iomap_read_inline_data(const struct iomap_iter *iter,
 	if (WARN_ON_ONCE(size > iomap->length))
 		return -EIO;
 	if (offset > 0)
-		iop = iomap_page_create(iter->inode, folio);
+		iop = iomap_page_create(iter->inode, folio, iter->flags);
 	else
 		iop = to_iomap_page(folio);

I really don't like what this change has done to this file. I'm modifying this function, and I start thinking "Well, hang on, if flags has IOMAP_NOWAIT set, then GFP_NOWAIT can fail, and iop will be NULL, so we'll end up marking the entire folio uptodate when really we should only be marking some blocks uptodate, so we should really be failing the entire read if the allocation failed, but maybe it's OK because IOMAP_NOWAIT is never set in this path".

Quoted 8 lines
Quoted 3 lines

I don't know how we fix this. Maybe return ERR_PTR(-ENOMEM) or -EAGAIN if the memory allocation fails (leaving the NULL return for "we don't need an iop"). Thoughts?

I don't see any problem with that, aside from being pre-coffee and on vacation for the rest of today. ;)

--D

If IOMAP_NOWAIT is set, and the allocation fails, we should return -EAGAIN, so the write request is retried in the slow path.

--Stefan

#8 lore raw

Re: [RESEND PATCH v9 04/14] iomap: Add flags parameter to iomap_page_create()

Quoted 2 lines · Stefan Roesch

If IOMAP_NOWAIT is set, and the allocation fails, we should return -EAGAIN, so the write request is retried in the slow path.

Yes. Another vote for doing the ERR_PTR.

willy, are you going to look into that yourself or are you waiting for someone to take care of it?

author
#9 lore raw

PATCH RESEND v9 5/14 iomap: Add async buffered write support

In reply to Stefan Roesch

This adds async buffered write support to iomap.

This replaces the call to balance_dirty_pages_ratelimited() with the call to balance_dirty_pages_ratelimited_flags. This allows to specify if the write request is async or not.

In addition this also moves the above function call to the beginning of the function. If the function call is at the end of the function and the decision is made to throttle writes, then there is no request that io-uring can wait on. By moving it to the beginning of the function, the write request is not issued, but returns -EAGAIN instead. io-uring will punt the request and process it in the io-worker.

By moving the function call to the beginning of the function, the write throttling will happen one page later.

Signed-off-by Stefan Roesch <[email protected]>
Reviewed-by Jan Kara <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/iomap/buffered-io.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)
fs/iomap/buffered-io.c +28 -5
index 3c97b713f831..83cf093fcb92 100644
@@ -559,6 +559,7 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 	loff_t block_size = i_blocksize(iter->inode);
 	loff_t block_start = round_down(pos, block_size);
 	loff_t block_end = round_up(pos + len, block_size);
+	unsigned int nr_blocks = i_blocks_per_folio(iter->inode, folio);
 	size_t from = offset_in_folio(folio, pos), to = from + len;
 	size_t poff, plen;
 

@@ -567,6 +568,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 	folio_clear_error(folio);
 
 	iop = iomap_page_create(iter->inode, folio, iter->flags);
+	if ((iter->flags & IOMAP_NOWAIT) && !iop && nr_blocks > 1)
+		return -EAGAIN;
 
 	do {
 		iomap_adjust_read_range(iter->inode, folio, &block_start,

@@ -584,7 +587,12 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 				return -EIO;
 			folio_zero_segments(folio, poff, from, to, poff + plen);
 		} else {
-			int status = iomap_read_folio_sync(block_start, folio,
+			int status;
+
+			if (iter->flags & IOMAP_NOWAIT)
+				return -EAGAIN;
+
+			status = iomap_read_folio_sync(block_start, folio,
 					poff, plen, srcmap);
 			if (status)
 				return status;

@@ -613,6 +621,9 @@ static int iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 	unsigned fgp = FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE | FGP_NOFS;
 	int status = 0;
 
+	if (iter->flags & IOMAP_NOWAIT)
+		fgp |= FGP_NOWAIT;
+
 	BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length);
 	if (srcmap != &iter->iomap)
 		BUG_ON(pos + len > srcmap->offset + srcmap->length);

@@ -632,7 +643,7 @@ static int iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 	folio = __filemap_get_folio(iter->inode->i_mapping, pos >> PAGE_SHIFT,
 			fgp, mapping_gfp_mask(iter->inode->i_mapping));
 	if (!folio) {
-		status = -ENOMEM;
+		status = (iter->flags & IOMAP_NOWAIT) ? -EAGAIN : -ENOMEM;
 		goto out_no_page;
 	}
 	if (pos + len > folio_pos(folio) + folio_size(folio))

@@ -750,6 +761,8 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 	loff_t pos = iter->pos;
 	ssize_t written = 0;
 	long status = 0;
+	struct address_space *mapping = iter->inode->i_mapping;
+	unsigned int bdp_flags = (iter->flags & IOMAP_NOWAIT) ? BDP_ASYNC : 0;
 
 	do {
 		struct folio *folio;

@@ -762,6 +775,11 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		bytes = min_t(unsigned long, PAGE_SIZE - offset,
 						iov_iter_count(i));
 again:
+		status = balance_dirty_pages_ratelimited_flags(mapping,
+							       bdp_flags);
+		if (unlikely(status))
+			break;
+
 		if (bytes > length)
 			bytes = length;
 

@@ -770,6 +788,10 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		 * Otherwise there's a nasty deadlock on copying from the
 		 * same page as we're writing to, without it being marked
 		 * up-to-date.
+		 *
+		 * For async buffered writes the assumption is that the user
+		 * page has already been faulted in. This can be optimized by
+		 * faulting the user page.
 		 */
 		if (unlikely(fault_in_iov_iter_readable(i, bytes) == bytes)) {
 			status = -EFAULT;

@@ -781,7 +803,7 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 			break;
 
 		page = folio_file_page(folio, pos >> PAGE_SHIFT);
-		if (mapping_writably_mapped(iter->inode->i_mapping))
+		if (mapping_writably_mapped(mapping))
 			flush_dcache_page(page);
 
 		copied = copy_page_from_iter_atomic(page, offset, bytes, i);

@@ -806,8 +828,6 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		pos += status;
 		written += status;
 		length -= status;
-
-		balance_dirty_pages_ratelimited(iter->inode->i_mapping);
 	} while (iov_iter_count(i) && length);
 
 	return written ? written : status;

@@ -825,6 +845,9 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i,
 	};
 	int ret;
 
+	if (iocb->ki_flags & IOCB_NOWAIT)
+		iter.flags |= IOMAP_NOWAIT;
+
 	while ((ret = iomap_iter(&iter, ops)) > 0)
 		iter.processed = iomap_write_iter(&iter, i);
 	if (iter.pos == iocb->ki_pos)
Signature · 1 line
2.30.2
author
#10 lore raw

PATCH RESEND v9 6/14 iomap: Return -EAGAIN from iomap_write_iter()

In reply to Stefan Roesch

If iomap_write_iter() encounters -EAGAIN, return -EAGAIN to the caller.

Signed-off-by Stefan Roesch <[email protected]>
---
 fs/iomap/buffered-io.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
fs/iomap/buffered-io.c +7 -1
index 83cf093fcb92..f2e36240079f 100644
@@ -830,7 +830,13 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		length -= status;
 	} while (iov_iter_count(i) && length);
 
-	return written ? written : status;
+	if (status == -EAGAIN) {
+		iov_iter_revert(i, written);
+		return -EAGAIN;
+	}
+	if (written)
+		return written;
+	return status;
 }
 
 ssize_t
Signature · 1 line
2.30.2
Reviewed-by on v9 6/14
#11 lore raw

Re: [RESEND PATCH v9 06/14] iomap: Return -EAGAIN from iomap_write_iter()

Quoted 13 lines · Stefan Roesch

If iomap_write_iter() encounters -EAGAIN, return -EAGAIN to the caller.

Signed-off-by Stefan Roesch <[email protected]>
---
 fs/iomap/buffered-io.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
fs/iomap/buffered-io.c +0 -0
index 83cf093fcb92..f2e36240079f 100644
@@ -830,7 +830,13 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		length -= status;

fs/iomap/buffered-io.c static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) · line 839

Quoted 10 lines
fs/iomap/buffered-io.c +7 -1
 	} while (iov_iter_count(i) && length);
 
-	return written ? written : status;
+	if (status == -EAGAIN) {
+		iov_iter_revert(i, written);
+		return -EAGAIN;
+	}
+	if (written)
+		return written;
+	return status;

Any particular reason for decomposing the ternary into this? It still looks correct, but it doesn't seem totally necessary...

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 6 lines
fs/iomap/buffered-io.c +0 -0
 }
 
 ssize_t
Signature · 1 line
2.30.2
author
#12 lore raw

Re: [RESEND PATCH v9 06/14] iomap: Return -EAGAIN from iomap_write_iter()

Quoted 16 lines · Darrick J. Wong
Quoted 15 lines · Stefan Roesch

If iomap_write_iter() encounters -EAGAIN, return -EAGAIN to the caller.

Signed-off-by Stefan Roesch <[email protected]>
---
 fs/iomap/buffered-io.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
fs/iomap/buffered-io.c +0 -0
index 83cf093fcb92..f2e36240079f 100644
@@ -830,7 +830,13 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		length -= status;
 	} while (iov_iter_count(i) && length);
Quoted 12 lines

fs/iomap/buffered-io.c static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) · line 839

Quoted 8 lines
fs/iomap/buffered-io.c +7 -1
-	return written ? written : status;
+	if (status == -EAGAIN) {
+		iov_iter_revert(i, written);
+		return -EAGAIN;
+	}
+	if (written)
+		return written;
+	return status;

Any particular reason for decomposing the ternary into this? It still looks correct, but it doesn't seem totally necessary...

Do you prefer this version?

+ if (status == -EAGAIN) {
+ iov_iter_revert(i, written);
+ return -EAGAIN;
+ }

	return written ? written : status;
Quoted 10 lines
Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 6 lines
fs/iomap/buffered-io.c +0 -0
 }
 
 ssize_t
Signature · 1 line
2.30.2
#13 lore raw

Re: [RESEND PATCH v9 06/14] iomap: Return -EAGAIN from iomap_write_iter()

Quoted 38 lines · Stefan Roesch
Quoted 16 lines · Darrick J. Wong
Quoted 15 lines · Stefan Roesch

If iomap_write_iter() encounters -EAGAIN, return -EAGAIN to the caller.

Signed-off-by Stefan Roesch <[email protected]>
---
 fs/iomap/buffered-io.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
fs/iomap/buffered-io.c +0 -0
index 83cf093fcb92..f2e36240079f 100644
@@ -830,7 +830,13 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		length -= status;
 	} while (iov_iter_count(i) && length);
Quoted 12 lines

fs/iomap/buffered-io.c static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) · line 839

Quoted 8 lines
fs/iomap/buffered-io.c +7 -1
-	return written ? written : status;
+	if (status == -EAGAIN) {
+		iov_iter_revert(i, written);
+		return -EAGAIN;
+	}
+	if (written)
+		return written;
+	return status;

Any particular reason for decomposing the ternary into this? It still looks correct, but it doesn't seem totally necessary...

Do you prefer this version?

fs/iomap/buffered-io.c +4 -0
+	if (status == -EAGAIN) {
+		iov_iter_revert(i, written);
+		return -EAGAIN;
+	}
Quoted 1 line
	return written ? written : status;

Yes, because it /does/ make it a lot more obvious that the only change is intercepting EAGAIN to rewind the iov_iter. :)

--D

Quoted 12 lines
Quoted 12 lines
Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 6 lines
fs/iomap/buffered-io.c +0 -0
 }
 
 ssize_t
Signature · 1 line
2.30.2
author
#15 lore raw

PATCH RESEND v9 7/14 fs: Add check for async buffered writes to generic_write_checks

In reply to Stefan Roesch

This introduces the flag FMODE_BUF_WASYNC. If devices support async buffered writes, this flag can be set. It also modifies the check in generic_write_checks to take async buffered writes into consideration.

Signed-off-by Stefan Roesch <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
Reviewed-by Christian Brauner (Microsoft) <[email protected]>
---
 fs/read_write.c    | 4 +++-
 include/linux/fs.h | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)
fs/read_write.c +3 -1
index b1b1cdfee9d3..a1f4d45c2406 100644
@@ -1649,7 +1649,9 @@ int generic_write_checks_count(struct kiocb *iocb, loff_t *count)
 	if (iocb->ki_flags & IOCB_APPEND)
 		iocb->ki_pos = i_size_read(inode);
 
-	if ((iocb->ki_flags & IOCB_NOWAIT) && !(iocb->ki_flags & IOCB_DIRECT))
+	if ((iocb->ki_flags & IOCB_NOWAIT) &&
+	    !((iocb->ki_flags & IOCB_DIRECT) ||
+	      (file->f_mode & FMODE_BUF_WASYNC)))
 		return -EINVAL;
 
 	return generic_write_check_limits(iocb->ki_filp, iocb->ki_pos, count);
include/linux/fs.h +3 -0
index 9ad5e3520fae..bc84847c201e 100644
@@ -180,6 +180,9 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 /* File supports async buffered reads */
 #define FMODE_BUF_RASYNC	((__force fmode_t)0x40000000)
 
+/* File supports async nowait buffered writes */
+#define FMODE_BUF_WASYNC	((__force fmode_t)0x80000000)
+
 /*
  * Attribute flags.  These should be or-ed together to figure out what
  * has been changed!
Signature · 1 line
2.30.2
#16 lore raw

Re: [RESEND PATCH v9 07/14] fs: Add check for async buffered writes to generic_write_checks

FYI, I think a subject like

"fs: add a FMODE_BUF_WASYNC flags for f_mode"

might be a more descriptive. As the new flag here really is the interesting part, not that we check it.

maintainer
#17 lore raw

Re: [RESEND PATCH v9 07/14] fs: Add check for async buffered writes to generic_write_checks

Quoted 6 lines · Christoph Hellwig

FYI, I think a subject like

"fs: add a FMODE_BUF_WASYNC flags for f_mode"

might be a more descriptive. As the new flag here really is the interesting part, not that we check it.

Agree on that - if others do too, I can just make that edit.

Signature · 1 line
Jens Axboe
maintainer
#18 lore raw

Re: [RESEND PATCH v9 07/14] fs: Add check for async buffered writes to generic_write_checks

In reply to Christoph Hellwig
Quoted 6 lines · Christoph Hellwig

FYI, I think a subject like

"fs: add a FMODE_BUF_WASYNC flags for f_mode"

might be a more descriptive. As the new flag here really is the interesting part, not that we check it.

I made that edit.

Signature · 1 line
Jens Axboe
author
#19 lore raw

PATCH RESEND v9 8/14 fs: add __remove_file_privs() with flags parameter

In reply to Stefan Roesch

This adds the function __remove_file_privs, which allows the caller to pass the kiocb flags parameter.

No intended functional changes in this patch.

Signed-off-by Stefan Roesch <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
Reviewed-by Jan Kara <[email protected]>
Reviewed-by Christian Brauner (Microsoft) <[email protected]>
---
 fs/inode.c | 57 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 20 deletions(-)
fs/inode.c +37 -20
index bd4da9c5207e..a2e18379c8a6 100644
@@ -2010,36 +2010,43 @@ static int __remove_privs(struct user_namespace *mnt_userns,
 	return notify_change(mnt_userns, dentry, &newattrs, NULL);
 }
 
-/*
- * Remove special file priviledges (suid, capabilities) when file is written
- * to or truncated.
- */
-int file_remove_privs(struct file *file)
+static int __file_remove_privs(struct file *file, unsigned int flags)
 {
 	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = file_inode(file);
+	int error;
 	int kill;
-	int error = 0;
 
-	/*
-	 * Fast path for nothing security related.
-	 * As well for non-regular files, e.g. blkdev inodes.
-	 * For example, blkdev_write_iter() might get here
-	 * trying to remove privs which it is not allowed to.
-	 */
 	if (IS_NOSEC(inode) || !S_ISREG(inode->i_mode))
 		return 0;
 
 	kill = dentry_needs_remove_privs(dentry);
-	if (kill < 0)
+	if (kill <= 0)
 		return kill;
-	if (kill)
-		error = __remove_privs(file_mnt_user_ns(file), dentry, kill);
+
+	if (flags & IOCB_NOWAIT)
+		return -EAGAIN;
+
+	error = __remove_privs(file_mnt_user_ns(file), dentry, kill);
 	if (!error)
 		inode_has_no_xattr(inode);
 
 	return error;
 }
+
+/**
+ * file_remove_privs - remove special file privileges (suid, capabilities)
+ * @file: file to remove privileges from
+ *
+ * When file is modified by a write or truncation ensure that special
+ * file privileges are removed.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int file_remove_privs(struct file *file)
+{
+	return __file_remove_privs(file, 0);
+}
 EXPORT_SYMBOL(file_remove_privs);
 
 /**

@@ -2090,18 +2097,28 @@ int file_update_time(struct file *file)
 }
 EXPORT_SYMBOL(file_update_time);
 
-/* Caller must hold the file's inode lock */
+/**
+ * file_modified - handle mandated vfs changes when modifying a file
+ * @file: file that was modified
+ *
+ * When file has been modified ensure that special
+ * file privileges are removed and time settings are updated.
+ *
+ * Context: Caller must hold the file's inode lock.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
 int file_modified(struct file *file)
 {
-	int err;
+	int ret;
 
 	/*
 	 * Clear the security bits if the process is not being run by root.
 	 * This keeps people from modifying setuid and setgid binaries.
 	 */
-	err = file_remove_privs(file);
-	if (err)
-		return err;
+	ret = __file_remove_privs(file, 0);
+	if (ret)
+		return ret;
 
 	if (unlikely(file->f_mode & FMODE_NOCMTIME))
 		return 0;
Signature · 1 line
2.30.2
author
#20 lore raw

PATCH RESEND v9 9/14 fs: Split off inode_needs_update_time and __file_update_time

In reply to Stefan Roesch

This splits off the functions inode_needs_update_time() and __file_update_time() from the function file_update_time().

This is required to support async buffered writes. No intended functional changes in this patch.

Signed-off-by Stefan Roesch <[email protected]>
Reviewed-by Jan Kara <[email protected]>
Reviewed-by Christian Brauner (Microsoft) <[email protected]>
---
 fs/inode.c | 76 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 26 deletions(-)
fs/inode.c +50 -26
index a2e18379c8a6..ff726d99ecc7 100644
@@ -2049,35 +2049,18 @@ int file_remove_privs(struct file *file)
 }
 EXPORT_SYMBOL(file_remove_privs);
 
-/**
- *	file_update_time	-	update mtime and ctime time
- *	@file: file accessed
- *
- *	Update the mtime and ctime members of an inode and mark the inode
- *	for writeback.  Note that this function is meant exclusively for
- *	usage in the file write path of filesystems, and filesystems may
- *	choose to explicitly ignore update via this function with the
- *	S_NOCMTIME inode flag, e.g. for network filesystem where these
- *	timestamps are handled by the server.  This can return an error for
- *	file systems who need to allocate space in order to update an inode.
- */
-
-int file_update_time(struct file *file)
+static int inode_needs_update_time(struct inode *inode, struct timespec64 *now)
 {
-	struct inode *inode = file_inode(file);
-	struct timespec64 now;
 	int sync_it = 0;
-	int ret;
 
 	/* First try to exhaust all avenues to not sync */
 	if (IS_NOCMTIME(inode))
 		return 0;
 
-	now = current_time(inode);
-	if (!timespec64_equal(&inode->i_mtime, &now))
+	if (!timespec64_equal(&inode->i_mtime, now))
 		sync_it = S_MTIME;
 
-	if (!timespec64_equal(&inode->i_ctime, &now))
+	if (!timespec64_equal(&inode->i_ctime, now))
 		sync_it |= S_CTIME;
 
 	if (IS_I_VERSION(inode) && inode_iversion_need_inc(inode))

@@ -2086,15 +2069,50 @@ int file_update_time(struct file *file)
 	if (!sync_it)
 		return 0;
 
-	/* Finally allowed to write? Takes lock. */
-	if (__mnt_want_write_file(file))
-		return 0;
+	return sync_it;
+}
+
+static int __file_update_time(struct file *file, struct timespec64 *now,
+			int sync_mode)
+{
+	int ret = 0;
+	struct inode *inode = file_inode(file);
 
-	ret = inode_update_time(inode, &now, sync_it);
-	__mnt_drop_write_file(file);
+	/* try to update time settings */
+	if (!__mnt_want_write_file(file)) {
+		ret = inode_update_time(inode, now, sync_mode);
+		__mnt_drop_write_file(file);
+	}
 
 	return ret;
 }
+
+/**
+ * file_update_time - update mtime and ctime time
+ * @file: file accessed
+ *
+ * Update the mtime and ctime members of an inode and mark the inode for
+ * writeback. Note that this function is meant exclusively for usage in
+ * the file write path of filesystems, and filesystems may choose to
+ * explicitly ignore updates via this function with the _NOCMTIME inode
+ * flag, e.g. for network filesystem where these imestamps are handled
+ * by the server. This can return an error for file systems who need to
+ * allocate space in order to update an inode.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int file_update_time(struct file *file)
+{
+	int ret;
+	struct inode *inode = file_inode(file);
+	struct timespec64 now = current_time(inode);
+
+	ret = inode_needs_update_time(inode, &now);
+	if (ret <= 0)
+		return ret;
+
+	return __file_update_time(file, &now, ret);
+}
 EXPORT_SYMBOL(file_update_time);
 
 /**

@@ -2111,6 +2129,8 @@ EXPORT_SYMBOL(file_update_time);
 int file_modified(struct file *file)
 {
 	int ret;
+	struct inode *inode = file_inode(file);
+	struct timespec64 now = current_time(inode);
 
 	/*
 	 * Clear the security bits if the process is not being run by root.

@@ -2123,7 +2143,11 @@ int file_modified(struct file *file)
 	if (unlikely(file->f_mode & FMODE_NOCMTIME))
 		return 0;
 
-	return file_update_time(file);
+	ret = inode_needs_update_time(inode, &now);
+	if (ret <= 0)
+		return ret;
+
+	return __file_update_time(file, &now, ret);
 }
 EXPORT_SYMBOL(file_modified);
Signature · 1 line
2.30.2
author
#21 lore raw

PATCH RESEND v9 10/14 fs: Add async write file modification handling.

In reply to Stefan Roesch

This adds a file_modified_async() function to return -EAGAIN if the request either requires to remove privileges or needs to update the file modification time. This is required for async buffered writes, so the request gets handled in the io worker of io-uring.

Signed-off-by Stefan Roesch <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
Reviewed-by Jan Kara <[email protected]>
Reviewed-by Christian Brauner (Microsoft) <[email protected]>
---
 fs/inode.c         | 45 ++++++++++++++++++++++++++++++++++++++++++---
 include/linux/fs.h |  1 +
 2 files changed, 43 insertions(+), 3 deletions(-)
fs/inode.c +42 -3
index ff726d99ecc7..259ebf438893 100644
@@ -2116,17 +2116,21 @@ int file_update_time(struct file *file)
 EXPORT_SYMBOL(file_update_time);
 
 /**
- * file_modified - handle mandated vfs changes when modifying a file
+ * file_modified_flags - handle mandated vfs changes when modifying a file
  * @file: file that was modified
+ * @flags: kiocb flags
  *
  * When file has been modified ensure that special
  * file privileges are removed and time settings are updated.
  *
+ * If IOCB_NOWAIT is set, special file privileges will not be removed and
+ * time settings will not be updated. It will return -EAGAIN.
+ *
  * Context: Caller must hold the file's inode lock.
  *
  * Return: 0 on success, negative errno on failure.
  */
-int file_modified(struct file *file)
+static int file_modified_flags(struct file *file, int flags)
 {
 	int ret;
 	struct inode *inode = file_inode(file);

@@ -2136,7 +2140,7 @@ int file_modified(struct file *file)
 	 * Clear the security bits if the process is not being run by root.
 	 * This keeps people from modifying setuid and setgid binaries.
 	 */
-	ret = __file_remove_privs(file, 0);
+	ret = __file_remove_privs(file, flags);
 	if (ret)
 		return ret;
 

@@ -2146,11 +2150,46 @@ int file_modified(struct file *file)
 	ret = inode_needs_update_time(inode, &now);
 	if (ret <= 0)
 		return ret;
+	if (flags & IOCB_NOWAIT)
+		return -EAGAIN;
 
 	return __file_update_time(file, &now, ret);
 }
+
+/**
+ * file_modified - handle mandated vfs changes when modifying a file
+ * @file: file that was modified
+ *
+ * When file has been modified ensure that special
+ * file privileges are removed and time settings are updated.
+ *
+ * Context: Caller must hold the file's inode lock.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int file_modified(struct file *file)
+{
+	return file_modified_flags(file, 0);
+}
 EXPORT_SYMBOL(file_modified);
 
+/**
+ * kiocb_modified - handle mandated vfs changes when modifying a file
+ * @iocb: iocb that was modified
+ *
+ * When file has been modified ensure that special
+ * file privileges are removed and time settings are updated.
+ *
+ * Context: Caller must hold the file's inode lock.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int kiocb_modified(struct kiocb *iocb)
+{
+	return file_modified_flags(iocb->ki_filp, iocb->ki_flags);
+}
+EXPORT_SYMBOL_GPL(kiocb_modified);
+
 int inode_needs_sync(struct inode *inode)
 {
 	if (IS_SYNC(inode))
include/linux/fs.h +1 -0
index bc84847c201e..c0d99b5a166b 100644
@@ -2390,6 +2390,7 @@ static inline void file_accessed(struct file *file)
 }
 
 extern int file_modified(struct file *file);
+int kiocb_modified(struct kiocb *iocb);
 
 int sync_inode_metadata(struct inode *inode, int wait);
Signature · 1 line
2.30.2
author
#22 lore raw

PATCH RESEND v9 11/14 io_uring: Add support for async buffered writes

In reply to Stefan Roesch

This enables the async buffered writes for the filesystems that support async buffered writes in io-uring. Buffered writes are enabled for blocks that are already in the page cache or can be acquired with noio.

Signed-off-by Stefan Roesch <[email protected]>
---
 fs/io_uring.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)
fs/io_uring.c +24 -5
index 3aab4182fd89..22a0bb8c5fe5 100644
@@ -4311,7 +4311,7 @@ static inline int io_iter_do_read(struct io_kiocb *req, struct iov_iter *iter)
 		return -EINVAL;
 }
 
-static bool need_read_all(struct io_kiocb *req)
+static bool need_complete_io(struct io_kiocb *req)
 {
 	return req->flags & REQ_F_ISREG ||
 		S_ISBLK(file_inode(req->file)->i_mode);

@@ -4440,7 +4440,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 	} else if (ret == -EIOCBQUEUED) {
 		goto out_free;
 	} else if (ret == req->cqe.res || ret <= 0 || !force_nonblock ||
-		   (req->flags & REQ_F_NOWAIT) || !need_read_all(req)) {
+		   (req->flags & REQ_F_NOWAIT) || !need_complete_io(req)) {
 		/* read all, failed, already did sync or don't want to retry */
 		goto done;
 	}

@@ -4536,9 +4536,10 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 		if (unlikely(!io_file_supports_nowait(req)))
 			goto copy_iov;
 
-		/* file path doesn't support NOWAIT for non-direct_IO */
-		if (force_nonblock && !(kiocb->ki_flags & IOCB_DIRECT) &&
-		    (req->flags & REQ_F_ISREG))
+		/* File path supports NOWAIT for non-direct_IO only for block devices. */
+		if (!(kiocb->ki_flags & IOCB_DIRECT) &&
+			!(kiocb->ki_filp->f_mode & FMODE_BUF_WASYNC) &&
+			(req->flags & REQ_F_ISREG))
 			goto copy_iov;
 
 		kiocb->ki_flags |= IOCB_NOWAIT;

@@ -4592,6 +4593,24 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 		/* IOPOLL retry should happen for io-wq threads */
 		if (ret2 == -EAGAIN && (req->ctx->flags & IORING_SETUP_IOPOLL))
 			goto copy_iov;
+
+		if (ret2 != req->cqe.res && ret2 >= 0 && need_complete_io(req)) {
+			struct io_async_rw *rw;
+
+			/* This is a partial write. The file pos has already been
+			 * updated, setup the async struct to complete the request
+			 * in the worker. Also update bytes_done to account for
+			 * the bytes already written.
+			 */
+			iov_iter_save_state(&s->iter, &s->iter_state);
+			ret = io_setup_async_rw(req, iovec, s, true);
+
+			rw = req->async_data;
+			if (rw)
+				rw->bytes_done += ret2;
+
+			return ret ? ret : -EAGAIN;
+		}
 done:
 		kiocb_done(req, ret2, issue_flags);
 	} else {
Signature · 1 line
2.30.2
author
#23 lore raw

PATCH RESEND v9 12/14 io_uring: Add tracepoint for short writes

In reply to Stefan Roesch

This adds the io_uring_short_write tracepoint to io_uring. A short write is issued if not all pages that are required for a write are in the page cache and the async buffered writes have to return EAGAIN.

Signed-off-by Stefan Roesch <[email protected]>
---
 fs/io_uring.c                   |  3 +++
 include/trace/events/io_uring.h | 25 +++++++++++++++++++++++++
 2 files changed, 28 insertions(+)
fs/io_uring.c +3 -0
index 22a0bb8c5fe5..510c09192832 100644
@@ -4597,6 +4597,9 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 		if (ret2 != req->cqe.res && ret2 >= 0 && need_complete_io(req)) {
 			struct io_async_rw *rw;
 
+			trace_io_uring_short_write(req->ctx, kiocb->ki_pos - ret2,
+						req->cqe.res, ret2);
+
 			/* This is a partial write. The file pos has already been
 			 * updated, setup the async struct to complete the request
 			 * in the worker. Also update bytes_done to account for
include/trace/events/io_uring.h +25 -0
index 66fcc5a1a5b1..25df513660cc 100644
@@ -600,6 +600,31 @@ TRACE_EVENT(io_uring_cqe_overflow,
 		  __entry->cflags, __entry->ocqe)
 );
 
+TRACE_EVENT(io_uring_short_write,
+
+	TP_PROTO(void *ctx, u64 fpos, u64 wanted, u64 got),
+
+	TP_ARGS(ctx, fpos, wanted, got),
+
+	TP_STRUCT__entry(
+		__field(void *,	ctx)
+		__field(u64,	fpos)
+		__field(u64,	wanted)
+		__field(u64,	got)
+	),
+
+	TP_fast_assign(
+		__entry->ctx	= ctx;
+		__entry->fpos	= fpos;
+		__entry->wanted	= wanted;
+		__entry->got	= got;
+	),
+
+	TP_printk("ring %p, fpos %lld, wanted %lld, got %lld",
+			  __entry->ctx, __entry->fpos,
+			  __entry->wanted, __entry->got)
+);
+
 #endif /* _TRACE_IO_URING_H */
 
 /* This part must be outside protection */
Signature · 1 line
2.30.2
author
#24 lore raw

PATCH RESEND v9 13/14 xfs: Specify lockmode when calling xfs_ilock_for_iomap()

In reply to Stefan Roesch

This patch changes the helper function xfs_ilock_for_iomap such that the lock mode must be passed in.

Signed-off-by Stefan Roesch <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
Reviewed-by Darrick J. Wong <[email protected]>
---
 fs/xfs/xfs_iomap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
fs/xfs/xfs_iomap.c +3 -3
index 5a393259a3a3..bcf7c3694290 100644
@@ -664,7 +664,7 @@ xfs_ilock_for_iomap(
 	unsigned		flags,
 	unsigned		*lockmode)
 {
-	unsigned		mode = XFS_ILOCK_SHARED;
+	unsigned int		mode = *lockmode;
 	bool			is_write = flags & (IOMAP_WRITE | IOMAP_ZERO);
 
 	/*

@@ -742,7 +742,7 @@ xfs_direct_write_iomap_begin(
 	int			nimaps = 1, error = 0;
 	bool			shared = false;
 	u16			iomap_flags = 0;
-	unsigned		lockmode;
+	unsigned int		lockmode = XFS_ILOCK_SHARED;
 
 	ASSERT(flags & (IOMAP_WRITE | IOMAP_ZERO));
 

@@ -1172,7 +1172,7 @@ xfs_read_iomap_begin(
 	xfs_fileoff_t		end_fsb = xfs_iomap_end_fsb(mp, offset, length);
 	int			nimaps = 1, error = 0;
 	bool			shared = false;
-	unsigned		lockmode;
+	unsigned int		lockmode = XFS_ILOCK_SHARED;
 
 	ASSERT(!(flags & (IOMAP_WRITE | IOMAP_ZERO)));
Signature · 1 line
2.30.2
author
#25 lore raw

PATCH RESEND v9 14/14 xfs: Add async buffered write support

In reply to Stefan Roesch

This adds the async buffered write support to XFS. For async buffered write requests, the request will return -EAGAIN if the ilock cannot be obtained immediately.

Signed-off-by Stefan Roesch <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/xfs/xfs_file.c  | 11 +++++------
 fs/xfs/xfs_iomap.c |  5 ++++-
 2 files changed, 9 insertions(+), 7 deletions(-)
fs/xfs/xfs_file.c +5 -6
index 5a171c0b244b..8d9b14d2b912 100644
@@ -410,7 +410,7 @@ xfs_file_write_checks(
 		spin_unlock(&ip->i_flags_lock);
 
 out:
-	return file_modified(file);
+	return kiocb_modified(iocb);
 }
 
 static int

@@ -700,12 +700,11 @@ xfs_file_buffered_write(
 	bool			cleared_space = false;
 	unsigned int		iolock;
 
-	if (iocb->ki_flags & IOCB_NOWAIT)
-		return -EOPNOTSUPP;
-
 write_retry:
 	iolock = XFS_IOLOCK_EXCL;
-	xfs_ilock(ip, iolock);
+	ret = xfs_ilock_iocb(iocb, iolock);
+	if (ret)
+		return ret;
 
 	ret = xfs_file_write_checks(iocb, from, &iolock);
 	if (ret)

@@ -1165,7 +1164,7 @@ xfs_file_open(
 {
 	if (xfs_is_shutdown(XFS_M(inode->i_sb)))
 		return -EIO;
-	file->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC;
+	file->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC | FMODE_BUF_WASYNC;
 	return generic_file_open(inode, file);
 }
fs/xfs/xfs_iomap.c +4 -1
index bcf7c3694290..5d50fed291b4 100644
@@ -886,6 +886,7 @@ xfs_buffered_write_iomap_begin(
 	bool			eof = false, cow_eof = false, shared = false;
 	int			allocfork = XFS_DATA_FORK;
 	int			error = 0;
+	unsigned int		lockmode = XFS_ILOCK_EXCL;
 
 	if (xfs_is_shutdown(mp))
 		return -EIO;

@@ -897,7 +898,9 @@ xfs_buffered_write_iomap_begin(
 
 	ASSERT(!XFS_IS_REALTIME_INODE(ip));
 
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	error = xfs_ilock_for_iomap(ip, flags, &lockmode);
+	if (error)
+		return error;
 
 	if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(&ip->i_df)) ||
 	    XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
Signature · 1 line
2.30.2
Reviewed-by Reviewed-by Reviewed-by on the v9 series
#26 lore raw
In reply to Stefan Roesch
Quoted 18 lines · Stefan Roesch

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):

Quoted 5 lines

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!

Quoted 48 lines

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.

Quoted 6 lines

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!

Quoted 4 lines

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...)

Quoted 3 lines

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

Reviewed-by Darrick J. Wong <[email protected]>
Quoted 5 lines

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 <[email protected]>
Quoted 1 line

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...

Quoted 3 lines

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 <[email protected]>

--D

Quoted 18 lines
 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

Signature · 1 line
2.30.2
maintainer
#27 lore raw
Quoted 4 lines · Darrick J. Wong
Quoted 2 lines

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?

It should not really require any new tests, as anything buffered + io_uring on xfs will now use this code. But Stefan has run a bunch of things on the side too, some of those synthetic (like ensure that various parts of a buffered write range isn't cached, etc) and some more generic (fsx). There might be some that could be turned into xfstests, I'll let him answer that one.

Quoted 5 lines

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!

Really not sure what's going on with email these days, it's quite a pain... Thanks for taking a look so quickly!

I've added your reviewed-bys and also made that ternary change you suggested. Only other change is addressing a kernelbot noticing that one ret in the mm side was being set to zero only, so we could kill it. End result:

https://git.kernel.dk/cgit/linux-block/log/?h=for-5.20/io_uring-buffered-writes

Signature · 1 line
Jens Axboe
#28 lore raw
In reply to Darrick J. Wong
Quoted 2 lines · Darrick J. Wong

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

I can see a all here. Sometimes it helps to just wait a bit.

maintainer
#29 lore raw
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Darrick J. Wong

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

I can see a all here. Sometimes it helps to just wait a bit.

on lore? I'm still seeing some missing. Which is a bit odd, since eg b4 can pull the series down just fine.

Signature · 1 line
Jens Axboe
#30 lore raw
Quoted 9 lines · Jens Axboe
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Darrick J. Wong

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

I can see a all here. Sometimes it helps to just wait a bit.

on lore? I'm still seeing some missing. Which is a bit odd, since eg b4 can pull the series down just fine.

I'm still seeing some missing on the io-uring lore too:

https://lore.kernel.org/io-uring/[email protected]/ (missing)

But changing the path to /all/ shows the complete series:

https://lore.kernel.org/all/[email protected]/ (complete)

b4 seems to be fetching from /all/.

Signature · 1 line
Ammar Faizi
maintainer
#31 lore raw
Quoted 13 lines · Ammar Faizi
Quoted 9 lines · Jens Axboe
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Darrick J. Wong

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

I can see a all here. Sometimes it helps to just wait a bit.

on lore? I'm still seeing some missing. Which is a bit odd, since eg b4 can pull the series down just fine.

I'm still seeing some missing on the io-uring lore too:

Quoted 7 lines
   https://lore.kernel.org/io-uring/[email protected]/ (missing)

But changing the path to /all/ shows the complete series:

   https://lore.kernel.org/all/[email protected]/ (complete)

b4 seems to be fetching from /all/.

Ah, I see. Konstantin, do you know what is going on here? tldr - /all/ has the whole series, io-uring list does not.

Signature · 1 line
Jens Axboe
maintainer Applied
#32 lore raw

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

In reply to Stefan Roesch
Quoted 9 lines · Stefan Roesch

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.

[...]

Applied, thanks!

[13/14] xfs: Specify lockmode when calling xfs_ilock_for_iomap()
(no commit info)
[14/14] xfs: Add async buffered write support
(no commit info)

Best regards,

Signature · 1 line
Jens Axboe