[PATCH RFC] btrfs: wait for encoded writes in fast fsync

[PATCH RFC] btrfs: wait for encoded writes in fast fsync

[Qu Wenruo]

[BUG REPORT]
Sashiko reported a bug that encoded writes can have its file extents
recorded before their data reaches disks.

[CAUSE]
An encoded write returns to the caller as soon as its bio is submitted,
and its folios are not in the inode's page cache:

  btrfs_ioctl_encoded_write()
   \- btrfs_do_encoded_write()
       |- btrfs_alloc_ordered_extent()
       \- btrfs_submit_compressed_write()   <- returns before the bio ends

A fast fsync only waits for writeback of the folios in the page cache,
so it has nothing to wait for on such an ordered extent:

  btrfs_sync_file()
   |- btrfs_get_ordered_extents_for_logging()
   |- filemap_fdatawait_range()             <- no folio under writeback
   \- btrfs_log_dentry_safe()
       \- ...
           \- log_extent_csums()
               \- log_csums()               <- csum_list may be in flight

This has two problems:

- fsync can return before the data of the encoded write is on disk, while
  the log tree describing it is already persisted.

- With async data checksum generation, the sums are attached to
  ordered->csum_list by btrfs_csum_one_bio() before the checksums are
  calculated.  log_extent_csums() can then log zeroed or partially
  calculated checksums.

For buffered writes this is not a problem, as writeback of a folio ends
after the bio endio, which in turn runs after the checksums are
generated.

[FIX]
Fix it by waiting for the encoded ordered extents in the fast fsync path
before waiting for writeback.

And if the encoded OE failed, also fail the whole fsync early.

Such an ordered extent can only complete after its bio is finished,
meaning the data is on disk and the checksums are fully generated.

Link: https://sashiko.dev/#/patchset/8eb322321cac98013cc46dfe8d89fa58c1ad3dee.1793766229.git.wqu%40suse.com
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
Reason for RFC:

This is only covering encoded IO, not direct IO.

As we should not mix direct and buffered IO in the first place, fsync()
definitely counts as a function in the buffered IO family.

The problem is whether we should treat encoded IOs as buffered or direct?

The encoded write itself doesn't wait for write to finish, so it looks
like buffered, but on the other hand it doesn't touch page cache at all,
which is similar to direct IO.

Personally speaking, I tend to treat encoded IO more like buffered, thus
that's why I sent the patch out, other than rejecting the Sashiko review
result.
---
 fs/btrfs/file.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 20e15dc30bfb..474bbb0adfef 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1629,12 +1629,31 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 		ret = btrfs_wait_ordered_range(inode, start, len);
 		clear_bit(BTRFS_INODE_COW_WRITE_ERROR, &inode->runtime_flags);
 	} else {
+		struct btrfs_ordered_extent *ordered;
+
 		/*
 		 * Get our ordered extents as soon as possible to avoid doing
 		 * checksum lookups in the csum tree, and use instead the
 		 * checksums attached to the ordered extents.
 		 */
 		btrfs_get_ordered_extents_for_logging(inode, &ctx.ordered_extents);
+
+		/*
+		 * Encoded writes don't mark any folio of the inode as writeback,
+		 * thus the fdatawait below can't wait for them.  Wait for them
+		 * explicitly, so that their data is on disk and their checksums
+		 * are fully generated before we log them.
+		 */
+		list_for_each_entry(ordered, &ctx.ordered_extents, log_list) {
+			if (test_bit(BTRFS_ORDERED_ENCODED, &ordered->flags)) {
+				btrfs_start_ordered_extent(ordered);
+				if (test_bit(BTRFS_ORDERED_IOERR, &ordered->flags)) {
+					ret = -EIO;
+					goto out_release_extents;
+				}
+			}
+		}
+
 		ret = filemap_fdatawait_range(inode->vfs_inode.i_mapping, start, end);
 		if (ret)
 			goto out_release_extents;
-- 
2.56.0

Re: [PATCH RFC] btrfs: wait for encoded writes in fast fsync

[Christoph Hellwig]

On Wed, Nov 04, 2026 at 05:40:46PM +1030, Qu Wenruo wrote:
> This is only covering encoded IO, not direct IO.
> 
> As we should not mix direct and buffered IO in the first place, fsync()
> definitely counts as a function in the buffered IO family.

It absolutely does not.  fsync/fdatasync covers all data and metadata
changes.  It is a fundamental part of O_DIRECT data integrity
operations to write back metadata and to flush disk caches.

Re: [PATCH RFC] btrfs: wait for encoded writes in fast fsync

[Qu Wenruo]

在 2026/11/4 18:41, Christoph Hellwig 写道:
> On Wed, Nov 04, 2026 at 05:40:46PM +1030, Qu Wenruo wrote:
>> This is only covering encoded IO, not direct IO.
>>
>> As we should not mix direct and buffered IO in the first place, fsync()
>> definitely counts as a function in the buffered IO family.
> 
> It absolutely does not.  fsync/fdatasync covers all data and metadata
> changes.  It is a fundamental part of O_DIRECT data integrity
> operations to write back metadata and to flush disk caches.

Even if there is a direct write in the fly? How would you even detect 
it, there is no dirty page cache in the range.

Re: [PATCH RFC] btrfs: wait for encoded writes in fast fsync

[Christoph Hellwig]

On Wed, Nov 04, 2026 at 06:44:55PM +1030, Qu Wenruo wrote:
> > It absolutely does not.  fsync/fdatasync covers all data and metadata
> > changes.  It is a fundamental part of O_DIRECT data integrity
> > operations to write back metadata and to flush disk caches.
> 
> Even if there is a direct write in the fly? How would you even detect it,
> there is no dirty page cache in the range.

In-flight I/O is per definition not captured by fsync.  fsync applies
to previously completed I/O.

Re: [PATCH RFC] btrfs: wait for encoded writes in fast fsync

[Filipe Manana]

On Wed, Nov 4, 2026 at 8:15 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 2026/11/4 18:41, Christoph Hellwig 写道:
> > On Wed, Nov 04, 2026 at 05:40:46PM +1030, Qu Wenruo wrote:
> >> This is only covering encoded IO, not direct IO.
> >>
> >> As we should not mix direct and buffered IO in the first place, fsync()
> >> definitely counts as a function in the buffered IO family.
> >
> > It absolutely does not.  fsync/fdatasync covers all data and metadata
> > changes.  It is a fundamental part of O_DIRECT data integrity
> > operations to write back metadata and to flush disk caches.
>
> Even if there is a direct write in the fly? How would you even detect
> it, there is no dirty page cache in the range.

In btrfs direct IO writes lock the inode (vfs lock), create ordered
extents and then unlock.
Fsync always locks the inode (in exclusive/write mode), so it will see
the ordered extents and take proper action.

>

Re: [PATCH RFC] btrfs: wait for encoded writes in fast fsync

[Filipe Manana]

On Wed, Nov 4, 2026 at 7:11 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [BUG REPORT]
> Sashiko reported a bug that encoded writes can have its file extents
> recorded before their data reaches disks.
>
> [CAUSE]
> An encoded write returns to the caller as soon as its bio is submitted,
> and its folios are not in the inode's page cache:
>
>   btrfs_ioctl_encoded_write()
>    \- btrfs_do_encoded_write()
>        |- btrfs_alloc_ordered_extent()
>        \- btrfs_submit_compressed_write()   <- returns before the bio ends
>
> A fast fsync only waits for writeback of the folios in the page cache,
> so it has nothing to wait for on such an ordered extent:
>
>   btrfs_sync_file()
>    |- btrfs_get_ordered_extents_for_logging()
>    |- filemap_fdatawait_range()             <- no folio under writeback
>    \- btrfs_log_dentry_safe()
>        \- ...
>            \- log_extent_csums()
>                \- log_csums()               <- csum_list may be in flight
>
> This has two problems:
>
> - fsync can return before the data of the encoded write is on disk, while
>   the log tree describing it is already persisted.
>
> - With async data checksum generation, the sums are attached to
>   ordered->csum_list by btrfs_csum_one_bio() before the checksums are
>   calculated.  log_extent_csums() can then log zeroed or partially
>   calculated checksums.
>
> For buffered writes this is not a problem, as writeback of a folio ends
> after the bio endio, which in turn runs after the checksums are
> generated.
>
> [FIX]
> Fix it by waiting for the encoded ordered extents in the fast fsync path
> before waiting for writeback.
>
> And if the encoded OE failed, also fail the whole fsync early.
>
> Such an ordered extent can only complete after its bio is finished,
> meaning the data is on disk and the checksums are fully generated.
>
> Link: https://sashiko.dev/#/patchset/8eb322321cac98013cc46dfe8d89fa58c1ad3dee.1793766229.git.wqu%40suse.com
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Reason for RFC:
>
> This is only covering encoded IO, not direct IO.

And what is missing regarding direct IO?

Direct IO writes lock the inode, create ordered extents and wait for
them to complete before unlocking them, so an fsync will not miss
anything to my knowledge, as it locks the inode.

>
> As we should not mix direct and buffered IO in the first place, fsync()
> definitely counts as a function in the buffered IO family.

Direct IO and fsync are not incompatible in any way.

Applications that use direct IO writes need to call fsync to ensure persistence.
Having worked in the database industry before, I can assure you using
direct IO and fsync is used in the real world.

>
> The problem is whether we should treat encoded IOs as buffered or direct?
>
> The encoded write itself doesn't wait for write to finish, so it looks
> like buffered, but on the other hand it doesn't touch page cache at all,
> which is similar to direct IO.
>
> Personally speaking, I tend to treat encoded IO more like buffered, thus
> that's why I sent the patch out, other than rejecting the Sashiko review
> result.
> ---
>  fs/btrfs/file.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 20e15dc30bfb..474bbb0adfef 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1629,12 +1629,31 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>                 ret = btrfs_wait_ordered_range(inode, start, len);
>                 clear_bit(BTRFS_INODE_COW_WRITE_ERROR, &inode->runtime_flags);
>         } else {
> +               struct btrfs_ordered_extent *ordered;
> +
>                 /*
>                  * Get our ordered extents as soon as possible to avoid doing
>                  * checksum lookups in the csum tree, and use instead the
>                  * checksums attached to the ordered extents.
>                  */
>                 btrfs_get_ordered_extents_for_logging(inode, &ctx.ordered_extents);
> +
> +               /*
> +                * Encoded writes don't mark any folio of the inode as writeback,
> +                * thus the fdatawait below can't wait for them.  Wait for them
> +                * explicitly, so that their data is on disk and their checksums
> +                * are fully generated before we log them.
> +                */
> +               list_for_each_entry(ordered, &ctx.ordered_extents, log_list) {
> +                       if (test_bit(BTRFS_ORDERED_ENCODED, &ordered->flags)) {
> +                               btrfs_start_ordered_extent(ordered);

We don't need to wait for the ordered extents to fully complete, we
only need to wait for the IO to complete.
So just:

wait_event(entry->wait, test_bit(BTRFS_ORDERED_IO_DONE, &entry->flags));

Otherwise, it adds unnecessary latency.

Thanks.


> +                               if (test_bit(BTRFS_ORDERED_IOERR, &ordered->flags)) {
> +                                       ret = -EIO;
> +                                       goto out_release_extents;
> +                               }
> +                       }
> +               }
> +
>                 ret = filemap_fdatawait_range(inode->vfs_inode.i_mapping, start, end);
>                 if (ret)
>                         goto out_release_extents;
> --
> 2.56.0
>
>