Re: [PATCH v2 1/4] btrfs: do not allow fast fsync if there is any async dio write

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2026/11/10 20:31, Filipe Manana 写道:
> On Tue, Nov 10, 2026 at 7:06 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>>
>>
>> 在 2026/11/10 16:42, Qu Wenruo 写道:
>>> Sashiko reported a possible race where an async direct write happened, a
>>> fast fsync() can return before the write reaches the storage:
>>>
>>>     CPU0 (io_uring/AIO direct write)| CPU1
>>>     --------------------------------+-----------------------------------
>>>     btrfs_direct_write()
>>>     |-btrfs_dio_write()
>>>     | \-__iomap_dio_rw()
>>>     |   |-btrfs_dio_iomap_begin()
>>>     |   | |-btrfs_new_extent_direct()
>>>     |   |   ^^^ new extent + ordered extent, data not written yet
>>>     |   |
>>>     |   |-btrfs_dio_submit_io()
>>>     |   | |-btrfs_submit_bbio()
>>>     |   |   ^^^ bio in flight, not finished
>>>     |   |
>>>     |   \- return -EIOCBQUEUED
>>>     \-btrfs_inode_unlock()
>>>                                       btrfs_sync_file()
>>>                                       |-btrfs_get_ordered_extents_for_logging()
>>>                                       | ^^^grabs the incomplete OE
>>>                                       |
>>>                                       |-filemap_fdatawait_range()
>>>                                       | ^^^no folio under writeback, returns
>>>                                       |    immediately
>>>                                       |
>>>                                       |-btrfs_log_dentry_safe()
>>>                                       | | ...
>>>                                       | \-btrfs_log_changed_extents()
>>>                                       |   ^^^ logs file extent item for the new extent
>>>                                       |       OE only marked PENDING, nobody waits for its IO
>>>                                       |
>>>                                       \-btrfs_sync_log()
>>>                                         ^^^ log tree is on disk, data bio still in flight
>>>                                             fsync() returns
>>>
>>>     power loss, the data bio never reached disk
>>>
>>>     mount
>>>     |-btrfs_recover_log_trees()
>>>       ^^^ replays the extent item, file now points to unwritten blocks
>>>
>>> To prevent such problem, disable fast fsync for async direct writes.
>>>
>>> Fixes: 487781796d30 ("btrfs: make fast fsyncs wait only for writeback")
>>> Link: https://sashiko.dev/#/bug/linux-ee047626-2fa7-46d5-b543-861136993fc7
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>    fs/btrfs/direct-io.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
>>> index ed3914257d39..6e796163015e 100644
>>> --- a/fs/btrfs/direct-io.c
>>> +++ b/fs/btrfs/direct-io.c
>>> @@ -974,6 +974,17 @@ ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
>>>
>>>        if (IS_ERR_OR_NULL(dio)) {
>>>                ret = PTR_ERR_OR_ZERO(dio);
>>> +             /*
>>> +              * The async direct write is not waiting for its IO to finish.
>>> +              *
>>> +              * If fsync() went through the fast path, the direct write OE
>>> +              * will be logged, but the data may not yet reach disks.
>>> +              * And fsync() returned without waiting for the data.
>>> +              *
>>> +              * So for async direct writes, we can not go to fast sync path.
>>> +              */
>>> +             if (ret == -EIOCBQUEUED)
>>> +                     btrfs_set_inode_full_sync(BTRFS_I(inode));
>>
>> Unfortunately btrfs_set_inode_full_sync() is not safe if the caller is
>> only holding a shared inode lock.
>>
>> So it can still race with fsync.
> 
> How can it race? An fsync takes the inode lock in exclusive mode.

CPU0 (sync DIO write within EOF)  CPU1 (async DIO write within EOF)
btrfs_direct_write()              btrfs_direct_write()
   btrfs_inode_lock(..., SHARED)     btrfs_inode_lock(..., SHARED)
   iomap_dio_complete()              btrfs_dio_write()
     ...                               ...
     btrfs_sync_file()                 btrfs_dio_iomap_begin()
       down_write(&i_mmap_lock)        btrfs_dio_submit_io()
       full_sync = test_bit(...)
       /* full_sync is false, skips
          btrfs_wait_ordered_range() */
                                     btrfs_set_inode_full_sync()
       btrfs_log_dentry_safe()
         ...
         btrfs_log_inode()
           test_bit(NEEDS_FULL_SYNC) <-- now true!
           clear_bit(NEEDS_FULL_SYNC)
           copy_inode_items_to_log()
           /* skips btrfs_log_changed_extents()
              and purges modified_extents */

> 
>>
>> The shared inode lock for dio writes is introduced by commit
>> e9adabb9712e ("btrfs: use shared lock for direct writes within EOF").
>>
>> This is making me wonder if the shared inode lock for direct IO is a
>> good idea in the first place.
> 
> Why would it not be a good idea?

OK, then I guess encoded write can be fixed using the same wait, other 
than using that FULL_SYNC flag.

> 
>>
>> Or should I change fsync() to wait for direct IOs instead of using
>> btrfs_set_inode_full_sync()?
> 
> Under which conditions is  -EIOCBQUEUED returned?
> Are these async writes (like io_uring)?

Yes, AIO/io_uring.
> 
> If that's common, then forcing full fsyncs badly affects performance.
> 
> We had fast fsync long before 487781796d30.
> It got introduced back in 2012 in commit 5dc562c541e1, then removed in
> b5e6c3e170b770 and then added back (in a slightly different form) in
> 487781796d30.
> So are you sure 487781796d30 is the culprit?

At least that's the newest commit that doesn't wait for async direct 
writes nor encoded writes.

Thanks,
Qu

> 
> 
>>
>> Thanks,
>> Qu
>>>        } else {
>>>                /*
>>>                 * If we have a synchronous write, we must make sure the fsync
>>
>>
>