Re: [PATCH v2 3/5] btrfs: wait for ordered extent IO in fast fsync

[Qu Wenruo <wqu@suse.com>]

在 2026/11/4 04:40, Filipe Manana 写道:
> On Tue, Nov 3, 2026 at 3:57 PM Josef Bacik <josef@toxicpanda.com> wrote:
>>
>> A fast fsync waits for writeback and then checks
>> BTRFS_INODE_COW_WRITE_ERROR to see whether a COW write failed. If one
>> did, it must wait for the ordered extents to complete before logging,
>> since until then the extent maps of the failed range point to extents
>> that were never written. Logging them puts unwritten extents in place
>> of the data that was there before when the log is replayed.
>>
>> The flag can be set too late for that check. end_bbio_data_write()
>> clears writeback before it finishes the ordered extent:
>>
>>    end_bbio_data_write()            fsync
>>
>>    btrfs_folio_clear_writeback()
>>                                     filemap_fdatawait_range()
>>                                     BTRFS_INODE_COW_WRITE_ERROR clear
>>                                     btrfs_log_changed_extents()
>>    btrfs_finish_ordered_extent()
>>      set BTRFS_INODE_COW_WRITE_ERROR
>>
>> The write paths that fail before submitting a bio, writepage_delalloc()
>> and submit_write_sector(), clear writeback and finish the ordered
>> extent through btrfs_mark_ordered_io_finished(), which never sets the
>> flag at all.
>>
>> Fix by dropping the flag and having a fast fsync wait for
>> BTRFS_ORDERED_IO_DONE on the ordered extents it collected for logging.
>> can_finish_ordered_extent() sets it only after recording any error with
>> BTRFS_ORDERED_IOERR. The wait costs little, because IO_DONE is set
>> right after writeback is cleared, not when the ordered extent completes.
>>
>> Fixes: f13e01b89daf ("btrfs: ensure fast fsync waits for ordered extents after a write failure")
>> Fixes: a11d6912fdd9 ("btrfs: remove folio parameter from ordered io related functions")

In fact, if we merge this one, it will also fix the encoded write bug.

I think if we go this path, I can revert the fix of "btrfs: skip fast 
sync for inodes with encoded writes".

Thanks,
Qu

>> CC: stable@vger.kernel.org # 6.10+
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> The idea looks sound to me.
> 
> Sashiko found something and from a quick glance it seems valid to me:
> 
> https://sashiko.dev/#/patchset/20261103-b4-btrfs-data-integrity-v2-0-577dc2690061%40toxicpanda.com
> 
> Thanks.
> 
> 
>> ---
>>   fs/btrfs/btrfs_inode.h  | 10 ----------
>>   fs/btrfs/file.c         | 52 +++++++++++++++++++++++++++++++++++++++----------
>>   fs/btrfs/ordered-data.c | 31 -----------------------------
>>   3 files changed, 42 insertions(+), 51 deletions(-)
>>
>> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
>> index f7aa0d145185..65ba8ec23258 100644
>> --- a/fs/btrfs/btrfs_inode.h
>> +++ b/fs/btrfs/btrfs_inode.h
>> @@ -89,16 +89,6 @@ enum {
>>          BTRFS_INODE_FREE_SPACE_INODE,
>>          /* Set when there are no capabilities in XATTs for the inode. */
>>          BTRFS_INODE_NO_CAP_XATTR,
>> -       /*
>> -        * Set if an error happened when doing a COW write before submitting a
>> -        * bio or during writeback. Used for both buffered writes and direct IO
>> -        * writes. This is to signal a fast fsync that it has to wait for
>> -        * ordered extents to complete and therefore not log extent maps that
>> -        * point to unwritten extents (when an ordered extent completes and it
>> -        * has the BTRFS_ORDERED_IOERR flag set, it drops extent maps in its
>> -        * range).
>> -        */
>> -       BTRFS_INODE_COW_WRITE_ERROR,
>>          /*
>>           * Set by reflink while it holds the source's VFS inode lock shared.
>>           * Direct IO writes also take that lock shared; one that finds this bit
>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>> index f978c6524aa0..4ae9aed8993f 100644
>> --- a/fs/btrfs/file.c
>> +++ b/fs/btrfs/file.c
>> @@ -1512,6 +1512,42 @@ static inline bool skip_inode_logging(const struct btrfs_log_ctx *ctx)
>>          return false;
>>   }
>>
>> +/*
>> + * Wait for the IO of the ordered extents collected for logging to be done, and
>> + * return true if a COW write among them failed.
>> + *
>> + * Waiting for writeback is not enough to know that: the end IO handlers clear
>> + * writeback before they account the IO in the ordered extent. An ordered
>> + * extent gets BTRFS_ORDERED_IO_DONE only once all of its IO is accounted, and
>> + * can_finish_ordered_extent() sets BTRFS_ORDERED_IOERR before that.
>> + */
>> +static bool cow_write_failed(struct btrfs_inode *inode,
>> +                            struct list_head *ordered_extents)
>> +{
>> +       struct btrfs_ordered_extent *ordered;
>> +       bool failed = false;
>> +
>> +       list_for_each_entry(ordered, ordered_extents, log_list)
>> +               wait_event(ordered->wait,
>> +                          test_bit(BTRFS_ORDERED_IO_DONE, &ordered->flags));
>> +
>> +       /*
>> +        * can_finish_ordered_extent() records a write error before it sets
>> +        * BTRFS_ORDERED_IO_DONE, both under this lock.
>> +        */
>> +       spin_lock(&inode->ordered_tree_lock);
>> +       list_for_each_entry(ordered, ordered_extents, log_list) {
>> +               if (test_bit(BTRFS_ORDERED_IOERR, &ordered->flags) &&
>> +                   !test_bit(BTRFS_ORDERED_NOCOW, &ordered->flags)) {
>> +                       failed = true;
>> +                       break;
>> +               }
>> +       }
>> +       spin_unlock(&inode->ordered_tree_lock);
>> +
>> +       return failed;
>> +}
>> +
>>   /*
>>    * fsync call for both files and directories.  This logs the inode into
>>    * the tree log instead of forcing full commits whenever possible.
>> @@ -1615,7 +1651,7 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>>           * IO of a lower priority task while holding a transaction open.
>>           *
>>           * For a full fsync we wait for the ordered extents to complete while
>> -        * for a fast fsync we wait just for writeback to complete, and then
>> +        * for a fast fsync we wait just for their IO to complete, and then
>>           * attach the ordered extents to the transaction so that a transaction
>>           * commit waits for their completion, to avoid data loss if we fsync,
>>           * the current transaction commits before the ordered extents complete
>> @@ -1627,7 +1663,6 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>>           */
>>          if (full_sync || btrfs_is_zoned(fs_info)) {
>>                  ret = btrfs_wait_ordered_range(inode, start, len);
>> -               clear_bit(BTRFS_INODE_COW_WRITE_ERROR, &inode->runtime_flags);
>>          } else {
>>                  /*
>>                   * Get our ordered extents as soon as possible to avoid doing
>> @@ -1640,15 +1675,12 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>>                          goto out_release_extents;
>>
>>                  /*
>> -                * Check and clear the BTRFS_INODE_COW_WRITE_ERROR now after
>> -                * starting and waiting for writeback, because for buffered IO
>> -                * it may have been set during the end IO callback
>> -                * (end_bbio_data_write() -> btrfs_finish_ordered_extent()) in
>> -                * case an error happened and we need to wait for ordered
>> -                * extents to complete so that any extent maps that point to
>> -                * unwritten locations are dropped and we don't log them.
>> +                * If a COW write failed, before submitting a bio or during IO,
>> +                * the extent maps of its range point to unwritten locations
>> +                * until the ordered extent completes and drops them. Wait for
>> +                * that, so that we don't log them.
>>                   */
>> -               if (test_and_clear_bit(BTRFS_INODE_COW_WRITE_ERROR, &inode->runtime_flags))
>> +               if (cow_write_failed(inode, &ctx.ordered_extents))
>>                          ret = btrfs_wait_ordered_range(inode, start, len);
>>          }
>>
>> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
>> index df74c75d6c29..27aca31866fd 100644
>> --- a/fs/btrfs/ordered-data.c
>> +++ b/fs/btrfs/ordered-data.c
>> @@ -436,37 +436,6 @@ void btrfs_finish_ordered_extent(struct btrfs_ordered_extent *ordered,
>>                                          uptodate);
>>          spin_unlock(&inode->ordered_tree_lock);
>>
>> -       /*
>> -        * If this is a COW write it means we created new extent maps for the
>> -        * range and they point to unwritten locations if we got an error either
>> -        * before submitting a bio or during IO.
>> -        *
>> -        * We have marked the ordered extent with BTRFS_ORDERED_IOERR, and we
>> -        * are queuing its completion below. During completion, at
>> -        * btrfs_finish_one_ordered(), we will drop the extent maps for the
>> -        * unwritten extents.
>> -        *
>> -        * However because completion runs in a work queue we can end up having
>> -        * a fast fsync running before that. In the case of direct IO, once we
>> -        * unlock the inode the fsync might start, and we queue the completion
>> -        * before unlocking the inode. In the case of buffered IO when writeback
>> -        * finishes (end_bbio_data_write()) we queue the completion, so if the
>> -        * writeback was triggered by a fast fsync, the fsync might start
>> -        * logging before ordered extent completion runs in the work queue.
>> -        *
>> -        * The fast fsync will log file extent items based on the extent maps it
>> -        * finds, so if by the time it collects extent maps the ordered extent
>> -        * completion didn't happen yet, it will log file extent items that
>> -        * point to unwritten extents, resulting in a corruption if a crash
>> -        * happens and the log tree is replayed. Note that a fast fsync does not
>> -        * wait for completion of ordered extents in order to reduce latency.
>> -        *
>> -        * Set a flag in the inode so that the next fast fsync will wait for
>> -        * ordered extents to complete before starting to log.
>> -        */
>> -       if (!uptodate && !test_bit(BTRFS_ORDERED_NOCOW, &ordered->flags))
>> -               set_bit(BTRFS_INODE_COW_WRITE_ERROR, &inode->runtime_flags);
>> -
>>          if (ret)
>>                  btrfs_queue_ordered_fn(ordered);
>>   }
>>
>> --
>> 2.55.0
>>
>>