io-uring/xfs: support async buffered writes

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

---
 fs/iomap/buffered-io.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

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

index d2a9f699e17e..705f80cd2d4e 100644
@@ -44,16 +44,23 @@ 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);

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

+
 	iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)),
-			GFP_NOFS | __GFP_NOFAIL);
+		      gfp);
+
 	spin_lock_init(&iop->uptodate_lock);
 	if (folio_test_uptodate(folio))
 		bitmap_fill(iop->uptodate, nr_blocks);

@@ -226,7 +233,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 +271,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 +554,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 +565,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 +1338,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

Change the signature of iomap_write_iter() to return an error code. In case we cannot allocate a page in iomap_write_begin(), we will not retry the memory alloction in iomap_write_begin().

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

@@ -843,12 +845,15 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i,
 		.flags		= IOMAP_WRITE,
 	};
 	int ret;
+	int error = 0;

Quoted 3 lines · Stefan Roesch

Change the signature of iomap_write_iter() to return an error code. In case we cannot allocate a page in iomap_write_begin(), we will not retry the memory alloction in iomap_write_begin().

loff_t can already represent an error code. And it's already used like that.

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

Quoted 7 lines

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

@@ -829,7 +830,8 @@ 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;
+	*processed = written ? written : error;
+	return error;

I think the change you really want is:

	if (status == -EAGAIN)
		return -EAGAIN;
	if (written)
		return written;
	return status;

Quoted 5 lines

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

@@ -843,12 +845,15 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i,
 		.flags		= IOMAP_WRITE,
 	};
 	int ret;
+	int error = 0;

fs/iomap/buffered-io.c iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i, · line 856

Quoted 10 lines

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

 
 	if (iocb->ki_flags & IOCB_NOWAIT)
 		iter.flags |= IOMAP_NOWAIT;
 
-	while ((ret = iomap_iter(&iter, ops)) > 0)
-		iter.processed = iomap_write_iter(&iter, i);
+	while ((ret = iomap_iter(&iter, ops)) > 0) {
+		if (error != -EAGAIN)
+			error = iomap_write_iter(&iter, i, &iter.processed);
+	}

You don't need to change any of this. Look at how iomap_iter_advance() works.

Quoted 3 lines · Stefan Roesch

Change the signature of iomap_write_iter() to return an error code. In case we cannot allocate a page in iomap_write_begin(), we will not retry the memory alloction in iomap_write_begin().

loff_t can already represent an error code. And it's already used like that.

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

Quoted 7 lines

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

@@ -829,7 +830,8 @@ 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;
+	*processed = written ? written : error;
+	return error;

I think the change you really want is:

	if (status == -EAGAIN)
		return -EAGAIN;
	if (written)
		return written;
	return status;

Quoted 5 lines

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

@@ -843,12 +845,15 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i,
 		.flags		= IOMAP_WRITE,
 	};
 	int ret;
+	int error = 0;

fs/iomap/buffered-io.c iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i, · line 856

Quoted 10 lines

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

 
 	if (iocb->ki_flags & IOCB_NOWAIT)
 		iter.flags |= IOMAP_NOWAIT;
 
-	while ((ret = iomap_iter(&iter, ops)) > 0)
-		iter.processed = iomap_write_iter(&iter, i);
+	while ((ret = iomap_iter(&iter, ops)) > 0) {
+		if (error != -EAGAIN)
+			error = iomap_write_iter(&iter, i, &iter.processed);
+	}

You don't need to change any of this. Look at how iomap_iter_advance() works.

Quoted 18 lines · Matthew Wilcox

Quoted 3 lines · Stefan Roesch

Change the signature of iomap_write_iter() to return an error code. In case we cannot allocate a page in iomap_write_begin(), we will not retry the memory alloction in iomap_write_begin().

loff_t can already represent an error code. And it's already used like that.

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

Quoted 7 lines

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

@@ -829,7 +830,8 @@ 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;
+	*processed = written ? written : error;
+	return error;

I think the change you really want is:

Quoted 6 lines

	if (status == -EAGAIN)
		return -EAGAIN;
	if (written)
		return written;
	return status;

I think the change needs to be:

Quoted 30 lines · Stefan Roesch

Quoted 18 lines · Matthew Wilcox

Quoted 3 lines · Stefan Roesch

Change the signature of iomap_write_iter() to return an error code. In case we cannot allocate a page in iomap_write_begin(), we will not retry the memory alloction in iomap_write_begin().

loff_t can already represent an error code. And it's already used like that.

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

Quoted 7 lines

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

@@ -829,7 +830,8 @@ 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;
+	*processed = written ? written : error;
+	return error;

I think the change you really want is:

Quoted 6 lines

	if (status == -EAGAIN)
		return -EAGAIN;
	if (written)
		return written;
	return status;

I think the change needs to be:

Review

Quoted 8 lines

(quoted diff) +7 -1

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

Ah yes, I think you're right.

Does it work to leave everything the way it is, call back into the iomap_write_iter() after having done a short write, get the -EAGAIN at that point and pass the already-advanced iterator to the worker thread? I haven't looked into the details of how that works, so maybe you just can't do that.

Quoted 42 lines · Matthew Wilcox

Quoted 30 lines · Stefan Roesch

Quoted 18 lines · Matthew Wilcox

Quoted 3 lines · Stefan Roesch

Change the signature of iomap_write_iter() to return an error code. In case we cannot allocate a page in iomap_write_begin(), we will not retry the memory alloction in iomap_write_begin().

loff_t can already represent an error code. And it's already used like that.

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

Quoted 7 lines

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

@@ -829,7 +830,8 @@ 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;
+	*processed = written ? written : error;
+	return error;

I think the change you really want is:

Quoted 6 lines

	if (status == -EAGAIN)
		return -EAGAIN;
	if (written)
		return written;
	return status;

I think the change needs to be:

Review

Quoted 8 lines

(quoted diff) +7 -1

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

Ah yes, I think you're right.

Quoted 5 lines

Does it work to leave everything the way it is, call back into the iomap_write_iter() after having done a short write, get the -EAGAIN at that point and pass the already-advanced iterator to the worker thread? I haven't looked into the details of how that works, so maybe you just can't do that.

With the above change, short writes are handled correctly.

Quoted 45 lines · Stefan Roesch

Quoted 42 lines · Matthew Wilcox

Quoted 30 lines · Stefan Roesch

Quoted 18 lines · Matthew Wilcox

Quoted 3 lines · Stefan Roesch

Change the signature of iomap_write_iter() to return an error code. In case we cannot allocate a page in iomap_write_begin(), we will not retry the memory alloction in iomap_write_begin().

loff_t can already represent an error code. And it's already used like that.

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

Quoted 7 lines

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

@@ -829,7 +830,8 @@ 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;
+	*processed = written ? written : error;
+	return error;

I think the change you really want is:

Quoted 6 lines

	if (status == -EAGAIN)
		return -EAGAIN;
	if (written)
		return written;
	return status;

I think the change needs to be:

Review

Quoted 8 lines

(quoted diff) +7 -1

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

Ah yes, I think you're right.

Quoted 7 lines

Quoted 5 lines

Does it work to leave everything the way it is, call back into the iomap_write_iter() after having done a short write, get the -EAGAIN at that point and pass the already-advanced iterator to the worker thread? I haven't looked into the details of how that works, so maybe you just can't do that.

With the above change, short writes are handled correctly.

Are they though? What about a write that crosses an extent boundary? iomap_write_iter() gets called once per extent and I have a feeling that you really need to revert the entire write, rather than just the part of the write that was to that extent.

Anyway, my question was whether we need to revert or whether the worker thread can continue from where we left off.

Quoted 55 lines · Matthew Wilcox

Quoted 41 lines · Stefan Roesch

Quoted 40 lines · Matthew Wilcox

Quoted 28 lines · Stefan Roesch

Quoted 18 lines · Matthew Wilcox

Quoted 3 lines · Stefan Roesch

Change the signature of iomap_write_iter() to return an error code. In case we cannot allocate a page in iomap_write_begin(), we will not retry the memory alloction in iomap_write_begin().

loff_t can already represent an error code. And it's already used like that.

Review static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) · line 834

Quoted 7 lines

(quoted diff) +2 -1

@@ -829,7 +830,8 @@ 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;
+	*processed = written ? written : error;
+	return error;

I think the change you really want is:

Quoted 6 lines

	if (status == -EAGAIN)
		return -EAGAIN;
	if (written)
		return written;
	return status;

I think the change needs to be:

Review

Quoted 8 lines

(quoted diff) +7 -1

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

Ah yes, I think you're right.

Quoted 7 lines

Quoted 5 lines

Does it work to leave everything the way it is, call back into the iomap_write_iter() after having done a short write, get the -EAGAIN at that point and pass the already-advanced iterator to the worker thread? I haven't looked into the details of how that works, so maybe you just can't do that.

With the above change, short writes are handled correctly.

Are they though? What about a write that crosses an extent boundary? iomap_write_iter() gets called once per extent and I have a feeling that you really need to revert the entire write, rather than just the part of the write that was to that extent.

Quoted 2 lines

Anyway, my question was whether we need to revert or whether the worker thread can continue from where we left off.

Without iov_iter_revert() fsx fails with errors in short writes and also my test program which issues short writes fails.

Quoted 59 lines · Stefan Roesch

Quoted 57 lines · Matthew Wilcox

Quoted 41 lines · Stefan Roesch

Quoted 40 lines · Matthew Wilcox

Quoted 28 lines · Stefan Roesch

Quoted 18 lines · Matthew Wilcox

Quoted 3 lines · Stefan Roesch

Change the signature of iomap_write_iter() to return an error code. In case we cannot allocate a page in iomap_write_begin(), we will not retry the memory alloction in iomap_write_begin().

loff_t can already represent an error code. And it's already used like that.

Review static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) · line 834

Quoted 7 lines

(quoted diff) +2 -1

@@ -829,7 +830,8 @@ 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;
+	*processed = written ? written : error;
+	return error;

I think the change you really want is:

Quoted 6 lines

	if (status == -EAGAIN)
		return -EAGAIN;
	if (written)
		return written;
	return status;

I think the change needs to be:

Review

Quoted 8 lines

(quoted diff) +7 -1

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

Ah yes, I think you're right.

Quoted 7 lines

Quoted 5 lines

Does it work to leave everything the way it is, call back into the iomap_write_iter() after having done a short write, get the -EAGAIN at that point and pass the already-advanced iterator to the worker thread? I haven't looked into the details of how that works, so maybe you just can't do that.

With the above change, short writes are handled correctly.

Are they though? What about a write that crosses an extent boundary? iomap_write_iter() gets called once per extent and I have a feeling that you really need to revert the entire write, rather than just the part of the write that was to that extent.

Anyway, my question was whether we need to revert or whether the worker thread can continue from where we left off.

Quoted 2 lines

Without iov_iter_revert() fsx fails with errors in short writes and also my test program which issues short writes fails.

Does your test program include starting in one extent, completing the portion of the write which is in that extent successfully, and having the portion of the write which is in the second extent be short?

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

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

fs/inode.c +37 -20

index 9d9b422504d1..ac1cf5aa78c8 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

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

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

fs/inode.c +50 -26

index ac1cf5aa78c8..c44573a32c6a 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

+ /**

fs/inode.c +3 -0

+  * file_update_time - update mtime and ctime time
+  * @file: file accessed
+  *

Quoted 4 lines · Stefan Roesch

+ /**

fs/inode.c +3 -0

+  * file_update_time - update mtime and ctime time
+  * @file: file accessed
+  *

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

Quoted 10 lines

fs/inode.c +10 -0

+  * 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.
+  */

Can you remove the extra leading space from each of these lines?

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

---
 fs/inode.c         | 43 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/fs.h |  1 +
 2 files changed, 42 insertions(+), 2 deletions(-)

fs/inode.c +41 -2

index c44573a32c6a..4503bed063e7 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);

@@ -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 bdf1ce48a458..553e57ec3efa 100644
@@ -2392,6 +2392,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

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

fs/inode.c +0 -0

index c44573a32c6a..4503bed063e7 100644

Quoted 7 lines · 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]>

I've found one small bug here:

Quoted 4 lines

fs/inode.c +0 -0

index c44573a32c6a..4503bed063e7 100644

...

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

Quoted 5 lines

fs/inode.c +1 -1

-int file_modified(struct file *file)
+static int file_modified_flags(struct file *file, int flags)
 {
 	int ret;
 	struct inode *inode = file_inode(file);

We need to use 'flags' for __file_remove_privs_flags() call in this patch.

Honza

Quoted 16 lines · Jan Kara

Quoted 7 lines · 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]>

I've found one small bug here:

Quoted 4 lines

fs/inode.c +0 -0

index c44573a32c6a..4503bed063e7 100644

...

Quoted 8 lines

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

Quoted 5 lines

fs/inode.c +1 -1

-int file_modified(struct file *file)
+static int file_modified_flags(struct file *file, int flags)
 {
 	int ret;
 	struct inode *inode = file_inode(file);

We need to use 'flags' for __file_remove_privs_flags() call in this patch.

I assume that you meant that the function should not be called _file_remove_privs(), but instead file_remove_privs_flags(). Is that correct?

Quoted 28 lines · Stefan Roesch

Quoted 24 lines · Jan Kara

Quoted 7 lines · 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]>

I've found one small bug here:

Quoted 4 lines

fs/inode.c +0 -0

index c44573a32c6a..4503bed063e7 100644

...

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

Quoted 5 lines

fs/inode.c +1 -1

-int file_modified(struct file *file)
+static int file_modified_flags(struct file *file, int flags)
 {
 	int ret;
 	struct inode *inode = file_inode(file);

We need to use 'flags' for __file_remove_privs_flags() call in this patch.

Quoted 2 lines

I assume that you meant that the function should not be called _file_remove_privs(), but instead file_remove_privs_flags(). Is that correct?

No, I meant that patch 8 adds call __file_remove_privs(..., 0) to file_modified() and this patch then forgets to update that call to __file_remove_privs(..., flags) so that information propagates properly.

Honza

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 4503bed063e7..7185d860d423 100644
@@ -2150,10 +2150,17 @@ static int file_modified_flags(struct file *file, int flags)
 	ret = inode_needs_update_time(inode, &now);
 	if (ret <= 0)
 		return ret;
-	if (flags & IOCB_NOWAIT)
+	if (flags & IOCB_NOWAIT) {
+		if (IS_PENDING_TIME(inode))
+			return 0;
+
+		inode_set_flags(inode, S_PENDING_TIME, S_PENDING_TIME);
 		return -EAGAIN;
+	}
 
-	return __file_update_time(file, &now, ret);
+	ret = __file_update_time(file, &now, ret);
+	inode_set_flags(inode, 0, S_PENDING_TIME);
+	return ret;
 }
 
 /**

include/linux/fs.h +3 -0

index 553e57ec3efa..15f9a7beba55 100644
@@ -2151,6 +2151,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

@@ -2193,6 +2195,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

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

Quoted 1 line

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

Thinking about this, there is a snag with this S_PENDING_TIME scheme. It can happen that we report write as completed to userspace before timestamps are actually updated. So following stat(2) can still return old time stamp which might confuse some userspace application. It might be even nastier with i_version which is used by NFS and can thus cause data consistency issues for NFS.

Honza

Quoted 53 lines

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

fs/inode.c +9 -2

index 4503bed063e7..7185d860d423 100644
@@ -2150,10 +2150,17 @@ static int file_modified_flags(struct file *file, int flags)
 	ret = inode_needs_update_time(inode, &now);
 	if (ret <= 0)
 		return ret;
-	if (flags & IOCB_NOWAIT)
+	if (flags & IOCB_NOWAIT) {
+		if (IS_PENDING_TIME(inode))
+			return 0;
+
+		inode_set_flags(inode, S_PENDING_TIME, S_PENDING_TIME);
 		return -EAGAIN;
+	}
 
-	return __file_update_time(file, &now, ret);
+	ret = __file_update_time(file, &now, ret);
+	inode_set_flags(inode, 0, S_PENDING_TIME);
+	return ret;
 }
 
 /**

include/linux/fs.h +3 -0

index 553e57ec3efa..15f9a7beba55 100644
@@ -2151,6 +2151,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

@@ -2193,6 +2195,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

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

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

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

breaks generic/471...

Quoted 6 lines · Al Viro

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

breaks generic/471...

That test case is odd, because it makes some weird assumptions about what RWF_NOWAIT means. Most notably that it makes it mean if we should instantiate blocks or not. Where did those assumed semantics come from? On the read side, we have clearly documented that it should "not wait for data which is not immediately available".

Now it is possible that we're returning a spurious -EAGAIN here when we should not be. And that would be a bug imho. I'll dig in and see what's going on.

Quoted 14 lines · Jens Axboe

Quoted 6 lines · Al Viro

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

breaks generic/471...

That test case is odd, because it makes some weird assumptions about what RWF_NOWAIT means. Most notably that it makes it mean if we should instantiate blocks or not. Where did those assumed semantics come from? On the read side, we have clearly documented that it should "not wait for data which is not immediately available".

Quoted 3 lines

Now it is possible that we're returning a spurious -EAGAIN here when we should not be. And that would be a bug imho. I'll dig in and see what's going on.

This is the timestamp update that needs doing which will now return -EAGAIN if IOCB_NOWAIT is set as it may block.

I do wonder if we should just allow inode time updates with IOCB_NOWAIT, even on the io_uring side. Either that, or passed in RWF_NOWAIT semantics don't map completely to internal IOCB_NOWAIT semantics. At least in terms of what generic/471 is doing, but I'm not sure who came up with that and if it's established semantics or just some made up ones from whomever wrote that test. I don't think they make any sense, to be honest.

Quoted 22 lines · Jens Axboe

Quoted 14 lines · Jens Axboe

Quoted 6 lines · Al Viro

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

breaks generic/471...

That test case is odd, because it makes some weird assumptions about what RWF_NOWAIT means. Most notably that it makes it mean if we should instantiate blocks or not. Where did those assumed semantics come from? On the read side, we have clearly documented that it should "not wait for data which is not immediately available".

Quoted 3 lines

Now it is possible that we're returning a spurious -EAGAIN here when we should not be. And that would be a bug imho. I'll dig in and see what's going on.

This is the timestamp update that needs doing which will now return -EAGAIN if IOCB_NOWAIT is set as it may block.

Quoted 7 lines

I do wonder if we should just allow inode time updates with IOCB_NOWAIT, even on the io_uring side. Either that, or passed in RWF_NOWAIT semantics don't map completely to internal IOCB_NOWAIT semantics. At least in terms of what generic/471 is doing, but I'm not sure who came up with that and if it's established semantics or just some made up ones from whomever wrote that test. I don't think they make any sense, to be honest.

Further support that generic/471 is just randomly made up semantics, it needs to special case btrfs with nocow or you'd get -EAGAIN anyway for that test.

And it's relying on some random timing to see if this works. I really think that test case is just hot garbage, and doesn't test anything meaningful.

Signature · 1 line

Jens Axboe

Quoted 35 lines · Jens Axboe

Quoted 22 lines · Jens Axboe

Quoted 14 lines · Jens Axboe

Quoted 6 lines · Al Viro

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

breaks generic/471...

That test case is odd, because it makes some weird assumptions about what RWF_NOWAIT means. Most notably that it makes it mean if we should instantiate blocks or not. Where did those assumed semantics come from? On the read side, we have clearly documented that it should "not wait for data which is not immediately available".

Quoted 3 lines

Now it is possible that we're returning a spurious -EAGAIN here when we should not be. And that would be a bug imho. I'll dig in and see what's going on.

This is the timestamp update that needs doing which will now return -EAGAIN if IOCB_NOWAIT is set as it may block.

Quoted 7 lines

I do wonder if we should just allow inode time updates with IOCB_NOWAIT, even on the io_uring side. Either that, or passed in RWF_NOWAIT semantics don't map completely to internal IOCB_NOWAIT semantics. At least in terms of what generic/471 is doing, but I'm not sure who came up with that and if it's established semantics or just some made up ones from whomever wrote that test. I don't think they make any sense, to be honest.

Further support that generic/471 is just randomly made up semantics, it needs to special case btrfs with nocow or you'd get -EAGAIN anyway for that test.

Quoted 3 lines

And it's relying on some random timing to see if this works. I really think that test case is just hot garbage, and doesn't test anything meaningful.

<shrug> I had thought that NOWAIT means "don't wait for *any*thing", which would include timestamp updates... but then I've never been all that clear on what specifically NOWAIT will and won't wait for. :/

Quoted 40 lines · Darrick J. Wong

Quoted 38 lines · Jens Axboe

Quoted 22 lines · Jens Axboe

Quoted 14 lines · Jens Axboe

Quoted 6 lines · Al Viro

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

breaks generic/471...

That test case is odd, because it makes some weird assumptions about what RWF_NOWAIT means. Most notably that it makes it mean if we should instantiate blocks or not. Where did those assumed semantics come from? On the read side, we have clearly documented that it should "not wait for data which is not immediately available".

Quoted 3 lines

Now it is possible that we're returning a spurious -EAGAIN here when we should not be. And that would be a bug imho. I'll dig in and see what's going on.

This is the timestamp update that needs doing which will now return -EAGAIN if IOCB_NOWAIT is set as it may block.

Quoted 7 lines

I do wonder if we should just allow inode time updates with IOCB_NOWAIT, even on the io_uring side. Either that, or passed in RWF_NOWAIT semantics don't map completely to internal IOCB_NOWAIT semantics. At least in terms of what generic/471 is doing, but I'm not sure who came up with that and if it's established semantics or just some made up ones from whomever wrote that test. I don't think they make any sense, to be honest.

Further support that generic/471 is just randomly made up semantics, it needs to special case btrfs with nocow or you'd get -EAGAIN anyway for that test.

And it's relying on some random timing to see if this works. I really think that test case is just hot garbage, and doesn't test anything meaningful.

Quoted 3 lines

<shrug> I had thought that NOWAIT means "don't wait for *any*thing", which would include timestamp updates... but then I've never been all that clear on what specifically NOWAIT will and won't wait for. :/

Agree, at least the read semantics (kind of) make sense, but the ones seemingly made up by generic/471 don't seem to make any sense at all.

Quoted 41 lines · Jens Axboe

Quoted 40 lines · Darrick J. Wong

Quoted 38 lines · Jens Axboe

Quoted 22 lines · Jens Axboe

Quoted 14 lines · Jens Axboe

Quoted 6 lines · Al Viro

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

breaks generic/471...

That test case is odd, because it makes some weird assumptions about what RWF_NOWAIT means. Most notably that it makes it mean if we should instantiate blocks or not. Where did those assumed semantics come from? On the read side, we have clearly documented that it should "not wait for data which is not immediately available".

Quoted 3 lines

Now it is possible that we're returning a spurious -EAGAIN here when we should not be. And that would be a bug imho. I'll dig in and see what's going on.

This is the timestamp update that needs doing which will now return -EAGAIN if IOCB_NOWAIT is set as it may block.

Quoted 7 lines

I do wonder if we should just allow inode time updates with IOCB_NOWAIT, even on the io_uring side. Either that, or passed in RWF_NOWAIT semantics don't map completely to internal IOCB_NOWAIT semantics. At least in terms of what generic/471 is doing, but I'm not sure who came up with that and if it's established semantics or just some made up ones from whomever wrote that test. I don't think they make any sense, to be honest.

Further support that generic/471 is just randomly made up semantics, it needs to special case btrfs with nocow or you'd get -EAGAIN anyway for that test.

And it's relying on some random timing to see if this works. I really think that test case is just hot garbage, and doesn't test anything meaningful.

Quoted 7 lines

Quoted 3 lines

<shrug> I had thought that NOWAIT means "don't wait for *any*thing", which would include timestamp updates... but then I've never been all that clear on what specifically NOWAIT will and won't wait for. :/

Agree, at least the read semantics (kind of) make sense, but the ones seemingly made up by generic/471 don't seem to make any sense at all.

Added Goldwyn to the CC list for this.

This appears to be just a confusion about what we think NOWAIT should mean. Looking at the btrfs code it seems like Goldwyn took it as literally as possible so we wouldn't do any NOWAIT IO's unless it was into a NOCOW area, meaning we literally wouldn't do anything other than wrap the bio up and fire it off.

The general consensus seems to be that NOWAIT isn't that strict, and that BTRFS's definition was too strict. I wrote initial patches to give to Stefan to clean up the Btrfs side to allow us to use NOWAIT under a lot more circumstances.

Goldwyn, this test seems to be a little specific to our case, and can be flakey if the timing isn't just right. I think we should just remove it? Especially since how we define NOWAIT isn't quite right. Does that sound reasonable to you?

This appears to be just a confusion about what we think NOWAIT should mean. Looking at the btrfs code it seems like Goldwyn took it as literally as possible so we wouldn't do any NOWAIT IO's unless it was into a NOCOW area, meaning we literally wouldn't do anything other than wrap the bio up and fire it off.

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.

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

This series looks good to me now, but will need some slight rebasing since the 5.20 io_uring branch has split up the code a bit. Trivial to do though, I suspect it'll apply directly if we just change fs/io_uring.c to io_uring/rw.c instead.

The bigger question is how to stage this, as it's touching a bit of fs, mm, and io_uring...

Quoted 14 lines · Jens Axboe

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

This series looks good to me now, but will need some slight rebasing since the 5.20 io_uring branch has split up the code a bit. Trivial to do though, I suspect it'll apply directly if we just change fs/io_uring.c to io_uring/rw.c instead.

Quoted 2 lines

The bigger question is how to stage this, as it's touching a bit of fs, mm, and io_uring...

What data integrity testing has this had? Has it been run through a few billion fsx operations with w/ io_uring read/write enabled?

Quoted 18 lines · Dave Chinner

Quoted 16 lines · Jens Axboe

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

This series looks good to me now, but will need some slight rebasing since the 5.20 io_uring branch has split up the code a bit. Trivial to do though, I suspect it'll apply directly if we just change fs/io_uring.c to io_uring/rw.c instead.

The bigger question is how to stage this, as it's touching a bit of fs, mm, and io_uring...

Quoted 2 lines

What data integrity testing has this had? Has it been run through a few billion fsx operations with w/ io_uring read/write enabled?

I'll let Stefan expand on this, but just mention what I know - it has been fun via fio at least. Each of the performance tests were hour long each, and also specific test cases were written to test the boundary conditions of what pages of a range where in page cache, etc. Also with data verification.

Don't know if fsx specifically has been used it.