[PATCH] btrfs: fallback to buffered read if the inode has data checksum

[PATCH] btrfs: fallback to buffered read if the inode has data checksum

[Qu Wenruo]

Commit 968f19c5b1b7 ("btrfs: always fallback to buffered write if the
inode requires checksum") makes direct writes to fallback to buffered
ones if the inode has data checksum.

That commit is to avoid unreliable user space modifying the write buffer
during writeback, which can lead to data checksum mismatch.
(As the checksum is calculated, then buffer is modified, and the
modified data is submitted)

On the other hand, it's also possible that the user space program can
modify the read buffer at any time.

If the content is just read from the disk, then during checksum
verification the user space program modified the read buffer, it will
cause false alerts about csum mismatch.

Despite the possibility of false alerts, we should also keep the
behavior between direct read and direct write consistent.
If direct writes are already falling back to buffered for inodes with
checksum, the direct reads should also follow the same behavior.

So here, add the same data checksum checks for direct reads, so that
those reads will also fallback to buffered reads.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
This will make test case btrfs/267 fail, as the fallback to buffered
read will happen in a different context (workqueue), screwing up the pid
based read balance.

Furthermore true direct reads will require NODATASUM, it no longer makes
any sense to test direct IO read repair.

If this is merged, I'll send out a patch to remove btrfs/267.
---
 fs/btrfs/direct-io.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
index 6018d8c3e101..6f35fed5fa3f 100644
--- a/fs/btrfs/direct-io.c
+++ b/fs/btrfs/direct-io.c
@@ -1046,6 +1046,13 @@ ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to)
 	if (check_direct_read(inode_to_fs_info(inode), to, iocb->ki_pos))
 		return 0;
 
+	/*
+	 * To keep the behavior consistent with direct write, fall back to
+	 * buffered IO if the inode has data checksum.
+	 */
+	if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM))
+		return 0;
+
 	btrfs_inode_lock(BTRFS_I(inode), BTRFS_ILOCK_SHARED);
 again:
 	/*
-- 
2.50.1

Re: [PATCH] btrfs: fallback to buffered read if the inode has data checksum

[Qu Wenruo]

在 2025/9/26 08:20, Qu Wenruo 写道:
> Commit 968f19c5b1b7 ("btrfs: always fallback to buffered write if the
> inode requires checksum") makes direct writes to fallback to buffered
> ones if the inode has data checksum.
> 
> That commit is to avoid unreliable user space modifying the write buffer
> during writeback, which can lead to data checksum mismatch.
> (As the checksum is calculated, then buffer is modified, and the
> modified data is submitted)
> 
> On the other hand, it's also possible that the user space program can
> modify the read buffer at any time.
> 
> If the content is just read from the disk, then during checksum
> verification the user space program modified the read buffer, it will
> cause false alerts about csum mismatch.
> 
> Despite the possibility of false alerts, we should also keep the
> behavior between direct read and direct write consistent.
> If direct writes are already falling back to buffered for inodes with
> checksum, the direct reads should also follow the same behavior.
> 
> So here, add the same data checksum checks for direct reads, so that
> those reads will also fallback to buffered reads.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

A gentle ping.

Any feedback on this patch?

Thanks,
Qu

> ---
> This will make test case btrfs/267 fail, as the fallback to buffered
> read will happen in a different context (workqueue), screwing up the pid
> based read balance.
> 
> Furthermore true direct reads will require NODATASUM, it no longer makes
> any sense to test direct IO read repair.
> 
> If this is merged, I'll send out a patch to remove btrfs/267.
> ---
>   fs/btrfs/direct-io.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
> index 6018d8c3e101..6f35fed5fa3f 100644
> --- a/fs/btrfs/direct-io.c
> +++ b/fs/btrfs/direct-io.c
> @@ -1046,6 +1046,13 @@ ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to)
>   	if (check_direct_read(inode_to_fs_info(inode), to, iocb->ki_pos))
>   		return 0;
>   
> +	/*
> +	 * To keep the behavior consistent with direct write, fall back to
> +	 * buffered IO if the inode has data checksum.
> +	 */
> +	if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM))
> +		return 0;
> +
>   	btrfs_inode_lock(BTRFS_I(inode), BTRFS_ILOCK_SHARED);
>   again:
>   	/*