[PATCH 0/3] btrfs: a few direct IO fixes/improvements

[PATCH 0/3] btrfs: a few direct IO fixes/improvements

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Several fixes and improvements regarding direct IO, with the first one being
recently reported by Dominique when using qemu with io_uring on images that
have a mix of compressed and non-compressed extents. That in particular
exposed a bug in qemu's handling of partial reads, fixed recently by
Dominique, but despite that we could avoid returning a partial read when
there's really no need to, not just for efficiency but also because many
applications don't take into consideration partial reads (MariaDB for
example didn't use to until recently) or are buggy dealing with them.
More details in the changelogs.

Filipe Manana (3):
  btrfs: return -EAGAIN for NOWAIT dio reads/writes on compressed and
    inline extents
  btrfs: don't fallback to buffered IO for NOWAIT direct IO writes
  btrfs: fault in pages for dio reads/writes in a more controlled way

 fs/btrfs/file.c  | 72 ++++++++++++++++++++++++++++++++++++------------
 fs/btrfs/inode.c | 14 +++++++++-
 2 files changed, 68 insertions(+), 18 deletions(-)

-- 
2.35.1

[PATCH 1/3] btrfs: return -EAGAIN for NOWAIT dio reads/writes on compressed and inline extents

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When doing a direct IO read or write, we always return -ENOTBLK when we
find a compressed extent (or an inline extent) so that we fallback to
buffered IO. This however is not ideal in case we are in a NOWAIT context
(io_uring for example), because buffered IO can block and we currently
have no support for NOWAIT semantics for buffered IO, so if we need to
fallback to buffered IO we should first signal the caller that we may
need to block by returning -EAGAIN instead.

This behaviour can also result in short reads being returned to user
space, which although it's not incorrect and user space should be able
to deal with partial reads, it's somewhat surprising and even some popular
applications like QEMU (Link tag #1) and MariaDB (Link tag #2) don't
deal with short reads properly (or at all).

The short read case happens when we try to read from a range that has a
non-compressed and non-inline extent followed by a compressed extent.
After having read the first extent, when we find the compressed extent we
return -ENOTBLK from btrfs_dio_iomap_begin(), which results in iomap to
treat the request as a short read, returning 0 (success) and waiting for
previously submitted bios to complete (this happens at
fs/iomap/direct-io.c:__iomap_dio_rw()). After that, and while at
btrfs_file_read_iter(), we call filemap_read() to use buffered IO to
read the remaining data, and pass it the number of bytes we were able to
read with direct IO. Than at filemap_read() if we get a page fault error
when accessing the read buffer, we return a partial read instead of an
-EFAULT error, because the number of bytes previously read is greater
than zero.

So fix this by returning -EAGAIN for NOWAIT direct IO when we find a
compressed or an inline extent.

Reported-by: Dominique MARTINET <dominique.martinet@atmark-techno.com>
Link: https://lore.kernel.org/linux-btrfs/YrrFGO4A1jS0GI0G@atmark-techno.com/
Link: https://jira.mariadb.org/browse/MDEV-27900?focusedCommentId=216582&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-216582
Tested-by: Dominique MARTINET <dominique.martinet@atmark-techno.com>
CC: stable@vger.kernel.org # 5.10+
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 7a54f964ff37..b86be4c3513d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7684,7 +7684,19 @@ static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
 	if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags) ||
 	    em->block_start == EXTENT_MAP_INLINE) {
 		free_extent_map(em);
-		ret = -ENOTBLK;
+		/*
+		 * If we are in a NOWAIT context, return -EAGAIN in order to
+		 * fallback to buffered IO. This is not only because we can
+		 * block with buffered IO (no support for NOWAIT semantics at
+		 * the moment) but also to avoid returning short reads to user
+		 * space - this happens if we were able to read some data from
+		 * previous non-compressed extents and then when we fallback to
+		 * buffered IO, at btrfs_file_read_iter() by calling
+		 * filemap_read(), we fail to fault in pages for the read buffer,
+		 * in which case filemap_read() returns a short read (the number
+		 * of bytes previously read is > 0, so it does not return -EFAULT).
+		 */
+		ret = (flags & IOMAP_NOWAIT) ? -EAGAIN : -ENOTBLK;
 		goto unlock_err;
 	}
 
-- 
2.35.1

[PATCH 2/3] btrfs: don't fallback to buffered IO for NOWAIT direct IO writes

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Currently, for a direct IO write, if we need to fallback to buffered IO,
either the satisfy the whole write operation or just a part of it, we do
it in the current context even if it's a NOWAIT context. This is not ideal
because we currently don't have support for NOWAIT semantics in the
buffered IO path (we can block for several reasons), so we should instead
return -EAGAIN to the caller, so that it knows it should retry (the whole
operation or what's left of it) in a context where blocking is acceptable.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index da41a0c371bc..9c8e3a668d70 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1971,11 +1971,25 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	if (is_sync_write)
 		iocb->ki_flags |= IOCB_DSYNC;
 
-	/* If 'err' is -ENOTBLK then it means we must fallback to buffered IO. */
+	/*
+	 * If 'err' is -ENOTBLK or we have not written all data, then it means
+	 * we must fallback to buffered IO.
+	 */
 	if ((err < 0 && err != -ENOTBLK) || !iov_iter_count(from))
 		goto out;
 
 buffered:
+	/*
+	 * If we are in a NOWAIT context, then return -EAGAIN to signal the caller
+	 * it must retry the operation in a context where blocking is acceptable,
+	 * since we currently don't have NOWAIT semantics support for buffered IO
+	 * and may block there for many reasons (reserving space for example).
+	 */
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		err = -EAGAIN;
+		goto out;
+	}
+
 	pos = iocb->ki_pos;
 	written_buffered = btrfs_buffered_write(iocb, from);
 	if (written_buffered < 0) {
-- 
2.35.1

[PATCH 3/3] btrfs: fault in pages for dio reads/writes in a more controlled way

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When we need to fault in the pages of the iovector of a direct IO read or
write, we try to fault in all the remaining pages. While this works, it's
not ideal if there's a large number of remaining pages and the system is
under memory pressure. By the time we fault in some pages, some of the
previously faulted in pages may have been evicted due to memory pressure,
resulting in slower progress or falling back to buffered IO (which is fine
but it's not ideal).

So limit the number of pages we fault in. The amount is decided based on
what's left of the iovector and the threshold for dirty page rate limiting
(the nr_dirtied and nr_dirtied_pause fields of struct task_struct), and
it's borrowed from gfs2 (fs/gfs2/file.c:should_fault_in_pages()).

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file.c | 56 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 9c8e3a668d70..1528b8edc7a9 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1846,6 +1846,33 @@ static ssize_t check_direct_IO(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
+static size_t dio_fault_in_size(const struct kiocb *iocb,
+				const struct iov_iter *iov,
+				size_t prev_left)
+{
+	const size_t left = iov_iter_count(iov);
+	size_t size = PAGE_SIZE;
+
+	/*
+	 * If there's no progress since the last time we had to fault in pages,
+	 * then we fault in at most 1 page. Faulting in more than that may
+	 * result in making very slow progress or falling back to buffered IO,
+	 * because by the time we retry the DIO operation some of the first
+	 * remaining pages may have been evicted in order to fault in other pages
+	 * that follow them. That can happen when we are under memory pressure and
+	 * the iov represents a large buffer.
+	 */
+	if (left != prev_left) {
+		int dirty_tresh = current->nr_dirtied_pause - current->nr_dirtied;
+
+		size = max(dirty_tresh, 8) << PAGE_SHIFT;
+		size = min_t(size_t, SZ_1M, size);
+	}
+	size -= offset_in_page(iocb->ki_pos);
+
+	return min(left, size);
+}
+
 static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 {
 	const bool is_sync_write = (iocb->ki_flags & IOCB_DSYNC);
@@ -1956,7 +1983,9 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 		if (left == prev_left) {
 			err = -ENOTBLK;
 		} else {
-			fault_in_iov_iter_readable(from, left);
+			const size_t size = dio_fault_in_size(iocb, from, prev_left);
+
+			fault_in_iov_iter_readable(from, size);
 			prev_left = left;
 			goto again;
 		}
@@ -3737,25 +3766,20 @@ static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to)
 		read = ret;
 
 	if (iov_iter_count(to) > 0 && (ret == -EFAULT || ret > 0)) {
-		const size_t left = iov_iter_count(to);
+		if (iter_is_iovec(to)) {
+			const size_t left = iov_iter_count(to);
+			const size_t size = dio_fault_in_size(iocb, to, prev_left);
 
-		if (left == prev_left) {
-			/*
-			 * We didn't make any progress since the last attempt,
-			 * fallback to a buffered read for the remainder of the
-			 * range. This is just to avoid any possibility of looping
-			 * for too long.
-			 */
-			ret = read;
+			fault_in_iov_iter_writeable(to, size);
+			prev_left = left;
+			goto again;
 		} else {
 			/*
-			 * We made some progress since the last retry or this is
-			 * the first time we are retrying. Fault in as many pages
-			 * as possible and retry.
+			 * fault_in_iov_iter_writeable() only works for iovecs,
+			 * return with a partial read and fallback to buffered
+			 * IO for the rest of the range.
 			 */
-			fault_in_iov_iter_writeable(to, left);
-			prev_left = left;
-			goto again;
+			ret = read;
 		}
 	}
 	btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
-- 
2.35.1

Re: [PATCH 1/3] btrfs: return -EAGAIN for NOWAIT dio reads/writes on compressed and inline extents

[Christoph Hellwig]

On Mon, Jul 04, 2022 at 12:42:03PM +0100, fdmanana@kernel.org wrote:
> +		 * filemap_read(), we fail to fault in pages for the read buffer,
> +		 * in which case filemap_read() returns a short read (the number
> +		 * of bytes previously read is > 0, so it does not return -EFAULT).

Two overly long lines here, which are especially annoying in block
comments as they completely break the layout.

> +		ret = (flags & IOMAP_NOWAIT) ? -EAGAIN : -ENOTBLK;

Nit: I'd find this a lot more readable with a good old if / else.

Either way the technical change looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] btrfs: don't fallback to buffered IO for NOWAIT direct IO writes

[Christoph Hellwig]

>  buffered:
> +	/*
> +	 * If we are in a NOWAIT context, then return -EAGAIN to signal the caller
> +	 * it must retry the operation in a context where blocking is acceptable,
> +	 * since we currently don't have NOWAIT semantics support for buffered IO

.. more overly long comments here.

> +	 * and may block there for many reasons (reserving space for example).
> +	 */
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		err = -EAGAIN;
> +		goto out;
> +	}

but more importantly, shouldn't this be above the buffered label? The
only places that jumps to it is the alignment check, and if the
alignment is incorrect now, it won't get any better in a workqueue
context.

Re: [PATCH 2/3] btrfs: don't fallback to buffered IO for NOWAIT direct IO writes

[Filipe Manana]

On Mon, Jul 04, 2022 at 05:00:14AM -0700, Christoph Hellwig wrote:
> >  buffered:
> > +	/*
> > +	 * If we are in a NOWAIT context, then return -EAGAIN to signal the caller
> > +	 * it must retry the operation in a context where blocking is acceptable,
> > +	 * since we currently don't have NOWAIT semantics support for buffered IO
> 
> .. more overly long comments here.

We generally don't mind about going beyond 80 characters as long as it's not too
much (83 here is reasonable), given that the new limit was changed to 100 characters
some time ago. And this applies to comments and code.

> 
> > +	 * and may block there for many reasons (reserving space for example).
> > +	 */
> > +	if (iocb->ki_flags & IOCB_NOWAIT) {
> > +		err = -EAGAIN;
> > +		goto out;
> > +	}
> 
> but more importantly, shouldn't this be above the buffered label? The
> only places that jumps to it is the alignment check, and if the
> alignment is incorrect now, it won't get any better in a workqueue
> context.

It was intentionally placed there not only because of that single goto but
to make it less error prone in case we get more gotos into that label in
the future.

Thanks.

Re: [PATCH 2/3] btrfs: don't fallback to buffered IO for NOWAIT direct IO writes

[Christoph Hellwig]

On Mon, Jul 04, 2022 at 01:11:14PM +0100, Filipe Manana wrote:
> We generally don't mind about going beyond 80 characters as long as it's not too
> much (83 here is reasonable), given that the new limit was changed to 100 characters
> some time ago. And this applies to comments and code.

The limit is for individual lines where that benefits readability.
That's never the case for block comments by definition.

> > > +	 * and may block there for many reasons (reserving space for example).
> > > +	 */
> > > +	if (iocb->ki_flags & IOCB_NOWAIT) {
> > > +		err = -EAGAIN;
> > > +		goto out;
> > > +	}
> > 
> > but more importantly, shouldn't this be above the buffered label? The
> > only places that jumps to it is the alignment check, and if the
> > alignment is incorrect now, it won't get any better in a workqueue
> > context.
> 
> It was intentionally placed there not only because of that single goto but
> to make it less error prone in case we get more gotos into that label in
> the future.

But it does the wrong thing for the only use of the goto right now..

Re: [PATCH 2/3] btrfs: don't fallback to buffered IO for NOWAIT direct IO writes

[Filipe Manana]

On Mon, Jul 04, 2022 at 05:12:28AM -0700, Christoph Hellwig wrote:
> On Mon, Jul 04, 2022 at 01:11:14PM +0100, Filipe Manana wrote:
> > We generally don't mind about going beyond 80 characters as long as it's not too
> > much (83 here is reasonable), given that the new limit was changed to 100 characters
> > some time ago. And this applies to comments and code.
> 
> The limit is for individual lines where that benefits readability.
> That's never the case for block comments by definition.

First time I'm hearing it, and never had complaints before.

> 
> > > > +	 * and may block there for many reasons (reserving space for example).
> > > > +	 */
> > > > +	if (iocb->ki_flags & IOCB_NOWAIT) {
> > > > +		err = -EAGAIN;
> > > > +		goto out;
> > > > +	}
> > > 
> > > but more importantly, shouldn't this be above the buffered label? The
> > > only places that jumps to it is the alignment check, and if the
> > > alignment is incorrect now, it won't get any better in a workqueue
> > > context.
> > 
> > It was intentionally placed there not only because of that single goto but
> > to make it less error prone in case we get more gotos into that label in
> > the future.
> 
> But it does the wrong thing for the only use of the goto right now..

Why is it wrong? The purpose it to never fallback directly to buffered IO if
it's a NOWAIT write. Moving the check to above the label, would make the
non-aligned case fallback directly to buffered IO under NOWAIT.

Re: [PATCH 2/3] btrfs: don't fallback to buffered IO for NOWAIT direct IO writes

[Christoph Hellwig]

On Mon, Jul 04, 2022 at 01:19:36PM +0100, Filipe Manana wrote:
> First time I'm hearing it, and never had complaints before.

Take a look at Documentation/process/coding-style.rst.

> Why is it wrong? The purpose it to never fallback directly to buffered IO if
> it's a NOWAIT write. Moving the check to above the label, would make the
> non-aligned case fallback directly to buffered IO under NOWAIT.

Oh, indeed. I keep forgetting that btrfs is unusual in that it does not
fail unaligned direct I/O like all other file systems but just falls
back to buffered I/O instead.

Re: [PATCH 1/3] btrfs: return -EAGAIN for NOWAIT dio reads/writes on compressed and inline extents

[David Sterba]

On Mon, Jul 04, 2022 at 04:57:41AM -0700, Christoph Hellwig wrote:
> On Mon, Jul 04, 2022 at 12:42:03PM +0100, fdmanana@kernel.org wrote:
> > +		 * filemap_read(), we fail to fault in pages for the read buffer,
> > +		 * in which case filemap_read() returns a short read (the number
> > +		 * of bytes previously read is > 0, so it does not return -EFAULT).
> 
> Two overly long lines here, which are especially annoying in block
> comments as they completely break the layout.

The only line that is not under 81 is the last one and what does not if
is "T).". This is within the acceptable overflow and I adjust many lines
in patches based on how the code looks (ie. avoiding some line breaks)
and if the potentially overflown text does not obscure the meaning.

Keeping the lines under 80 makes sense for me personally when resolving
conflicts in the 3+1 vimdiff view, but the limit is not strict and the
criteria is if the code follows the common patterns we've settled on and
what people in the btrfs group are used to, either reading or writing.
The kernel coding style does not cover everything and is a good starting
point. The rest is at
https://btrfs.wiki.kernel.org/index.php/Development_notes#Coding_style_preferences

Re: [PATCH 0/3] btrfs: a few direct IO fixes/improvements

[David Sterba]

On Mon, Jul 04, 2022 at 12:42:02PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Several fixes and improvements regarding direct IO, with the first one being
> recently reported by Dominique when using qemu with io_uring on images that
> have a mix of compressed and non-compressed extents. That in particular
> exposed a bug in qemu's handling of partial reads, fixed recently by
> Dominique, but despite that we could avoid returning a partial read when
> there's really no need to, not just for efficiency but also because many
> applications don't take into consideration partial reads (MariaDB for
> example didn't use to until recently) or are buggy dealing with them.
> More details in the changelogs.
> 
> Filipe Manana (3):
>   btrfs: return -EAGAIN for NOWAIT dio reads/writes on compressed and
>     inline extents
>   btrfs: don't fallback to buffered IO for NOWAIT direct IO writes
>   btrfs: fault in pages for dio reads/writes in a more controlled way

The first patch was already in misc-next, I've compared the code and
briefly the changelog and it looked exactly the same so I've kept the
patch, 2 and 3 added to misc-next. Thanks.