Re: [PATCH] btrfs: check snapshot_force_cow earlier in can_nocow_file_extent

[Filipe Manana]

On Tue, Feb 17, 2026 at 5:17 PM Guan-Jie Chen <jk.chen1095@xxxxxxxxx> wrote:
>
> Hi Filipe,
>
> Thanks again for picking up the patch.
>
> I realized that I should use my full real name and my personal email
> for the Signed-off-by line. Could you please update it to:
>
> Signed-off-by: Chen Guan Jie <jk.chen1095@xxxxxxxxx>

Ok, updated it.

>
> Sorry for the trouble, and thank you for your help.
>
> Best regards,
> Chen Guan Jie
>
> On Tue, Feb 17, 2026 at 19:42 Filipe Manana <fdmanana@xxxxxxxxxx> wrote:
>>
>> On Mon, Feb 16, 2026 at 10:17 PM jkchen <jk.chen1095@xxxxxxxxx> wrote:
>> >
>> > When a snapshot is being created, the atomic counter snapshot_force_cow
>> > is incremented to force incoming writes to fallback to COW. This is a
>> > critical mechanism to protect the consistency of the snapshot being taken.
>> >
>> > Currently, can_nocow_file_extent() checks this counter only after
>> > performing several checks, most notably the expensive cross-reference
>> > check via btrfs_cross_ref_exist(). btrfs_cross_ref_exist() releases the
>> > path and performs a search in the extent tree or backref cache, which
>> > involves btree traversals and locking overhead.
>> >
>> > This patch moves the snapshot_force_cow check to the very beginning of
>> > can_nocow_file_extent().
>> >
>> > This reordering is safe and beneficial because:
>> > 1. args->writeback_path is invariant for the duration of the call (set
>> >    by caller run_delalloc_nocow).
>> > 2. is_freespace_inode is a static property of the inode.
>> > 3. The state of snapshot_force_cow is driven by the btrfs_mksnapshot
>> >    process. Checking it earlier does not change the outcome of the NOCOW
>> >    decision, but effectively prunes the expensive code path when a
>> >    fallback to COW is inevitable.
>> >
>> > By failing fast when a snapshot is pending, we avoid the unnecessary
>> > overhead of btrfs_cross_ref_exist() and other extent item checks in the
>> > scenario where NOCOW is already known to be impossible.
>> >
>> > Signed-off-by: jkchen <jkchen@xxxxxxxxxx>
>>
>> Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>
>>
>> Looks good, I'll add it to the github for-next branch, thanks.
>>
>> > ---
>> >  fs/btrfs/inode.c | 10 +++++-----
>> >  1 file changed, 5 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> > index 845164485..3549031f3 100644
>> > --- a/fs/btrfs/inode.c
>> > +++ b/fs/btrfs/inode.c
>> > @@ -1921,6 +1921,11 @@ static int can_nocow_file_extent(struct btrfs_path *path,
>> >         int ret = 0;
>> >         bool nowait = path->nowait;
>> >
>> > +       /* If there are pending snapshots for this root, we must COW. */
>> > +       if (args->writeback_path && !is_freespace_inode &&
>> > +           atomic_read(&root->snapshot_force_cow))
>> > +               goto out;
>> > +
>> >         fi = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_file_extent_item);
>> >         extent_type = btrfs_file_extent_type(leaf, fi);
>> >
>> > @@ -1982,11 +1987,6 @@ static int can_nocow_file_extent(struct btrfs_path *path,
>> >                 path = NULL;
>> >         }
>> >
>> > -       /* If there are pending snapshots for this root, we must COW. */
>> > -       if (args->writeback_path && !is_freespace_inode &&
>> > -           atomic_read(&root->snapshot_force_cow))
>> > -               goto out;
>> > -
>> >         args->file_extent.num_bytes = min(args->end + 1, extent_end) - args->start;
>> >         args->file_extent.offset += args->start - key->offset;
>> >         io_start = args->file_extent.disk_bytenr + args->file_extent.offset;
>> > --
>> > 2.43.0
>> >
>> >