io-uring/xfs: support async buffered writes

Define FGP_ATOMIC flag and add support for the new flag in the function __filemap_get_folio().

Quoted 2 lines · Stefan Roesch

Define FGP_ATOMIC flag and add support for the new flag in the function __filemap_get_folio().

No. You don't get to use the emergency memory pools for doing writes. That completely screws up the MM.

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.

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

---
 fs/xfs/xfs_iomap.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

fs/xfs/xfs_iomap.c +1 -0

index e552ce541ec2..80b6c48e88af 100644
@@ -881,18 +881,28 @@ xfs_buffered_write_iomap_begin(
 	bool			eof = false, cow_eof = false, shared = false;
 	int			allocfork = XFS_DATA_FORK;
 	int			error = 0;
+	bool			no_wait = (flags & IOMAP_NOWAIT);
 
 	if (xfs_is_shutdown(mp))

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

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

---
 fs/xfs/xfs_iomap.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

fs/xfs/xfs_iomap.c +1 -0

index e552ce541ec2..80b6c48e88af 100644
@@ -881,18 +881,28 @@ xfs_buffered_write_iomap_begin(
 	bool			eof = false, cow_eof = false, shared = false;
 	int			allocfork = XFS_DATA_FORK;
 	int			error = 0;
+	bool			no_wait = (flags & IOMAP_NOWAIT);
 
 	if (xfs_is_shutdown(mp))
 		return -EIO;
 

fs/xfs/xfs_iomap.c xfs_buffered_write_iomap_begin( · line 895

Quoted 8 lines

fs/xfs/xfs_iomap.c +4 -1

 	/* we can't use delayed allocations when using extent size hints */
-	if (xfs_get_extsz_hint(ip))
+	if (xfs_get_extsz_hint(ip)) {
+		if (no_wait)
+			return -EAGAIN;
+
 		return xfs_direct_write_iomap_begin(inode, offset, count,
 				flags, iomap, srcmap);

Why can't we do IOMAP_NOWAIT allocation here? xfs_direct_write_iomap_begin() supports IOMAP_NOWAIT semantics just fine - that's what the direct IO path uses...

Quoted 5 lines

fs/xfs/xfs_iomap.c +1 -0

+	}
 
 	ASSERT(!XFS_IS_REALTIME_INODE(ip));
 

fs/xfs/xfs_iomap.c xfs_buffered_write_iomap_begin( · line 905

Quoted 7 lines

fs/xfs/xfs_iomap.c +6 -1

-	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	if (no_wait) {
+		if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL))
+			return -EAGAIN;
+	} else {
+		xfs_ilock(ip, XFS_ILOCK_EXCL);
+	}

handled by xfs_ilock_for_iomap().

Quoted 7 lines

fs/xfs/xfs_iomap.c +0 -0

 	if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(&ip->i_df)) ||
 	    XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {

@@ -902,6 +912,11 @@ xfs_buffered_write_iomap_begin(
 
 	XFS_STATS_INC(mp, xs_blk_mapw);
 

fs/xfs/xfs_iomap.c xfs_buffered_write_iomap_begin( · line 919

Quoted 5 lines

fs/xfs/xfs_iomap.c +5 -0

+	if (no_wait && xfs_need_iread_extents(XFS_IFORK_PTR(ip, XFS_DATA_FORK))) {
+		error = -EAGAIN;
+		goto out_unlock;
+	}
+

Also handled by xfs_ilock_for_iomap().

Quoted 5 lines

fs/xfs/xfs_iomap.c +0 -0

 	error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
 	if (error)
 		goto out_unlock;

fs/xfs/xfs_iomap.c xfs_buffered_write_iomap_begin( · line 956

Quoted 9 lines

fs/xfs/xfs_iomap.c +4 -0

@@ -933,6 +948,10 @@ xfs_buffered_write_iomap_begin(
 	if (xfs_is_cow_inode(ip)) {
 		if (!ip->i_cowfp) {
 			ASSERT(!xfs_is_reflink_inode(ip));
+			if (no_wait) {
+				error = -EAGAIN;
+				goto out_unlock;
+			}
 			xfs_ifork_init_cow(ip);
 		}

Also handled by xfs_ilock_for_iomap().

Quoted 5 lines

fs/xfs/xfs_iomap.c +0 -0

 		cow_eof = !xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb,

@@ -956,6 +975,11 @@ xfs_buffered_write_iomap_begin(
 			goto found_imap;
 		}
 

fs/xfs/xfs_iomap.c xfs_buffered_write_iomap_begin( · line 982

Quoted 5 lines

fs/xfs/xfs_iomap.c +5 -0

+		if (no_wait) {
+			error = -EAGAIN;
+			goto out_unlock;
+		}
+

Won't get here because this is COW path, and xfs_ilock_for_iomap() handles returning -EAGAIN for that case.

Quoted 8 lines

fs/xfs/xfs_iomap.c +0 -0

 		xfs_trim_extent(&imap, offset_fsb, end_fsb - offset_fsb);
 
 		/* Trim the mapping to the nearest shared extent boundary. */

@@ -993,6 +1017,11 @@ xfs_buffered_write_iomap_begin(
 			allocfork = XFS_COW_FORK;
 	}
 

fs/xfs/xfs_iomap.c xfs_buffered_write_iomap_begin( · line 954

Quoted 4 lines

fs/xfs/xfs_iomap.c +4 -0

+	if (no_wait) {
+		error = -EAGAIN;
+		goto out_unlock;
+	}

Why? Delayed allocation doesn't block...

Cheers,

Dave.

Quoted 23 lines · Dave Chinner

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

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

---
 fs/xfs/xfs_iomap.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

fs/xfs/xfs_iomap.c +1 -0

index e552ce541ec2..80b6c48e88af 100644
@@ -881,18 +881,28 @@ xfs_buffered_write_iomap_begin(
 	bool			eof = false, cow_eof = false, shared = false;
 	int			allocfork = XFS_DATA_FORK;
 	int			error = 0;
+	bool			no_wait = (flags & IOMAP_NOWAIT);
 
 	if (xfs_is_shutdown(mp))
 		return -EIO;
 

Quoted 12 lines

fs/xfs/xfs_iomap.c xfs_buffered_write_iomap_begin( · line 895

Quoted 8 lines

fs/xfs/xfs_iomap.c +4 -1

 	/* we can't use delayed allocations when using extent size hints */
-	if (xfs_get_extsz_hint(ip))
+	if (xfs_get_extsz_hint(ip)) {
+		if (no_wait)
+			return -EAGAIN;
+
 		return xfs_direct_write_iomap_begin(inode, offset, count,
 				flags, iomap, srcmap);

Why can't we do IOMAP_NOWAIT allocation here? xfs_direct_write_iomap_begin() supports IOMAP_NOWAIT semantics just fine - that's what the direct IO path uses...

I'll make that change in the next version.

Quoted 5 lines

Quoted 5 lines

fs/xfs/xfs_iomap.c +1 -0

+	}
 
 	ASSERT(!XFS_IS_REALTIME_INODE(ip));
 

Quoted 9 lines

fs/xfs/xfs_iomap.c xfs_buffered_write_iomap_begin( · line 905

Quoted 7 lines

fs/xfs/xfs_iomap.c +6 -1

-	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	if (no_wait) {
+		if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL))
+			return -EAGAIN;
+	} else {
+		xfs_ilock(ip, XFS_ILOCK_EXCL);
+	}

handled by xfs_ilock_for_iomap().

The helper xfs_ilock_for_iomap cannot be used for this purpose. The function xfs_ilock_for_iomap tries to deduce the lock mode. For the function xfs_file_buffered_write() the lock mode must be exclusive. However this is not always the case when the helper xfs_ilock_for_iomap is used. There are cases where xfs_is_cow_inode(() is not true.

I'll add a new helper that will be used here.

This modifies xfs write checks 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.

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

---
 fs/xfs/xfs_file.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

fs/xfs/xfs_file.c +26 -0

index 5bddb1e9e0b3..6f9da1059e8b 100644
@@ -299,6 +299,43 @@ xfs_file_read_iter(
 	return ret;
 }
 
+static int xfs_file_modified(struct file *file, int flags)
+{
+	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.
+	 * This keeps people from modifying setuid and setgid binaries.
+	 */
+	ret = need_file_remove_privs(inode, dentry);
+	if (ret < 0) {
+		return ret;
+	} else if (ret > 0) {
+		if (flags & IOCB_NOWAIT)
+			return -EAGAIN;
+
+		ret = do_file_remove_privs(file, inode, dentry, ret);
+		if (ret)
+			return ret;
+	}
+
+	ret = need_file_update_time(inode, file, &now);
+	if (ret <= 0)
+		return ret;

This modifies xfs write checks 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.

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

---
 fs/xfs/xfs_file.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

fs/xfs/xfs_file.c +0 -0

index 5bddb1e9e0b3..6f9da1059e8b 100644

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

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

---
 fs/xfs/xfs_file.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

fs/xfs/xfs_file.c +0 -2

index 6f9da1059e8b..49d54b939502 100644
@@ -739,12 +739,14 @@ xfs_file_buffered_write(
 	bool			cleared_space = false;
 	int			iolock;
 
-	if (iocb->ki_flags & IOCB_NOWAIT)
-		return -EOPNOTSUPP;

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

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

---
 fs/xfs/xfs_file.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

fs/xfs/xfs_file.c +0 -0

index 6f9da1059e8b..49d54b939502 100644
@@ -739,12 +739,14 @@ xfs_file_buffered_write(
 	bool			cleared_space = false;
 	int			iolock;
 

fs/xfs/xfs_file.c xfs_file_buffered_write( · line 749

Quoted 12 lines

fs/xfs/xfs_file.c +6 -4

-	if (iocb->ki_flags & IOCB_NOWAIT)
-		return -EOPNOTSUPP;
-
 write_retry:
 	iolock = XFS_IOLOCK_EXCL;
-	xfs_ilock(ip, iolock);
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!xfs_ilock_nowait(ip, iolock))
+			return -EAGAIN;
+	} else {
+		xfs_ilock(ip, iolock);
+	}

xfs_ilock_iocb().

-Dave.

Quoted 18 lines · Dave Chinner

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

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

---
 fs/xfs/xfs_file.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

fs/xfs/xfs_file.c +0 -0

index 6f9da1059e8b..49d54b939502 100644
@@ -739,12 +739,14 @@ xfs_file_buffered_write(
 	bool			cleared_space = false;
 	int			iolock;
 

Quoted 15 lines

fs/xfs/xfs_file.c xfs_file_buffered_write( · line 749

Quoted 12 lines

fs/xfs/xfs_file.c +6 -4

-	if (iocb->ki_flags & IOCB_NOWAIT)
-		return -EOPNOTSUPP;
-
 write_retry:
 	iolock = XFS_IOLOCK_EXCL;
-	xfs_ilock(ip, iolock);
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!xfs_ilock_nowait(ip, iolock))
+			return -EAGAIN;
+	} else {
+		xfs_ilock(ip, iolock);
+	}

xfs_ilock_iocb().

The helper xfs_ilock_iocb cannot be used as it hardcoded to use iocb->ki_filp to get a pointer to the xfs_inode.

However here we need to use iocb->ki_filp->f_mapping->host. I'll split off new helper for this in the next version of the patch.

Quoted 37 lines · Stefan Roesch

Quoted 18 lines · Dave Chinner

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

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

---
 fs/xfs/xfs_file.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

fs/xfs/xfs_file.c +0 -0

index 6f9da1059e8b..49d54b939502 100644
@@ -739,12 +739,14 @@ xfs_file_buffered_write(
 	bool			cleared_space = false;
 	int			iolock;
 

Quoted 15 lines

fs/xfs/xfs_file.c xfs_file_buffered_write( · line 749

Quoted 12 lines

fs/xfs/xfs_file.c +6 -4

-	if (iocb->ki_flags & IOCB_NOWAIT)
-		return -EOPNOTSUPP;
-
 write_retry:
 	iolock = XFS_IOLOCK_EXCL;
-	xfs_ilock(ip, iolock);
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!xfs_ilock_nowait(ip, iolock))
+			return -EAGAIN;
+	} else {
+		xfs_ilock(ip, iolock);
+	}

xfs_ilock_iocb().

Quoted 2 lines

The helper xfs_ilock_iocb cannot be used as it hardcoded to use iocb->ki_filp to get a pointer to the xfs_inode.

And the problem with that is?

I mean, look at what xfs_file_buffered_write() does to get the xfs_inode 10 lines about that change:

xfs_file_buffered_write(
struct kiocb *iocb,
struct iov_iter *from)

{
        struct file             *file = iocb->ki_filp;
        struct address_space    *mapping = file->f_mapping;
        struct inode            *inode = mapping->host;
        struct xfs_inode        *ip = XFS_I(inode);

In what cases does file_inode(iocb->ki_filp) point to a different inode than iocb->ki_filp->f_mapping->host? The dio write path assumes that file_inode(iocb->ki_filp) is correct, as do both the buffered and dio read paths.

What makes the buffered write path special in that file_inode(iocb->ki_filp) is not correctly set whilst iocb->ki_filp->f_mapping->host is?

Regardless, if this is a problem, then just pass the XFS inode to xfs_ilock_iocb() and this is a moot point.

Quoted 2 lines

However here we need to use iocb->ki_filp->f_mapping->host. I'll split off new helper for this in the next version of the patch.

We don't need a new helper here, either.

Cheers,

Dave.

Quoted 55 lines · Dave Chinner

Quoted 37 lines · Stefan Roesch

Quoted 18 lines · Dave Chinner

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

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

---
 fs/xfs/xfs_file.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

fs/xfs/xfs_file.c +0 -0

index 6f9da1059e8b..49d54b939502 100644
@@ -739,12 +739,14 @@ xfs_file_buffered_write(
 	bool			cleared_space = false;
 	int			iolock;
 

Quoted 15 lines

fs/xfs/xfs_file.c xfs_file_buffered_write( · line 749

Quoted 12 lines

fs/xfs/xfs_file.c +6 -4

-	if (iocb->ki_flags & IOCB_NOWAIT)
-		return -EOPNOTSUPP;
-
 write_retry:
 	iolock = XFS_IOLOCK_EXCL;
-	xfs_ilock(ip, iolock);
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!xfs_ilock_nowait(ip, iolock))
+			return -EAGAIN;
+	} else {
+		xfs_ilock(ip, iolock);
+	}

xfs_ilock_iocb().

Quoted 2 lines

The helper xfs_ilock_iocb cannot be used as it hardcoded to use iocb->ki_filp to get a pointer to the xfs_inode.

And the problem with that is?

I mean, look at what xfs_file_buffered_write() does to get the xfs_inode 10 lines about that change:

xfs_file_buffered_write(
struct kiocb *iocb,
struct iov_iter *from)

{
        struct file             *file = iocb->ki_filp;
        struct address_space    *mapping = file->f_mapping;
        struct inode            *inode = mapping->host;
        struct xfs_inode        *ip = XFS_I(inode);

Quoted 9 lines

In what cases does file_inode(iocb->ki_filp) point to a different inode than iocb->ki_filp->f_mapping->host? The dio write path assumes that file_inode(iocb->ki_filp) is correct, as do both the buffered and dio read paths.

What makes the buffered write path special in that file_inode(iocb->ki_filp) is not correctly set whilst iocb->ki_filp->f_mapping->host is?

In the function xfs_file_buffered_write() the code calls the function xfs_ilock(). The xfs_inode pointer that is passed in is iocb->ki_filp->f_mapping->host. The one used in xfs_ilock_iocb is ki_filp->f_inode.

After getting the lock, the code in xfs_file_buffered_write calls the function xfs_buffered_write_iomap_begin(). In this function the code calls xfs_ilock() for ki_filp->f_inode in exclusive mode.

If I replace the first xfs_ilock() call with xfs_ilock_iocb(), then it looks like I get a deadlock.

Am I missing something?

I can:
- replace the pointer to iocb with pointer to xfs_inode in the function xfs_ilock_iocb() and also pass in the flags value as a parameter.
or
- create function xfs_ilock_inode(), which xfs_ilock_iocb() calls. The existing calls will not need to change, only the xfs_ilock in xfs_file_buffered_write() will use xfs_ilock_inode().

Quoted 2 lines

Regardless, if this is a problem, then just pass the XFS inode to xfs_ilock_iocb() and this is a moot point.

Quoted 76 lines · Stefan Roesch

Quoted 55 lines · Dave Chinner

Quoted 37 lines · Stefan Roesch

Quoted 18 lines · Dave Chinner

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

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

---
 fs/xfs/xfs_file.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

fs/xfs/xfs_file.c +0 -0

index 6f9da1059e8b..49d54b939502 100644
@@ -739,12 +739,14 @@ xfs_file_buffered_write(
 	bool			cleared_space = false;
 	int			iolock;
 

Quoted 15 lines

fs/xfs/xfs_file.c xfs_file_buffered_write( · line 749

Quoted 12 lines

fs/xfs/xfs_file.c +6 -4

-	if (iocb->ki_flags & IOCB_NOWAIT)
-		return -EOPNOTSUPP;
-
 write_retry:
 	iolock = XFS_IOLOCK_EXCL;
-	xfs_ilock(ip, iolock);
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!xfs_ilock_nowait(ip, iolock))
+			return -EAGAIN;
+	} else {
+		xfs_ilock(ip, iolock);
+	}

xfs_ilock_iocb().

Quoted 2 lines

The helper xfs_ilock_iocb cannot be used as it hardcoded to use iocb->ki_filp to get a pointer to the xfs_inode.

And the problem with that is?

I mean, look at what xfs_file_buffered_write() does to get the xfs_inode 10 lines about that change:

xfs_file_buffered_write(
struct kiocb *iocb,
struct iov_iter *from)

{
        struct file             *file = iocb->ki_filp;
        struct address_space    *mapping = file->f_mapping;
        struct inode            *inode = mapping->host;
        struct xfs_inode        *ip = XFS_I(inode);

Quoted 9 lines

In what cases does file_inode(iocb->ki_filp) point to a different inode than iocb->ki_filp->f_mapping->host? The dio write path assumes that file_inode(iocb->ki_filp) is correct, as do both the buffered and dio read paths.

What makes the buffered write path special in that file_inode(iocb->ki_filp) is not correctly set whilst iocb->ki_filp->f_mapping->host is?

In the function xfs_file_buffered_write() the code calls the function xfs_ilock(). The xfs_inode pointer that is passed in is iocb->ki_filp->f_mapping->host. The one used in xfs_ilock_iocb is ki_filp->f_inode.

After getting the lock, the code in xfs_file_buffered_write calls the function xfs_buffered_write_iomap_begin(). In this function the code calls xfs_ilock() for ki_filp->f_inode in exclusive mode.

Quoted 4 lines

If I replace the first xfs_ilock() call with xfs_ilock_iocb(), then it looks like I get a deadlock.

Am I missing something?

Yes. They take different locks. xfs_file_buffered_write() takes the IOLOCK, xfs_buffered_write_iomap_begin() takes the ILOCK....

Quoted 7 lines

I can:
- replace the pointer to iocb with pointer to xfs_inode in the function xfs_ilock_iocb() and also pass in the flags value as a parameter.
or
- create function xfs_ilock_inode(), which xfs_ilock_iocb() calls. The existing calls will not need to change, only the xfs_ilock in xfs_file_buffered_write() will use xfs_ilock_inode().

Quoted 2 lines · You're making this way more complex than it needs to be. As I said:

Quoted 2 lines

Regardless, if this is a problem, then just pass the XFS inode to xfs_ilock_iocb() and this is a moot point.

Cheers,

Dave.

Quoted 12 lines · Dave Chinner

Quoted 8 lines · Stefan Roesch

Quoted 1 line · Dave Chinner

On Thu, Apr 28, 2022 at 12:58:59PM -0700, Stefan Roesch wrote:

- replace the pointer to iocb with pointer to xfs_inode in the function xfs_ilock_iocb() and also pass in the flags value as a parameter.
or
- create function xfs_ilock_inode(), which xfs_ilock_iocb() calls. The existing calls will not need to change, only the xfs_ilock in xfs_file_buffered_write() will use xfs_ilock_inode().

You're making this way more complex than it needs to be. As I said:

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

Regardless, if this is a problem, then just pass the XFS inode to xfs_ilock_iocb() and this is a moot point.

The function xfs_ilock_iocb() is expecting a pointer to the data structure kiocb, not a pointer to xfs_inode. I don't see how that's possible without changing the signature of xfs_ilock_iocb().

Quoted 17 lines · Stefan Roesch

Quoted 15 lines · Dave Chinner

Quoted 8 lines · Stefan Roesch

Quoted 1 line · Dave Chinner

On Thu, Apr 28, 2022 at 12:58:59PM -0700, Stefan Roesch wrote:

- replace the pointer to iocb with pointer to xfs_inode in the function xfs_ilock_iocb() and also pass in the flags value as a parameter.
or
- create function xfs_ilock_inode(), which xfs_ilock_iocb() calls. The existing calls will not need to change, only the xfs_ilock in xfs_file_buffered_write() will use xfs_ilock_inode().

Quoted 2 lines · You're making this way more complex than it needs to be. As I said:

Quoted 2 lines

Regardless, if this is a problem, then just pass the XFS inode to xfs_ilock_iocb() and this is a moot point.

Quoted 3 lines

The function xfs_ilock_iocb() is expecting a pointer to the data structure kiocb, not a pointer to xfs_inode. I don't see how that's possible without changing the signature of xfs_ilock_iocb().

For the *third time*: pass the xfs_inode to xfs_ilock_iocb() and update all the callers to do the same thing.

-Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 22 lines · Dave Chinner

Quoted 20 lines · Stefan Roesch

Quoted 10 lines · Dave Chinner

Quoted 8 lines · Stefan Roesch

Quoted 1 line · Dave Chinner

On Thu, Apr 28, 2022 at 12:58:59PM -0700, Stefan Roesch wrote:

- replace the pointer to iocb with pointer to xfs_inode in the function xfs_ilock_iocb() and also pass in the flags value as a parameter.
or
- create function xfs_ilock_inode(), which xfs_ilock_iocb() calls. The existing calls will not need to change, only the xfs_ilock in xfs_file_buffered_write() will use xfs_ilock_inode().

Quoted 5 lines

Quoted 2 lines · You're making this way more complex than it needs to be. As I said:

Quoted 2 lines

Regardless, if this is a problem, then just pass the XFS inode to xfs_ilock_iocb() and this is a moot point.

The function xfs_ilock_iocb() is expecting a pointer to the data structure kiocb, not a pointer to xfs_inode. I don't see how that's possible without changing the signature of xfs_ilock_iocb().

Quoted 2 lines

For the *third time*: pass the xfs_inode to xfs_ilock_iocb() and update all the callers to do the same thing.

I still don't understand why /any/ of this is necessary. When does iocb->ki_filp->f_inode != iocb->ki_filp->f_mapping->host?

Quoted 2 lines

I still don't understand why /any/ of this is necessary. When does iocb->ki_filp->f_inode != iocb->ki_filp->f_mapping->host?

I already asked that question because I don't know the answer, either. I suspect the answer is "block dev inodes" but that then just raises the question of "how do we get them here?" and I don't know the answer to that, either. I don't have the time to dig into this and I don't expect anyone to just pop up with an answer, either. So in the mean time, we can just ignore it for the purpose of this patch set...

Quoted 10 lines · Dave Chinner

Quoted 2 lines

I still don't understand why /any/ of this is necessary. When does iocb->ki_filp->f_inode != iocb->ki_filp->f_mapping->host?

I already asked that question because I don't know the answer, either. I suspect the answer is "block dev inodes" but that then just raises the question of "how do we get them here?" and I don't know the answer to that, either. I don't have the time to dig into this and I don't expect anyone to just pop up with an answer, either. So in the mean time, we can just ignore it for the purpose of this patch set...

Weird device nodes (including block device) is the answer. It never happens for a normal file system file struct that we'd see in XFS.

Ok, so we can just use XFS_I(file_inode(iocb->ki_filp)) then and we don't need to pass the xfs_inode at all. We probably should convert the rest of the io path to do this as well so we don't end up forgetting this again...

This change adds support for async write throttling in the function balance_dirty_pages(). So far if throttling was required, the code was waiting synchronously as long as the writes were throttled. This change introduces asynchronous throttling. Instead of waiting in the function balance_dirty_pages(), the timeout is set in the task_struct field bdp_pause. Once the timeout has expired, the writes are no longer throttled.

- Add a new parameter to the balance_dirty_pages() function
- This allows the caller to pass in the nowait flag
- When the nowait flag is specified, the code does not wait in balance_dirty_pages(), but instead stores the wait expiration in the new task_struct field bdp_pause.

- The function balance_dirty_pages_ratelimited() resets the new values in the task_struct, once the timeout has expired

This change is required to support write throttling for the async buffered writes. While the writes are throttled, io_uring still can make progress with processing other requests.

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

Quoted 14 lines · Stefan Roesch

This change adds support for async write throttling in the function balance_dirty_pages(). So far if throttling was required, the code was waiting synchronously as long as the writes were throttled. This change introduces asynchronous throttling. Instead of waiting in the function balance_dirty_pages(), the timeout is set in the task_struct field bdp_pause. Once the timeout has expired, the writes are no longer throttled.

- Add a new parameter to the balance_dirty_pages() function
- This allows the caller to pass in the nowait flag
- When the nowait flag is specified, the code does not wait in balance_dirty_pages(), but instead stores the wait expiration in the new task_struct field bdp_pause.

Quoted 8 lines

- The function balance_dirty_pages_ratelimited() resets the new values in the task_struct, once the timeout has expired

This change is required to support write throttling for the async buffered writes. While the writes are throttled, io_uring still can make progress with processing other requests.

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

Maybe I miss something but I don't think this will throttle writers enough. For three reasons:

1) The calculated throttling pauses should accumulate for the task so that if we compute that say it takes 0.1s to write 100 pages and the task writes 300 pages, the delay adds up to 0.3s properly. Otherwise the task would not be throttled as long as we expect the writeback to take.

2) We must not allow the amount of dirty pages to exceed the dirty limit. That can easily lead to page reclaim getting into trouble reclaiming pages and thus machine stalls, oom kills etc. So if we are coming close to dirty limit and we cannot sleep, we must just fail the nowait write.

3) Even with above two problems fixed I suspect results will be suboptimal because balance_dirty_pages() heuristics assume they get called reasonably often and throttle writes so if amount of dirty pages is coming close to dirty limit, they think we are overestimating writeback speed and update throttling parameters accordingly. So if io_uring code does not throttle writers often enough, I think dirty throttling parameters will be jumping wildly resulting in poor behavior.

So what I'd probably suggest is that if balance_dirty_pages() is called in "async" mode, we'd give tasks a pass until dirty_freerun_ceiling(). If balance_dirty_pages() decides the task needs to wait, we store the pause and bail all the way up into the place where we can sleep (io_uring code I assume), sleep there, and then continue doing write.

Honza

Quoted 45 lines · Jan Kara

Quoted 14 lines · Stefan Roesch

This change adds support for async write throttling in the function balance_dirty_pages(). So far if throttling was required, the code was waiting synchronously as long as the writes were throttled. This change introduces asynchronous throttling. Instead of waiting in the function balance_dirty_pages(), the timeout is set in the task_struct field bdp_pause. Once the timeout has expired, the writes are no longer throttled.

- Add a new parameter to the balance_dirty_pages() function
- This allows the caller to pass in the nowait flag
- When the nowait flag is specified, the code does not wait in balance_dirty_pages(), but instead stores the wait expiration in the new task_struct field bdp_pause.

Quoted 8 lines

- The function balance_dirty_pages_ratelimited() resets the new values in the task_struct, once the timeout has expired

This change is required to support write throttling for the async buffered writes. While the writes are throttled, io_uring still can make progress with processing other requests.

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

Maybe I miss something but I don't think this will throttle writers enough. For three reasons:

1) The calculated throttling pauses should accumulate for the task so that if we compute that say it takes 0.1s to write 100 pages and the task writes 300 pages, the delay adds up to 0.3s properly. Otherwise the task would not be throttled as long as we expect the writeback to take.

2) We must not allow the amount of dirty pages to exceed the dirty limit. That can easily lead to page reclaim getting into trouble reclaiming pages and thus machine stalls, oom kills etc. So if we are coming close to dirty limit and we cannot sleep, we must just fail the nowait write.

3) Even with above two problems fixed I suspect results will be suboptimal because balance_dirty_pages() heuristics assume they get called reasonably often and throttle writes so if amount of dirty pages is coming close to dirty limit, they think we are overestimating writeback speed and update throttling parameters accordingly. So if io_uring code does not throttle writers often enough, I think dirty throttling parameters will be jumping wildly resulting in poor behavior.

Quoted 6 lines

So what I'd probably suggest is that if balance_dirty_pages() is called in "async" mode, we'd give tasks a pass until dirty_freerun_ceiling(). If balance_dirty_pages() decides the task needs to wait, we store the pause and bail all the way up into the place where we can sleep (io_uring code I assume), sleep there, and then continue doing write.

Jan, thanks for the feedback. Are you suggesting to change the following check in the function balance_dirty_pages():

                /*
                 * Throttle it only when the background writeback cannot
                 * catch-up. This avoids (excessively) small writeouts
                 * when the wb limits are ramping up in case of !strictlimit.
                 *
                 * In strictlimit case make decision based on the wb counters
                 * and limits. Small writeouts when the wb limits are ramping
                 * up are the price we consciously pay for strictlimit-ing.
                 *
                 * If memcg domain is in effect, @dirty should be under
                 * both global and memcg freerun ceilings.
                 */
                if (dirty <= dirty_freerun_ceiling(thresh, bg_thresh) &&
                    (!mdtc ||
                     m_dirty <= dirty_freerun_ceiling(m_thresh, m_bg_thresh))) {
                        unsigned long intv;
                        unsigned long m_intv;

to include if we are in async mode?

There is no direct way to return that the process should sleep. Instead two new fields are introduced in the proc structure. These two fields are then used in io_uring to determine if the writes for a task need to be throttled.

In case the writes need to be throttled, the writes are not issued, but instead inserted on a wait queue. We cannot sleep in the general io_uring code path as we still want to process other requests which are affected by the throttling.

Sorry for delayed reply. This has fallen through the cracks...

Quoted 74 lines · Stefan Roesch

Quoted 45 lines · Jan Kara

Quoted 14 lines · Stefan Roesch

This change adds support for async write throttling in the function balance_dirty_pages(). So far if throttling was required, the code was waiting synchronously as long as the writes were throttled. This change introduces asynchronous throttling. Instead of waiting in the function balance_dirty_pages(), the timeout is set in the task_struct field bdp_pause. Once the timeout has expired, the writes are no longer throttled.

- Add a new parameter to the balance_dirty_pages() function
- This allows the caller to pass in the nowait flag
- When the nowait flag is specified, the code does not wait in balance_dirty_pages(), but instead stores the wait expiration in the new task_struct field bdp_pause.

Quoted 8 lines

- The function balance_dirty_pages_ratelimited() resets the new values in the task_struct, once the timeout has expired

This change is required to support write throttling for the async buffered writes. While the writes are throttled, io_uring still can make progress with processing other requests.

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

Maybe I miss something but I don't think this will throttle writers enough. For three reasons:

1) The calculated throttling pauses should accumulate for the task so that if we compute that say it takes 0.1s to write 100 pages and the task writes 300 pages, the delay adds up to 0.3s properly. Otherwise the task would not be throttled as long as we expect the writeback to take.

2) We must not allow the amount of dirty pages to exceed the dirty limit. That can easily lead to page reclaim getting into trouble reclaiming pages and thus machine stalls, oom kills etc. So if we are coming close to dirty limit and we cannot sleep, we must just fail the nowait write.

3) Even with above two problems fixed I suspect results will be suboptimal because balance_dirty_pages() heuristics assume they get called reasonably often and throttle writes so if amount of dirty pages is coming close to dirty limit, they think we are overestimating writeback speed and update throttling parameters accordingly. So if io_uring code does not throttle writers often enough, I think dirty throttling parameters will be jumping wildly resulting in poor behavior.

Quoted 6 lines

So what I'd probably suggest is that if balance_dirty_pages() is called in "async" mode, we'd give tasks a pass until dirty_freerun_ceiling(). If balance_dirty_pages() decides the task needs to wait, we store the pause and bail all the way up into the place where we can sleep (io_uring code I assume), sleep there, and then continue doing write.

Jan, thanks for the feedback. Are you suggesting to change the following check in the function balance_dirty_pages():

                /*
                 * Throttle it only when the background writeback cannot
                 * catch-up. This avoids (excessively) small writeouts
                 * when the wb limits are ramping up in case of !strictlimit.
                 *
                 * In strictlimit case make decision based on the wb counters
                 * and limits. Small writeouts when the wb limits are ramping
                 * up are the price we consciously pay for strictlimit-ing.
                 *
                 * If memcg domain is in effect, @dirty should be under
                 * both global and memcg freerun ceilings.
                 */
                if (dirty <= dirty_freerun_ceiling(thresh, bg_thresh) &&
                    (!mdtc ||
                     m_dirty <= dirty_freerun_ceiling(m_thresh, m_bg_thresh))) {
                        unsigned long intv;
                        unsigned long m_intv;

Quoted 1 line

to include if we are in async mode?

Actually no. This condition is the one that gives any task a free pass until dirty_freerun_ceiling(). So there's no need to do any modification for that. Sorry, I've probably formulated my suggestion in a bit confusing way.

Quoted 5 lines

There is no direct way to return that the process should sleep. Instead two new fields are introduced in the proc structure. These two fields are then used in io_uring to determine if the writes for a task need to be throttled.

Quoted 4 lines

In case the writes need to be throttled, the writes are not issued, but instead inserted on a wait queue. We cannot sleep in the general io_uring code path as we still want to process other requests which are affected by the throttling.

Probably you wanted to say "are not affected by the throttling" in the above.

I know that you're using fields in task_struct to propagate the delay info. But IMHO that is unnecessary (although I don't care too much). Instead we could factor out a variant of balance_dirty_pages() that returns 'pause' to sleep, 0 if no sleeping needed. Normal balance_dirty_pages() would use this for pause calculation, places wanting async throttling would only get the pause to sleep. So e.g. iomap_write_iter() would then check and if returned pause is > 0, it would abort the loop similary as we'd abort it for any other reason when NOWAIT write is aborted because we need to sleep. Iouring code then detects short write / EAGAIN and offloads the write to the workqueue where normal balance_dirty_pages() can sleep as needed.

This will make sure dirty limits are properly observed and we don't need that much special handling for it.

Honza

Quoted 11 lines · Jan Kara

I know that you're using fields in task_struct to propagate the delay info. But IMHO that is unnecessary (although I don't care too much). Instead we could factor out a variant of balance_dirty_pages() that returns 'pause' to sleep, 0 if no sleeping needed. Normal balance_dirty_pages() would use this for pause calculation, places wanting async throttling would only get the pause to sleep. So e.g. iomap_write_iter() would then check and if returned pause is > 0, it would abort the loop similary as we'd abort it for any other reason when NOWAIT write is aborted because we need to sleep. Iouring code then detects short write / EAGAIN and offloads the write to the workqueue where normal balance_dirty_pages() can sleep as needed.

Quoted 3 lines

This will make sure dirty limits are properly observed and we don't need that much special handling for it.

I like the idea of factoring out a function out balance_dirty_pages(), however

I see two challenges:
- the write operation has already completed at this point,
- so we can't really sleep on its completion in the io-worker in io-uring
- we don't know how long to sleep in io-uring

Currently balance_dirty_pages_ratelimited() is called at the end of the function iomap_write_iter(). If the function balance_dirty_pages_ratelimited() would instead be called at the beginning of the function iomap_write_iter() we could return -EAGAIN and then complete it in the io-worker.

Quoted 23 lines · Stefan Roesch

Quoted 11 lines · Jan Kara

I know that you're using fields in task_struct to propagate the delay info. But IMHO that is unnecessary (although I don't care too much). Instead we could factor out a variant of balance_dirty_pages() that returns 'pause' to sleep, 0 if no sleeping needed. Normal balance_dirty_pages() would use this for pause calculation, places wanting async throttling would only get the pause to sleep. So e.g. iomap_write_iter() would then check and if returned pause is > 0, it would abort the loop similary as we'd abort it for any other reason when NOWAIT write is aborted because we need to sleep. Iouring code then detects short write / EAGAIN and offloads the write to the workqueue where normal balance_dirty_pages() can sleep as needed.

Quoted 3 lines

This will make sure dirty limits are properly observed and we don't need that much special handling for it.

I like the idea of factoring out a function out balance_dirty_pages(), however

I see two challenges:
- the write operation has already completed at this point,
- so we can't really sleep on its completion in the io-worker in io-uring
- we don't know how long to sleep in io-uring

Quoted 5 lines

Currently balance_dirty_pages_ratelimited() is called at the end of the function iomap_write_iter(). If the function balance_dirty_pages_ratelimited() would instead be called at the beginning of the function iomap_write_iter() we could return -EAGAIN and then complete it in the io-worker.

Well, we call balance_dirty_pages_ratelimited() after each page. So it does not really matter much if the sleep is pushed to happen one page later. balance_dirty_pages_ratelimited() does ratelimiting of when balance_dirty_pages() are called so we have to make sure current->nr_dirtied is not zeroed out before we really do wait (because that is what determines whether we enter balance_dirty_pages() and how long we sleep there) but looking at the code that should work out just fine.

Honza

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
iops: 80k 269k

random writes:
without patch with patch
iops: 76k 249k

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 any further improvements.

Especially for mixed workloads this is a considerable improvement.

Support for async buffered writes:

Patch 1: block: add check for async buffered writes to generic_write_checks
Add a new flag FMODE_BUF_WASYNC so filesystems can specify that they support async buffered writes and include the flag in the check of the function generic_write_checks().

Patch 2: mm: add FGP_ATOMIC flag to __filemap_get_folio()
This adds the FGP_ATOMIC flag. This allows to specify the gfp flags for memory allocations for async buffered writes.

Patch 3: iomap: add iomap_page_create_gfp to allocate iomap_pages
Add new function to allow specifying gfp flags when allocating and initializing the structure iomap_page.

Patch 4: iomap: use iomap_page_create_gfp() in __iomap_write_begin
Add a gfp flag to the iomap_page_create function.

Patch 5: iomap: add async buffered write support
Set IOMAP_NOWAIT flag if IOCB_NOWAIT is set. Also use specific gfp flags if the iomap_page structure is allocated for an async buffered page.

Patch 6: xfs: add iomap async buffered write support
Add async buffered write support to the xfs iomap layer.

Support for async buffered write support and inode time modification

Patch 7: fs: split off need_remove_file_privs() do_remove_file_privs()
Splits of a check and action function, so they can later be invoked in the nowait code path.

Patch 8: fs: split off need_file_update_time and do_file_update_time
Splits of a check and action function, so they can later be invoked in the nowait code path.

Patch 9: fs: add pending file update time flag.
Add new flag so consecutive write requests for the same inode can proceed without waiting for the inode modification time to complete.

Patch 10: xfs: enable async write file modification handling.
Enable async write handling in xfs for the file modification time update. If the file modification update requires logging or removal of privileges that needs to wait, -EAGAIN is returned.

Patch 11: xfs: add async buffered write support
Take the ilock in nowait mode if async buffered writes are enabled.

Patch 12: io_uring: add support for async buffered writes
This enables the async buffered writes optimization in io_uring. Buffered writes are enabled for blocks that are already in the page cache.

Patch 13: io_uring: Add tracepoint for short writes

Support for write throttling of async buffered writes:

Patch 14: sched: add new fields to task_struct
Add two new fields to the task_struct. These fields store the deadline after which writes are no longer throttled.

Patch 15: mm: support write throttling for async buffered writes
This changes the balance_dirty_pages function to take an additional parameter. When nowait is specified the write throttling code no longer waits synchronously for the deadline to expire. Instead it sets the fields in task_struct. Once the deadline expires the fields are reset.