io-uring/xfs: support async buffered writes

RFC PATCH Superseded Stefan Roesch 4 participants 39 messages May 20, 2022 – May 25, 2022

Subsystems (7)

Memory management VFS Tracing XFS filesystem +3 more subsystems

lore

Share & tools

lore

Stefan Roesch

author

2022-05-20 18:37 UTC #0 lore raw

v4 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 and fio.

Changes:
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: Prepare balance_dirty_pages() for async buffered writes

Stefan Roesch (14):
mm: Add balance_dirty_pages_ratelimited_flags() function
iomap: Add gfp parameter to iomap_page_create()
iomap: Add async buffered write support
iomap: Use balance_dirty_pages_ratelimited_flags in iomap_write_iter
fs: Add check for async buffered writes to generic_write_checks
fs: Split off remove_needs_file_privs() __remove_file_privs()
fs: Split off file_needs_update_time and __file_update_time
fs: Add async write file modification handling.
fs: Optimization for concurrent file time updates.
io_uring: Add support for async buffered writes
io_uring: Add tracepoint for short writes
xfs: Add iomap async buffered write support
xfs: Add async buffered write support
xfs: Enable async buffered write support

 fs/inode.c                      | 178 ++++++++++++++++++++++++--------
 fs/io_uring.c                   |  32 +++++-
 fs/iomap/buffered-io.c          |  54 ++++++++--
 fs/read_write.c                 |   4 +-
 fs/xfs/xfs_file.c               |  36 +++----
 fs/xfs/xfs_iomap.c              |  14 ++-
 include/linux/fs.h              |   7 ++
 include/linux/writeback.h       |   7 ++
 include/trace/events/io_uring.h |  25 +++++
 mm/page-writeback.c             |  86 ++++++++-------
 10 files changed, 318 insertions(+), 125 deletions(-)

base-commit: 0cdd776ec92c0fec768c7079331804d3e52d4b27

Signature · 1 line

2.30.2

Stefan Roesch

author

2022-05-20 18:37 UTC #1 lore raw

RFC PATCH v4 1/17 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 7e2da284e427..8e5e003f0093 100644
@@ -1618,6 +1618,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

@@ -1648,6 +1661,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);

@@ -1814,23 +1828,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

Stefan Roesch

author

2022-05-20 18:37 UTC #2 lore raw

RFC PATCH v4 2/17 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 8e5e003f0093..89dcc7d8395a 100644
@@ -1720,8 +1720,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))

@@ -1825,9 +1825,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

Stefan Roesch

author

2022-05-20 18:37 UTC #3 lore raw

RFC PATCH v4 3/17 mm: Prepare balance_dirty_pages() for async buffered writes

In reply to Stefan Roesch

From: Jan Kara <[email protected]>

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.

Signed-off-by Jan Kara <[email protected]>

Signed-off-by Stefan Roesch <[email protected]>

---
 include/linux/writeback.h |  4 ++++
 mm/page-writeback.c       | 12 +++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

include/linux/writeback.h +4 -0

index fec248ab1fec..a9114c5090e9 100644
@@ -372,6 +372,10 @@ 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);
 bool wb_over_bg_thresh(struct bdi_writeback *wb);

mm/page-writeback.c +9 -3

index 89dcc7d8395a..7a320fd2ad33 100644
@@ -1545,8 +1545,8 @@ static inline void wb_dirty_limits(struct dirty_throttle_control *dtc)
  * If we're over `background_thresh' then the writeback threads are woken to
  * perform some writeout.
  */
-static void balance_dirty_pages(struct bdi_writeback *wb,
-				unsigned long pages_dirtied)
+static int balance_dirty_pages(struct bdi_writeback *wb,
+			       unsigned long pages_dirtied, unsigned int flags)
 {
 	struct dirty_throttle_control gdtc_stor = { GDTC_INIT(wb) };
 	struct dirty_throttle_control mdtc_stor = { MDTC_INIT(wb, &gdtc_stor) };

@@ -1566,6 +1566,7 @@ static void balance_dirty_pages(struct bdi_writeback *wb,
 	struct backing_dev_info *bdi = wb->bdi;
 	bool strictlimit = bdi->capabilities & BDI_CAP_STRICTLIMIT;
 	unsigned long start_time = jiffies;
+	int ret = 0;
 
 	for (;;) {
 		unsigned long now = jiffies;

@@ -1794,6 +1795,10 @@ static void balance_dirty_pages(struct bdi_writeback *wb,
 					  period,
 					  pause,
 					  start_time);
+		if (flags & BDP_ASYNC) {
+			ret = -EAGAIN;
+			break;
+		}
 		__set_current_state(TASK_KILLABLE);
 		wb->dirty_sleep = now;
 		io_schedule_timeout(pause);

@@ -1825,6 +1830,7 @@ static void balance_dirty_pages(struct bdi_writeback *wb,
 		if (fatal_signal_pending(current))
 			break;
 	}
+	return ret;
 }
 
 static DEFINE_PER_CPU(int, bdp_ratelimits);

@@ -1906,7 +1912,7 @@ void balance_dirty_pages_ratelimited(struct address_space *mapping)
 	preempt_enable();
 
 	if (unlikely(current->nr_dirtied >= ratelimit))
-		balance_dirty_pages(wb, current->nr_dirtied);
+		balance_dirty_pages(wb, current->nr_dirtied, 0);
 
 	wb_put(wb);
 }

Signature · 1 line

2.30.2

Christoph Hellwig

2022-05-22 07:17 UTC #4 lore raw

Re: [RFC PATCH v4 03/17] mm: Prepare balance_dirty_pages() for async buffered writes

Quoted 6 lines · Stefan Roesch

From: Jan Kara <[email protected]>
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.

I don't tink this really makes sense without the next patch, so I'd suggest to merge the two.

Stefan Roesch

author

2022-05-25 21:30 UTC #5 lore raw

Re: [RFC PATCH v4 03/17] mm: Prepare balance_dirty_pages() for async buffered writes

Quoted 10 lines · Christoph Hellwig

Quoted 6 lines · Stefan Roesch
From: Jan Kara <[email protected]>
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.
I don't tink this really makes sense without the next patch, so I'd suggest to merge the two.

I merged the two patches.

Stefan Roesch

author

2022-05-20 18:37 UTC #6 lore raw

RFC PATCH v4 4/17 mm: Add balance_dirty_pages_ratelimited_flags() function

In reply to Stefan Roesch

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.

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

Signed-off-by Stefan Roesch <[email protected]>

---
 include/linux/writeback.h |  3 +++
 mm/page-writeback.c       | 38 +++++++++++++++++++++++---------------
 2 files changed, 26 insertions(+), 15 deletions(-)

include/linux/writeback.h +3 -0

index a9114c5090e9..1bddad86a4f6 100644
@@ -377,6 +377,9 @@ void wb_update_bandwidth(struct bdi_writeback *wb);
 #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 +23 ~~-15~~

index 7a320fd2ad33..3701e813d05f 100644
@@ -1851,28 +1851,18 @@ static DEFINE_PER_CPU(int, bdp_ratelimits);
  */
 DEFINE_PER_CPU(int, dirty_throttle_leaks) = 0;
 
-/**
- * balance_dirty_pages_ratelimited - balance dirty memory state
- * @mapping: address_space which was dirtied
- *
- * Processes which are dirtying memory should call in here once for each page
- * which was newly dirtied.  The function will periodically check the system's
- * dirty state and will initiate writeback if needed.
- *
- * Once we're over the dirty memory limit we decrease the ratelimiting
- * by a lot, to prevent individual processes from overshooting the limit
- * by (ratelimit_pages) each.
- */
-void balance_dirty_pages_ratelimited(struct address_space *mapping)
+int balance_dirty_pages_ratelimited_flags(struct address_space *mapping,
+					unsigned int flags)
 {
 	struct inode *inode = mapping->host;
 	struct backing_dev_info *bdi = inode_to_bdi(inode);
 	struct bdi_writeback *wb = NULL;
 	int ratelimit;
+	int ret = 0;
 	int *p;
 
 	if (!(bdi->capabilities & BDI_CAP_WRITEBACK))
-		return;
+		return ret;
 
 	if (inode_cgwb_enabled(inode))
 		wb = wb_get_create_current(bdi, GFP_KERNEL);

@@ -1912,9 +1902,27 @@ void balance_dirty_pages_ratelimited(struct address_space *mapping)
 	preempt_enable();
 
 	if (unlikely(current->nr_dirtied >= ratelimit))
-		balance_dirty_pages(wb, current->nr_dirtied, 0);
+		balance_dirty_pages(wb, current->nr_dirtied, flags);
 
 	wb_put(wb);
+	return ret;
+}
+
+/**
+ * balance_dirty_pages_ratelimited - balance dirty memory state
+ * @mapping: address_space which was dirtied
+ *
+ * Processes which are dirtying memory should call in here once for each page
+ * which was newly dirtied.  The function will periodically check the system's
+ * dirty state and will initiate writeback if needed.
+ *
+ * Once we're over the dirty memory limit we decrease the ratelimiting
+ * by a lot, to prevent individual processes from overshooting the limit
+ * by (ratelimit_pages) each.
+ */
+void balance_dirty_pages_ratelimited(struct address_space *mapping)
+{
+	balance_dirty_pages_ratelimited_flags(mapping, 0);
 }
 EXPORT_SYMBOL(balance_dirty_pages_ratelimited);

Signature · 1 line

2.30.2

Jan Kara

maintainer

2022-05-23 10:41 UTC #7 lore raw

Re: [RFC PATCH v4 04/17] mm: Add balance_dirty_pages_ratelimited_flags() function

Quoted 8 lines · Stefan Roesch

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.
The new helper function is also used by balance_dirty_pages_ratelimited().
Signed-off-by Stefan Roesch <[email protected]>

Yeah, as Christoph says perhaps fold this into patch 3. Otherwise the changes look good to me.

Honza

Quoted 88 lines

---
 include/linux/writeback.h |  3 +++
 mm/page-writeback.c       | 38 +++++++++++++++++++++++---------------
 2 files changed, 26 insertions(+), 15 deletions(-)

include/linux/writeback.h +3 -0

index a9114c5090e9..1bddad86a4f6 100644
@@ -377,6 +377,9 @@ void wb_update_bandwidth(struct bdi_writeback *wb);
 #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 +23 ~~-15~~

index 7a320fd2ad33..3701e813d05f 100644
@@ -1851,28 +1851,18 @@ static DEFINE_PER_CPU(int, bdp_ratelimits);
  */
 DEFINE_PER_CPU(int, dirty_throttle_leaks) = 0;
 
-/**
- * balance_dirty_pages_ratelimited - balance dirty memory state
- * @mapping: address_space which was dirtied
- *
- * Processes which are dirtying memory should call in here once for each page
- * which was newly dirtied.  The function will periodically check the system's
- * dirty state and will initiate writeback if needed.
- *
- * Once we're over the dirty memory limit we decrease the ratelimiting
- * by a lot, to prevent individual processes from overshooting the limit
- * by (ratelimit_pages) each.
- */
-void balance_dirty_pages_ratelimited(struct address_space *mapping)
+int balance_dirty_pages_ratelimited_flags(struct address_space *mapping,
+					unsigned int flags)
 {
 	struct inode *inode = mapping->host;
 	struct backing_dev_info *bdi = inode_to_bdi(inode);
 	struct bdi_writeback *wb = NULL;
 	int ratelimit;
+	int ret = 0;
 	int *p;
 
 	if (!(bdi->capabilities & BDI_CAP_WRITEBACK))
-		return;
+		return ret;
 
 	if (inode_cgwb_enabled(inode))
 		wb = wb_get_create_current(bdi, GFP_KERNEL);

@@ -1912,9 +1902,27 @@ void balance_dirty_pages_ratelimited(struct address_space *mapping)
 	preempt_enable();
 
 	if (unlikely(current->nr_dirtied >= ratelimit))
-		balance_dirty_pages(wb, current->nr_dirtied, 0);
+		balance_dirty_pages(wb, current->nr_dirtied, flags);
 
 	wb_put(wb);
+	return ret;
+}
+
+/**
+ * balance_dirty_pages_ratelimited - balance dirty memory state
+ * @mapping: address_space which was dirtied
+ *
+ * Processes which are dirtying memory should call in here once for each page
+ * which was newly dirtied.  The function will periodically check the system's
+ * dirty state and will initiate writeback if needed.
+ *
+ * Once we're over the dirty memory limit we decrease the ratelimiting
+ * by a lot, to prevent individual processes from overshooting the limit
+ * by (ratelimit_pages) each.
+ */
+void balance_dirty_pages_ratelimited(struct address_space *mapping)
+{
+	balance_dirty_pages_ratelimited_flags(mapping, 0);
 }
 EXPORT_SYMBOL(balance_dirty_pages_ratelimited);

Signature · 1 line

2.30.2

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Stefan Roesch

author

2022-05-20 18:37 UTC #8 lore raw

RFC PATCH v4 5/17 iomap: Add gfp parameter to iomap_page_create()

In reply to Stefan Roesch

Add the gfp flags parameter to the function iomap_page_create() to be able to specify gfp flags.

No intended functional changes in this patch.

Signed-off-by Stefan Roesch <[email protected]>

---
 fs/iomap/buffered-io.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

fs/iomap/buffered-io.c +12 -7

index 8ce8720093b9..27e67bfc64f5 100644
@@ -44,7 +44,7 @@ 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, gfp_t gfp)
 {
 	struct iomap_page *iop = to_iomap_page(folio);
 	unsigned int nr_blocks = i_blocks_per_folio(inode, folio);

@@ -52,8 +52,8 @@ iomap_page_create(struct inode *inode, struct folio *folio)
 	if (iop || nr_blocks <= 1)
 		return iop;
 
-	iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)),
-			GFP_NOFS | __GFP_NOFAIL);
+	iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)), gfp);
+
 	spin_lock_init(&iop->uptodate_lock);
 	if (folio_test_uptodate(folio))
 		bitmap_fill(iop->uptodate, nr_blocks);

@@ -226,7 +226,8 @@ 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,
+					GFP_NOFS | __GFP_NOFAIL);
 	else
 		iop = to_iomap_page(folio);

@@ -264,7 +265,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, GFP_NOFS | __GFP_NOFAIL);
 	iomap_adjust_read_range(iter->inode, folio, &pos, length, &poff, &plen);
 	if (plen == 0)
 		goto done;

@@ -550,17 +551,20 @@ 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 = to_iomap_page(folio);
 	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);
 	size_t from = offset_in_folio(folio, pos), to = from + len;
 	size_t poff, plen;
+	gfp_t  gfp = GFP_NOFS | __GFP_NOFAIL;
 
 	if (folio_test_uptodate(folio))
 		return 0;
 	folio_clear_error(folio);
 
+	iop = iomap_page_create(iter->inode, folio, gfp);
+
 	do {
 		iomap_adjust_read_range(iter->inode, folio, &block_start,
 				block_end - block_start, &poff, &plen);

@@ -1332,7 +1336,8 @@ 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,
+						GFP_NOFS | __GFP_NOFAIL);
 	struct iomap_ioend *ioend, *next;
 	unsigned len = i_blocksize(inode);
 	unsigned nblocks = i_blocks_per_folio(inode, folio);

Signature · 1 line

2.30.2

Christoph Hellwig

2022-05-22 07:21 UTC #9 lore raw

Re: [RFC PATCH v4 05/17] iomap: Add gfp parameter to iomap_page_create()

fs/iomap/buffered-io.c iomap_page_create(struct inode *inode, struct folio *folio) · line 55

Quoted 3 lines · Stefan Roesch

fs/iomap/buffered-io.c +1 -2

-	iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)),
-			GFP_NOFS | __GFP_NOFAIL);
+	iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)), gfp);

Please avoid the overly long line.

Looking at this I also thnk my second suggestion of just passing the flags to iomap_page_create and let iomap_page_create itself pick the gfp_t might be the better option here.

Stefan Roesch

author

2022-05-25 21:30 UTC #10 lore raw

Re: [RFC PATCH v4 05/17] iomap: Add gfp parameter to iomap_page_create()

Quoted 6 lines · Christoph Hellwig

fs/iomap/buffered-io.c iomap_page_create(struct inode *inode, struct folio *folio) · line 55
Quoted 3 lines · Stefan Roesch
fs/iomap/buffered-io.c +1 -2
-	iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)),
-			GFP_NOFS | __GFP_NOFAIL);
+	iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)), gfp);
Please avoid the overly long line.

Fixed.

Quoted 4 lines

Looking at this I also thnk my second suggestion of just passing the flags to iomap_page_create and let iomap_page_create itself pick the gfp_t might be the better option here.

Replaced the gfp paramter with a flags parameter and decide on setting the gfp flags in iomap_page_create.

Stefan Roesch

author

2022-05-20 18:37 UTC #11 lore raw

RFC PATCH v4 6/17 iomap: Add async buffered write support

In reply to Stefan Roesch

This adds async buffered write support to iomap.

Signed-off-by Stefan Roesch <[email protected]>

---
 fs/iomap/buffered-io.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

fs/iomap/buffered-io.c +22 -1

index 27e67bfc64f5..187f4ddd7ba7 100644
@@ -555,15 +555,21 @@ 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;
 	gfp_t  gfp = GFP_NOFS | __GFP_NOFAIL;
 
+	if (iter->flags & IOMAP_NOWAIT)
+		gfp = GFP_NOWAIT;
+
 	if (folio_test_uptodate(folio))
 		return 0;
 	folio_clear_error(folio);
 
 	iop = iomap_page_create(iter->inode, folio, gfp);
+	if ((iter->flags & IOMAP_NOWAIT) && !iop && nr_blocks > 1)
+		return -EAGAIN;
 
 	do {
 		iomap_adjust_read_range(iter->inode, folio, &block_start,

@@ -581,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;

@@ -610,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);

@@ -767,6 +781,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 in the prepare phase of io-uring.
 		 */
 		if (unlikely(fault_in_iov_iter_readable(i, bytes) == bytes)) {
 			status = -EFAULT;

@@ -822,6 +840,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

Stefan Roesch

author

2022-05-20 18:37 UTC #12 lore raw

RFC PATCH v4 7/17 iomap: Use balance_dirty_pages_ratelimited_flags in iomap_write_iter

In reply to Stefan Roesch

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

---
 fs/iomap/buffered-io.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

fs/iomap/buffered-io.c +9 -3

index 187f4ddd7ba7..020452467ca8 100644
@@ -765,14 +765,22 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 	do {
 		struct folio *folio;
 		struct page *page;
+		struct address_space *mapping = iter->inode->i_mapping;
 		unsigned long offset;	/* Offset into pagecache page */
 		unsigned long bytes;	/* Bytes to write to page */
 		size_t copied;		/* Bytes copied from user */
+		unsigned int bdp_flags =
+			(iter->flags & IOMAP_NOWAIT) ? BDP_ASYNC : 0;
 
 		offset = offset_in_page(pos);
 		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;

@@ -796,7 +804,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);

@@ -821,8 +829,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;

Signature · 1 line

2.30.2

Christoph Hellwig

2022-05-22 07:19 UTC #13 lore raw

Re: [RFC PATCH v4 07/17] iomap: Use balance_dirty_pages_ratelimited_flags in iomap_write_iter

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

Quoted 10 lines · Stefan Roesch

fs/iomap/buffered-io.c +3 -0

@@ -765,14 +765,22 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 	do {
 		struct folio *folio;
 		struct page *page;
+		struct address_space *mapping = iter->inode->i_mapping;
 		unsigned long offset;	/* Offset into pagecache page */
 		unsigned long bytes;	/* Bytes to write to page */
 		size_t copied;		/* Bytes copied from user */
+		unsigned int bdp_flags =
+			(iter->flags & IOMAP_NOWAIT) ? BDP_ASYNC : 0;

Bot the mapping and bdp_flags don't change over the loop iterations, so we can initialize them once at the start of the function.

Otherwise this looks good, but I think this should go into the previous patch as it is a central part of supporting async buffered writes.

Stefan Roesch

author

2022-05-25 21:32 UTC #14 lore raw

Re: [RFC PATCH v4 07/17] iomap: Use balance_dirty_pages_ratelimited_flags in iomap_write_iter

Quoted 15 lines · Christoph Hellwig

fs/iomap/buffered-io.c static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) · line 773
Quoted 10 lines · Stefan Roesch
fs/iomap/buffered-io.c +3 -0
@@ -765,14 +765,22 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 	do {
 		struct folio *folio;
 		struct page *page;
+		struct address_space *mapping = iter->inode->i_mapping;
 		unsigned long offset;	/* Offset into pagecache page */
 		unsigned long bytes;	/* Bytes to write to page */
 		size_t copied;		/* Bytes copied from user */
+		unsigned int bdp_flags =
+			(iter->flags & IOMAP_NOWAIT) ? BDP_ASYNC : 0;
Bot the mapping and bdp_flags don't change over the loop iterations, so we can initialize them once at the start of the function.

Moved the variable definitions outside of the loop.

Quoted 4 lines

Otherwise this looks good, but I think this should go into the previous patch as it is a central part of supporting async buffered writes.

I merged it with the previous patch.

Stefan Roesch

author

2022-05-20 18:37 UTC #15 lore raw

RFC PATCH v4 8/17 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]>

---
 fs/read_write.c    | 4 +++-
 include/linux/fs.h | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

fs/read_write.c +3 -1

index e643aec2b0ef..175d98713b9a 100644
@@ -1633,7 +1633,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 bbde95387a23..3b479d02e210 100644
@@ -177,6 +177,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

Stefan Roesch

author

2022-05-20 18:37 UTC #16 lore raw

RFC PATCH v4 9/17 fs: Split off remove_needs_file_privs() __remove_file_privs()

In reply to Stefan Roesch

This splits off the function remove_needs_file_privs() from the function __remove_file_privs() from the function file_remove_privs().

No intended functional changes in this patch.

Signed-off-by Stefan Roesch <[email protected]>

---
 fs/inode.c | 75 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 20 deletions(-)

fs/inode.c +55 ~~-20~~

index 9d9b422504d1..1bb8b7db836f 100644
@@ -2010,17 +2010,8 @@ 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_needs_remove_privs(struct inode *inode, struct dentry *dentry)
 {
-	struct dentry *dentry = file_dentry(file);
-	struct inode *inode = file_inode(file);
-	int kill;
-	int error = 0;
-
 	/*
 	 * Fast path for nothing security related.
 	 * As well for non-regular files, e.g. blkdev inodes.

@@ -2030,16 +2021,42 @@ int file_remove_privs(struct file *file)
 	if (IS_NOSEC(inode) || !S_ISREG(inode->i_mode))
 		return 0;
 
-	kill = dentry_needs_remove_privs(dentry);
-	if (kill < 0)
-		return kill;
-	if (kill)
-		error = __remove_privs(file_mnt_user_ns(file), dentry, kill);
+	return dentry_needs_remove_privs(dentry);
+}
+
+static int __file_remove_privs(struct file *file, struct inode *inode,
+			struct dentry *dentry, int kill)
+{
+	int error = 0;
+
+	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)
+{
+	struct dentry *dentry = file_dentry(file);
+	struct inode *inode = file_inode(file);
+	int kill;
+
+	kill = file_needs_remove_privs(inode, dentry);
+	if (kill <= 0)
+		return kill;
+
+	return __file_remove_privs(file, inode, dentry, kill);
+}
 EXPORT_SYMBOL(file_remove_privs);
 
 /**

@@ -2090,18 +2107,36 @@ 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;
+	struct dentry *dentry = file_dentry(file);
+	struct inode *inode = file_inode(file);
 
 	/*
 	 * 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_needs_remove_privs(inode, dentry);
+	if (ret < 0)
+		return ret;
+
+	if (ret > 0) {
+		ret = __file_remove_privs(file, inode, dentry, ret);
+		if (ret)
+			return ret;
+	}
 
 	if (unlikely(file->f_mode & FMODE_NOCMTIME))
 		return 0;

Signature · 1 line

2.30.2

Christoph Hellwig

2022-05-22 07:32 UTC #17 lore raw

Re: [RFC PATCH v4 09/17] fs: Split off remove_needs_file_privs() __remove_file_privs()

So why don't we just define a __file_remove_privs that gets the iocb flags passed an an extra argument, and then make file_remove_privs pass 0 flags, and maybe also add a kiocb_remove_privs wrapper for callers that have the kiocb. Same for the timestamp update, btw. That seems less churn and less code overall.

Stefan Roesch

author

2022-05-25 21:34 UTC #18 lore raw

Re: [RFC PATCH v4 09/17] fs: Split off remove_needs_file_privs() __remove_file_privs()

Quoted 5 lines · Christoph Hellwig

So why don't we just define a __file_remove_privs that gets the iocb flags passed an an extra argument, and then make file_remove_privs pass 0 flags, and maybe also add a kiocb_remove_privs wrapper for callers that have the kiocb. Same for the timestamp update, btw. That seems less churn and less code overall.

I introduced a __file_remove_privs() function with an additional parameter
that takes the iocb_flags.

Stefan Roesch

author

2022-05-20 18:37 UTC #19 lore raw

RFC PATCH v4 10/17 fs: Split off file_needs_update_time and __file_update_time

In reply to Stefan Roesch

This splits off the functions file_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]>

---
 fs/inode.c | 75 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 26 deletions(-)

fs/inode.c +49 ~~-26~~

index 1bb8b7db836f..4bb7f583cc6b 100644
@@ -2059,35 +2059,19 @@ 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 file_needs_update_time(struct inode *inode, struct file *file,
+				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))

@@ -2096,15 +2080,49 @@ 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 inode *inode, struct file *file,
+			struct timespec64 *now, int sync_mode)
+{
+	int ret = 0;
 
-	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 = file_needs_update_time(inode, file, &now);
+	if (ret <= 0)
+		return ret;
+
+	return __file_update_time(inode, file, &now, ret);
+}
 EXPORT_SYMBOL(file_update_time);
 
 /**

@@ -2123,6 +2141,7 @@ int file_modified(struct file *file)
 	int ret;
 	struct dentry *dentry = file_dentry(file);
 	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.

@@ -2141,7 +2160,11 @@ int file_modified(struct file *file)
 	if (unlikely(file->f_mode & FMODE_NOCMTIME))
 		return 0;
 
-	return file_update_time(file);
+	ret = file_needs_update_time(inode, file, &now);
+	if (ret <= 0)
+		return ret;
+
+	return __file_update_time(inode, file, &now, ret);
 }
 EXPORT_SYMBOL(file_modified);

Signature · 1 line

2.30.2

Christoph Hellwig

2022-05-22 07:29 UTC #20 lore raw

Re: [RFC PATCH v4 10/17] fs: Split off file_needs_update_time and __file_update_time

fs/inode.c int file_remove_privs(struct file *file) · line 2065

Quoted 4 lines · Stefan Roesch

fs/inode.c +2 -0

+static int file_needs_update_time(struct inode *inode, struct file *file,
+				struct timespec64 *now)
 {
 	int sync_it = 0;

No need to pass both and inode and a file as the former can be trivially derived from the latter. But if I'm not misreading the patch, file is entirely unused here anyway, so can't we just drop it and rename the function to inode_needs_update_time?

fs/inode.c int file_update_time(struct file *file) · line 2095

Quoted 10 lines

fs/inode.c +9 -0

+static int __file_update_time(struct inode *inode, struct file *file,
+			struct timespec64 *now, int sync_mode)
+{
+	int ret = 0;
 
+	/* try to update time settings */
+	if (!__mnt_want_write_file(file)) {
+		ret = inode_update_time(inode, now, sync_mode);
+		__mnt_drop_write_file(file);
+	}

I'd be tempted to just open code this in the two callers, but either way works for me. If we keep the function please don't pass the inode separately.

Stefan Roesch

author

2022-05-25 22:33 UTC #21 lore raw

Re: [RFC PATCH v4 10/17] fs: Split off file_needs_update_time and __file_update_time

Quoted 11 lines · Christoph Hellwig

fs/inode.c int file_remove_privs(struct file *file) · line 2065
Quoted 4 lines · Stefan Roesch
fs/inode.c +2 -0
+static int file_needs_update_time(struct inode *inode, struct file *file,
+				struct timespec64 *now)
 {
 	int sync_it = 0;
No need to pass both and inode and a file as the former can be trivially derived from the latter. But if I'm not misreading the patch, file is entirely unused here anyway, so can't we just drop it and rename the function to inode_needs_update_time?

I renamed the function to inode_needs_update_time and only pass the inode pointer.

Quoted 5 lines

fs/inode.c +4 -0

+static int __file_update_time(struct inode *inode, struct file *file,
+			struct timespec64 *now, int sync_mode)
+{
+	int ret = 0;

Quoted 9 lines

fs/inode.c int file_update_time(struct file *file) · line 2095
Quoted 5 lines
fs/inode.c +5 -0
+	/* try to update time settings */
+	if (!__mnt_want_write_file(file)) {
+		ret = inode_update_time(inode, now, sync_mode);
+		__mnt_drop_write_file(file);
+	}
I'd be tempted to just open code this in the two callers, but either way works for me. If we keep the function please don't pass the inode separately.

The inode is no longer passed in.

Stefan Roesch

author

2022-05-20 18:37 UTC #22 lore raw

RFC PATCH v4 11/17 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]>

---
 fs/inode.c         | 25 ++++++++++++++++++++++++-
 fs/xfs/xfs_file.c  |  2 +-
 include/linux/fs.h |  1 +
 3 files changed, 26 insertions(+), 2 deletions(-)

fs/inode.c +24 -1

index 4bb7f583cc6b..3a5d0fa468ab 100644
@@ -2137,6 +2137,27 @@ EXPORT_SYMBOL(file_update_time);
  * Return: 0 on success, negative errno on failure.
  */
 int file_modified(struct file *file)
+{
+	return file_modified_async(file, 0);
+}
+EXPORT_SYMBOL(file_modified);
+
+/**
+ * file_modified_async - 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_async(struct file *file, int flags)
 {
 	int ret;
 	struct dentry *dentry = file_dentry(file);

@@ -2163,10 +2184,12 @@ int file_modified(struct file *file)
 	ret = file_needs_update_time(inode, file, &now);
 	if (ret <= 0)
 		return ret;
+	if (flags & IOCB_NOWAIT)
+		return -EAGAIN;
 
 	return __file_update_time(inode, file, &now, ret);
 }
-EXPORT_SYMBOL(file_modified);
+EXPORT_SYMBOL(file_modified_async);
 
 int inode_needs_sync(struct inode *inode)
 {

fs/xfs/xfs_file.c +1 -1

index 5bddb1e9e0b3..793918c83755 100644
@@ -410,7 +410,7 @@ xfs_file_write_checks(
 		spin_unlock(&ip->i_flags_lock);
 
 out:
-	return file_modified(file);
+	return file_modified_async(file, iocb->ki_flags);
 }
 
 static int

include/linux/fs.h +1 -0

index 3b479d02e210..9760283af7dc 100644
@@ -2382,6 +2382,7 @@ static inline void file_accessed(struct file *file)
 }
 
 extern int file_modified(struct file *file);
+extern int file_modified_async(struct file *file, int flags);
 
 int sync_inode_metadata(struct inode *inode, int wait);

Signature · 1 line

2.30.2

Christoph Hellwig

2022-05-22 07:24 UTC #23 lore raw

Re: [RFC PATCH v4 11/17] fs: Add async write file modification handling.

Quoted 1 line

+int file_modified_async(struct file *file, int flags)

file_modified_flags? Or even bette kiocb_modified and pass the kiocb as that makes is very clear which flags we use by making that implicit.

Quoted 1 line

+EXPORT_SYMBOL(file_modified_async);

EXPORT_SYMBOL_GPL, please.

fs/xfs/xfs_file.c xfs_file_write_checks( · line 413

Quoted 2 lines

fs/xfs/xfs_file.c +1 -1

-	return file_modified(file);
+	return file_modified_async(file, iocb->ki_flags);

And this should go into the XFS enablemnt patch.

Quoted 2 lines

extern int file_modified(struct file *file);
+extern int file_modified_async(struct file *file, int flags);

No need for the extern here, or any function declarations for that matter.

Stefan Roesch

author

2022-05-25 21:40 UTC #24 lore raw

Re: [RFC PATCH v4 11/17] fs: Add async write file modification handling.

Quoted 6 lines · Christoph Hellwig

Quoted 1 line
+int file_modified_async(struct file *file, int flags)
file_modified_flags? Or even bette kiocb_modified and pass the kiocb as that makes is very clear which flags we use by making that implicit.

- Renamed file_modified_async() to file_modified_flags()
- Made file_modified_flags() an internal function
- Add kiocb_modified function.

Quoted 3 lines

Quoted 1 line
+EXPORT_SYMBOL(file_modified_async);
EXPORT_SYMBOL_GPL, please.

Done.

Quoted 11 lines

fs/xfs/xfs_file.c xfs_file_write_checks( · line 413
Quoted 3 lines
fs/xfs/xfs_file.c +1 -1
-	return file_modified(file);
+	return file_modified_async(file, iocb->ki_flags);
And this should go into the XFS enablemnt patch.
Quoted 2 lines
extern int file_modified(struct file *file);
+extern int file_modified_async(struct file *file, int flags);
No need for the extern here, or any function declarations for that matter.

- Removed extern keyword in file_modified_async definition

Stefan Roesch

author

2022-05-20 18:37 UTC #25 lore raw

RFC PATCH v4 12/17 fs: Optimization for concurrent file time updates.

In reply to Stefan Roesch

This introduces the S_PENDING_TIME flag. If an async buffered write needs to update the time, it cannot be processed in the fast path of io-uring. When a time update is pending this flag is set for async buffered writes. Other concurrent async buffered writes for the same file do not need to wait while this time update is pending.

This reduces the number of async buffered writes that need to get punted to the io-workers in io-uring.

Signed-off-by Stefan Roesch <[email protected]>

---
 fs/inode.c         | 11 +++++++++--
 include/linux/fs.h |  3 +++
 2 files changed, 12 insertions(+), 2 deletions(-)

fs/inode.c +9 -2

index 3a5d0fa468ab..5c5021787780 100644
@@ -2184,10 +2184,17 @@ int file_modified_async(struct file *file, int flags)
 	ret = file_needs_update_time(inode, file, &now);
 	if (ret <= 0)
 		return ret;
-	if (flags & IOCB_NOWAIT)
+	if (flags & IOCB_NOWAIT) {
+		if (IS_PENDING_TIME(inode))
+			return 0;
+
+		inode->i_flags |= S_PENDING_TIME;
 		return -EAGAIN;
+	}
 
-	return __file_update_time(inode, file, &now, ret);
+	ret = __file_update_time(inode, file, &now, ret);
+	inode->i_flags &= ~S_PENDING_TIME;
+	return ret;
 }
 EXPORT_SYMBOL(file_modified_async);

include/linux/fs.h +3 -0

index 9760283af7dc..5f3aaf61fb4b 100644
@@ -2141,6 +2141,8 @@ struct super_operations {
 #define S_CASEFOLD	(1 << 15) /* Casefolded file */
 #define S_VERITY	(1 << 16) /* Verity file (using fs/verity/) */
 #define S_KERNEL_FILE	(1 << 17) /* File is in use by the kernel (eg. fs/cachefiles) */
+#define S_PENDING_TIME (1 << 18) /* File update time is pending */
+
 
 /*
  * Note that nosuid etc flags are inode-specific: setting some file-system

@@ -2183,6 +2185,7 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags
 #define IS_ENCRYPTED(inode)	((inode)->i_flags & S_ENCRYPTED)
 #define IS_CASEFOLDED(inode)	((inode)->i_flags & S_CASEFOLD)
 #define IS_VERITY(inode)	((inode)->i_flags & S_VERITY)
+#define IS_PENDING_TIME(inode) ((inode)->i_flags & S_PENDING_TIME)
 
 #define IS_WHITEOUT(inode)	(S_ISCHR(inode->i_mode) && \
 				 (inode)->i_rdev == WHITEOUT_DEV)

Signature · 1 line

2.30.2

Jan Kara

maintainer

2022-05-23 10:51 UTC #26 lore raw

Re: [RFC PATCH v4 12/17] fs: Optimization for concurrent file time updates.

Quoted 10 lines · Stefan Roesch

This introduces the S_PENDING_TIME flag. If an async buffered write needs to update the time, it cannot be processed in the fast path of io-uring. When a time update is pending this flag is set for async buffered writes. Other concurrent async buffered writes for the same file do not need to wait while this time update is pending.
This reduces the number of async buffered writes that need to get punted to the io-workers in io-uring.
Signed-off-by Stefan Roesch <[email protected]>

...

Quoted 9 lines

fs/inode.c +4 -1

@@ -2184,10 +2184,17 @@ int file_modified_async(struct file *file, int flags)
 	ret = file_needs_update_time(inode, file, &now);
 	if (ret <= 0)
 		return ret;
-	if (flags & IOCB_NOWAIT)
+	if (flags & IOCB_NOWAIT) {
+		if (IS_PENDING_TIME(inode))
+			return 0;
+

fs/inode.c int file_modified_async(struct file *file, int flags) · line 2199

Quoted 10 lines

fs/inode.c +5 -1

+		inode->i_flags |= S_PENDING_TIME;
 		return -EAGAIN;
+	}
 
-	return __file_update_time(inode, file, &now, ret);
+	ret = __file_update_time(inode, file, &now, ret);
+	inode->i_flags &= ~S_PENDING_TIME;
+	return ret;
 }
 EXPORT_SYMBOL(file_modified_async);

You still didn't address my concern that i_flags is modified without the protection of i_rwsem here. That can lead to corruption of i_flags value and rather nasty (and hard to debug) consequences. You can use inode_set_flags() here to make things kinda safe. The whole inode->i_flags handling is a mess but not yours to resolve ;)

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Stefan Roesch

author

2022-05-25 21:46 UTC #27 lore raw

Re: [RFC PATCH v4 12/17] fs: Optimization for concurrent file time updates.

Quoted 27 lines · Jan Kara

Quoted 10 lines · Stefan Roesch
This introduces the S_PENDING_TIME flag. If an async buffered write needs to update the time, it cannot be processed in the fast path of io-uring. When a time update is pending this flag is set for async buffered writes. Other concurrent async buffered writes for the same file do not need to wait while this time update is pending.
This reduces the number of async buffered writes that need to get punted to the io-workers in io-uring.
Signed-off-by Stefan Roesch <[email protected]>
...
Quoted 13 lines
fs/inode.c +6 -1
@@ -2184,10 +2184,17 @@ int file_modified_async(struct file *file, int flags)
 	ret = file_needs_update_time(inode, file, &now);
 	if (ret <= 0)
 		return ret;
-	if (flags & IOCB_NOWAIT)
+	if (flags & IOCB_NOWAIT) {
+		if (IS_PENDING_TIME(inode))
+			return 0;
+
+		inode->i_flags |= S_PENDING_TIME;
 		return -EAGAIN;
+	}
 

Quoted 13 lines

fs/inode.c int file_modified_async(struct file *file, int flags) · line 2199
Quoted 6 lines
fs/inode.c +3 -1
-	return __file_update_time(inode, file, &now, ret);
+	ret = __file_update_time(inode, file, &now, ret);
+	inode->i_flags &= ~S_PENDING_TIME;
+	return ret;
 }
 EXPORT_SYMBOL(file_modified_async);
You still didn't address my concern that i_flags is modified without the protection of i_rwsem here. That can lead to corruption of i_flags value and rather nasty (and hard to debug) consequences. You can use inode_set_flags() here to make things kinda safe. The whole inode->i_flags handling is a mess but not yours to resolve ;)

Replaced directly manipulating the inode flags with calls to the function inode_set_flags().

Quoted 1 line

Honza

Stefan Roesch

author

2022-05-20 18:37 UTC #28 lore raw

RFC PATCH v4 13/17 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 91de361ea9ab..f3aaac286509 100644
@@ -3746,7 +3746,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);

@@ -3875,7 +3875,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 	} else if (ret == -EIOCBQUEUED) {
 		goto out_free;
 	} else if (ret == req->result || 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;
 	}

@@ -3971,9 +3971,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;

@@ -4027,6 +4028,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->result && 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

Stefan Roesch

author

2022-05-20 18:37 UTC #29 lore raw

RFC PATCH v4 14/17 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 f3aaac286509..7435a9c2007f 100644
@@ -4032,6 +4032,9 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 		if (ret2 != req->result && ret2 >= 0 && need_complete_io(req)) {
 			struct io_async_rw *rw;
 
+			trace_io_uring_short_write(req->ctx, kiocb->ki_pos - ret2,
+						req->result, 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 cddf5b6fbeb4..661834361d33 100644
@@ -543,6 +543,31 @@ TRACE_EVENT(io_uring_req_failed,
 		  (unsigned long long) __entry->pad2, __entry->error)
 );
 
+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

Stefan Roesch

author

2022-05-20 18:37 UTC #30 lore raw

RFC PATCH v4 15/17 xfs: Add iomap async buffered write support

In reply to Stefan Roesch

This adds the async buffered write support to the iomap layer of XFS. If a lock cannot be acquired or additional reads need to be performed, the request will return -EAGAIN in case this is an async buffered write request.

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

Signed-off-by Stefan Roesch <[email protected]>

---
 fs/xfs/xfs_iomap.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

fs/xfs/xfs_iomap.c +9 -5

index e552ce541ec2..1aea962262ad 100644
@@ -659,7 +659,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);
 
 	/*

@@ -737,7 +737,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));

@@ -881,18 +881,22 @@ 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;
 
 	/* we can't use delayed allocations when using extent size hints */
-	if (xfs_get_extsz_hint(ip))
+	if (xfs_get_extsz_hint(ip)) {
 		return xfs_direct_write_iomap_begin(inode, offset, count,
 				flags, iomap, srcmap);
+	}
 
 	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)) {

@@ -1167,7 +1171,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

Christoph Hellwig

2022-05-22 07:33 UTC #31 lore raw

Re: [RFC PATCH v4 15/17] xfs: Add iomap async buffered write support

Quoted 7 lines · Stefan Roesch

This adds the async buffered write support to the iomap layer of XFS. If a lock cannot be acquired or additional reads need to be performed, the request will return -EAGAIN in case this is an async buffered write request.
This patch changes the helper function xfs_ilock_for_iomap such that the lock mode needs to be passed in.

So from a pure commit structure POV, I'd make the changes to xfs_ilock_for_iomap one separate and clearly document patch, and then merge all the rest of the XFS enablement into a single other patch.

Stefan Roesch

author

2022-05-25 21:41 UTC #32 lore raw

Re: [RFC PATCH v4 15/17] xfs: Add iomap async buffered write support

Quoted 6 lines · Christoph Hellwig

Quoted 5 lines · Stefan Roesch
This adds the async buffered write support to the iomap layer of XFS. If a lock cannot be acquired or additional reads need to be performed, the request will return -EAGAIN in case this is an async buffered write request.

Quoted 6 lines

Quoted 2 lines
This patch changes the helper function xfs_ilock_for_iomap such that the lock mode needs to be passed in.
So from a pure commit structure POV, I'd make the changes to xfs_ilock_for_iomap one separate and clearly document patch, and then merge all the rest of the XFS enablement into a single other patch.

Done.

Stefan Roesch

author

2022-05-20 18:38 UTC #33 lore raw

RFC PATCH v4 16/17 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.

This splits off a new helper xfs_ilock_inode from the existing helper xfs_ilock_iocb so it can be used for this function. The exising helper cannot be used as it hardcoded the inode to be used.

Signed-off-by Stefan Roesch <[email protected]>

---
 fs/xfs/xfs_file.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

fs/xfs/xfs_file.c +15 ~~-17~~

index 793918c83755..ad3175b7d366 100644
@@ -190,14 +190,13 @@ xfs_file_fsync(
 	return error;
 }
 
-static int
-xfs_ilock_iocb(
-	struct kiocb		*iocb,
+static inline int
+xfs_ilock_xfs_inode(
+	struct xfs_inode	*ip,
+	int			flags,
 	unsigned int		lock_mode)
 {
-	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
-
-	if (iocb->ki_flags & IOCB_NOWAIT) {
+	if (flags & IOCB_NOWAIT) {
 		if (!xfs_ilock_nowait(ip, lock_mode))
 			return -EAGAIN;
 	} else {

@@ -222,7 +221,7 @@ xfs_file_dio_read(
 
 	file_accessed(iocb->ki_filp);
 
-	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, XFS_IOLOCK_SHARED);
 	if (ret)
 		return ret;
 	ret = iomap_dio_rw(iocb, to, &xfs_read_iomap_ops, NULL, 0, 0);

@@ -244,7 +243,7 @@ xfs_file_dax_read(
 	if (!iov_iter_count(to))
 		return 0; /* skip atime */
 
-	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, XFS_IOLOCK_SHARED);
 	if (ret)
 		return ret;
 	ret = dax_iomap_rw(iocb, to, &xfs_read_iomap_ops);

@@ -264,7 +263,7 @@ xfs_file_buffered_read(
 
 	trace_xfs_file_buffered_read(iocb, to);
 
-	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, XFS_IOLOCK_SHARED);
 	if (ret)
 		return ret;
 	ret = generic_file_read_iter(iocb, to);

@@ -343,7 +342,7 @@ xfs_file_write_checks(
 	if (*iolock == XFS_IOLOCK_SHARED && !IS_NOSEC(inode)) {
 		xfs_iunlock(ip, *iolock);
 		*iolock = XFS_IOLOCK_EXCL;
-		error = xfs_ilock_iocb(iocb, *iolock);
+		error = xfs_ilock_xfs_inode(ip, iocb->ki_flags, *iolock);
 		if (error) {
 			*iolock = 0;
 			return error;

@@ -516,7 +515,7 @@ xfs_file_dio_write_aligned(
 	int			iolock = XFS_IOLOCK_SHARED;
 	ssize_t			ret;
 
-	ret = xfs_ilock_iocb(iocb, iolock);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, iolock);
 	if (ret)
 		return ret;
 	ret = xfs_file_write_checks(iocb, from, &iolock);

@@ -583,7 +582,7 @@ xfs_file_dio_write_unaligned(
 		flags = IOMAP_DIO_FORCE_WAIT;
 	}
 
-	ret = xfs_ilock_iocb(iocb, iolock);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, iolock);
 	if (ret)
 		return ret;

@@ -659,7 +658,7 @@ xfs_file_dax_write(
 	ssize_t			ret, error = 0;
 	loff_t			pos;
 
-	ret = xfs_ilock_iocb(iocb, iolock);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, iolock);
 	if (ret)
 		return ret;
 	ret = xfs_file_write_checks(iocb, from, &iolock);

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

Signature · 1 line

2.30.2

Christoph Hellwig

2022-05-22 07:35 UTC #34 lore raw

Re: [RFC PATCH v4 16/17] xfs: Add async buffered write support

Quoted 7 lines · 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.
This splits off a new helper xfs_ilock_inode from the existing helper xfs_ilock_iocb so it can be used for this function. The exising helper cannot be used as it hardcoded the inode to be used.

Actually this should also be a prep patch - but the please try to follow the rule that standalone enablement like refactoring or new funtionality for helpers is one patch, the actual use of it for a new feature should preferably one patch for the whole feature.

Stefan Roesch

author

2022-05-25 21:42 UTC #35 lore raw

Re: [RFC PATCH v4 16/17] xfs: Add async buffered write support

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · 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.

Quoted 8 lines

Quoted 3 lines
This splits off a new helper xfs_ilock_inode from the existing helper xfs_ilock_iocb so it can be used for this function. The exising helper cannot be used as it hardcoded the inode to be used.
Actually this should also be a prep patch - but the please try to follow the rule that standalone enablement like refactoring or new funtionality for helpers is one patch, the actual use of it for a new feature should preferably one patch for the whole feature.

Made a separate patch for changes to Separate patch for changes to xfs_ilock_inode().

Darrick J. Wong

2022-05-22 15:35 UTC #36 lore raw

Re: [RFC PATCH v4 16/17] xfs: Add async buffered write support

In reply to Stefan Roesch

Quoted 18 lines · 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.
This splits off a new helper xfs_ilock_inode from the existing helper xfs_ilock_iocb so it can be used for this function. The exising helper cannot be used as it hardcoded the inode to be used.
Signed-off-by Stefan Roesch <[email protected]>
---
 fs/xfs/xfs_file.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)
fs/xfs/xfs_file.c +0 -0
index 793918c83755..ad3175b7d366 100644

fs/xfs/xfs_file.c xfs_file_fsync( · line 194

Quoted 8 lines

fs/xfs/xfs_file.c +2 -3

@@ -190,14 +190,13 @@ xfs_file_fsync(
 	return error;
 }
 
-static int
-xfs_ilock_iocb(
-	struct kiocb		*iocb,
+static inline int
+xfs_ilock_xfs_inode(

A couple of nitpicky things:

"ilock" is shorthand for "inode lock", which means this name expands to "xfs inode lock xfs inode", which is redundant. Seeing as the whole point of this function is to take a particular inode lock with a particular set of IOCB flags, just leave the name as it was.

fs/xfs/xfs_file.c xfs_file_fsync( · line 196

Quoted 2 lines

fs/xfs/xfs_file.c +2 -0

+	struct xfs_inode	*ip,
+	int			flags,

"iocb_flags", not "flags", to reinforce what kind of flags are supposed to be passed in here.

--D

Quoted 91 lines

fs/xfs/xfs_file.c +11 ~~-14~~

 	unsigned int		lock_mode)
 {
-	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
-
-	if (iocb->ki_flags & IOCB_NOWAIT) {
+	if (flags & IOCB_NOWAIT) {
 		if (!xfs_ilock_nowait(ip, lock_mode))
 			return -EAGAIN;
 	} else {

@@ -222,7 +221,7 @@ xfs_file_dio_read(
 
 	file_accessed(iocb->ki_filp);
 
-	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, XFS_IOLOCK_SHARED);
 	if (ret)
 		return ret;
 	ret = iomap_dio_rw(iocb, to, &xfs_read_iomap_ops, NULL, 0, 0);

@@ -244,7 +243,7 @@ xfs_file_dax_read(
 	if (!iov_iter_count(to))
 		return 0; /* skip atime */
 
-	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, XFS_IOLOCK_SHARED);
 	if (ret)
 		return ret;
 	ret = dax_iomap_rw(iocb, to, &xfs_read_iomap_ops);

@@ -264,7 +263,7 @@ xfs_file_buffered_read(
 
 	trace_xfs_file_buffered_read(iocb, to);
 
-	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, XFS_IOLOCK_SHARED);
 	if (ret)
 		return ret;
 	ret = generic_file_read_iter(iocb, to);

@@ -343,7 +342,7 @@ xfs_file_write_checks(
 	if (*iolock == XFS_IOLOCK_SHARED && !IS_NOSEC(inode)) {
 		xfs_iunlock(ip, *iolock);
 		*iolock = XFS_IOLOCK_EXCL;
-		error = xfs_ilock_iocb(iocb, *iolock);
+		error = xfs_ilock_xfs_inode(ip, iocb->ki_flags, *iolock);
 		if (error) {
 			*iolock = 0;
 			return error;

@@ -516,7 +515,7 @@ xfs_file_dio_write_aligned(
 	int			iolock = XFS_IOLOCK_SHARED;
 	ssize_t			ret;
 
-	ret = xfs_ilock_iocb(iocb, iolock);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, iolock);
 	if (ret)
 		return ret;
 	ret = xfs_file_write_checks(iocb, from, &iolock);

@@ -583,7 +582,7 @@ xfs_file_dio_write_unaligned(
 		flags = IOMAP_DIO_FORCE_WAIT;
 	}
 
-	ret = xfs_ilock_iocb(iocb, iolock);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, iolock);
 	if (ret)
 		return ret;

@@ -659,7 +658,7 @@ xfs_file_dax_write(
 	ssize_t			ret, error = 0;
 	loff_t			pos;
 
-	ret = xfs_ilock_iocb(iocb, iolock);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, iolock);
 	if (ret)
 		return ret;
 	ret = xfs_file_write_checks(iocb, from, &iolock);

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

Signature · 1 line

2.30.2

Stefan Roesch

author

2022-05-25 21:44 UTC #37 lore raw

Re: [RFC PATCH v4 16/17] xfs: Add async buffered write support

Quoted 30 lines · Darrick J. Wong

Quoted 18 lines · 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.
This splits off a new helper xfs_ilock_inode from the existing helper xfs_ilock_iocb so it can be used for this function. The exising helper cannot be used as it hardcoded the inode to be used.
Signed-off-by Stefan Roesch <[email protected]>
---
 fs/xfs/xfs_file.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)
fs/xfs/xfs_file.c +0 -0
index 793918c83755..ad3175b7d366 100644
fs/xfs/xfs_file.c xfs_file_fsync( · line 194
Quoted 8 lines
fs/xfs/xfs_file.c +2 -3
@@ -190,14 +190,13 @@ xfs_file_fsync(
 	return error;
 }
 
-static int
-xfs_ilock_iocb(
-	struct kiocb		*iocb,
+static inline int
+xfs_ilock_xfs_inode(
A couple of nitpicky things:

Quoted 5 lines

"ilock" is shorthand for "inode lock", which means this name expands to "xfs inode lock xfs inode", which is redundant. Seeing as the whole point of this function is to take a particular inode lock with a particular set of IOCB flags, just leave the name as it was.

Renamed xfs_ilock_xfs_inode()n back to xfs_ilock_iocb().

Quoted 6 lines

fs/xfs/xfs_file.c xfs_file_fsync( · line 196
Quoted 2 lines
fs/xfs/xfs_file.c +2 -0
+	struct xfs_inode	*ip,
+	int			flags,
"iocb_flags", not "flags", to reinforce what kind of flags are supposed to be passed in here.

Renamed flags parameter to iocb_flags in function xfs_ilock_iocb().

Quoted 93 lines

--D

Quoted 91 lines

fs/xfs/xfs_file.c +11 ~~-14~~

 	unsigned int		lock_mode)
 {
-	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
-
-	if (iocb->ki_flags & IOCB_NOWAIT) {
+	if (flags & IOCB_NOWAIT) {
 		if (!xfs_ilock_nowait(ip, lock_mode))
 			return -EAGAIN;
 	} else {

@@ -222,7 +221,7 @@ xfs_file_dio_read(
 
 	file_accessed(iocb->ki_filp);
 
-	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, XFS_IOLOCK_SHARED);
 	if (ret)
 		return ret;
 	ret = iomap_dio_rw(iocb, to, &xfs_read_iomap_ops, NULL, 0, 0);

@@ -244,7 +243,7 @@ xfs_file_dax_read(
 	if (!iov_iter_count(to))
 		return 0; /* skip atime */
 
-	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, XFS_IOLOCK_SHARED);
 	if (ret)
 		return ret;
 	ret = dax_iomap_rw(iocb, to, &xfs_read_iomap_ops);

@@ -264,7 +263,7 @@ xfs_file_buffered_read(
 
 	trace_xfs_file_buffered_read(iocb, to);
 
-	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, XFS_IOLOCK_SHARED);
 	if (ret)
 		return ret;
 	ret = generic_file_read_iter(iocb, to);

@@ -343,7 +342,7 @@ xfs_file_write_checks(
 	if (*iolock == XFS_IOLOCK_SHARED && !IS_NOSEC(inode)) {
 		xfs_iunlock(ip, *iolock);
 		*iolock = XFS_IOLOCK_EXCL;
-		error = xfs_ilock_iocb(iocb, *iolock);
+		error = xfs_ilock_xfs_inode(ip, iocb->ki_flags, *iolock);
 		if (error) {
 			*iolock = 0;
 			return error;

@@ -516,7 +515,7 @@ xfs_file_dio_write_aligned(
 	int			iolock = XFS_IOLOCK_SHARED;
 	ssize_t			ret;
 
-	ret = xfs_ilock_iocb(iocb, iolock);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, iolock);
 	if (ret)
 		return ret;
 	ret = xfs_file_write_checks(iocb, from, &iolock);

@@ -583,7 +582,7 @@ xfs_file_dio_write_unaligned(
 		flags = IOMAP_DIO_FORCE_WAIT;
 	}
 
-	ret = xfs_ilock_iocb(iocb, iolock);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, iolock);
 	if (ret)
 		return ret;

@@ -659,7 +658,7 @@ xfs_file_dax_write(
 	ssize_t			ret, error = 0;
 	loff_t			pos;
 
-	ret = xfs_ilock_iocb(iocb, iolock);
+	ret = xfs_ilock_xfs_inode(ip, iocb->ki_flags, iolock);
 	if (ret)
 		return ret;
 	ret = xfs_file_write_checks(iocb, from, &iolock);

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

Signature · 1 line

2.30.2

Stefan Roesch

author

2022-05-20 18:37 UTC #38 lore raw

RFC PATCH v4 17/17 xfs: Enable async buffered write support

In reply to Stefan Roesch

This turns on the async buffered write support for XFS.

Signed-off-by Stefan Roesch <[email protected]>

---
 fs/xfs/xfs_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/xfs/xfs_file.c +1 -1

index ad3175b7d366..af4fdc852da5 100644
@@ -1169,7 +1169,7 @@ xfs_file_open(
 		return -EFBIG;
 	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 0;
 }

Signature · 1 line

2.30.2