Re: potential data race on `delayed_rsv->full`

[David Sterba <dsterba@suse.cz>]

On Tue, Oct 15, 2019 at 02:33:11PM -0400, Meng Xu wrote:
> I am reporting a potential data race around the `delayed_rsv->full` field.

Thanks for the report.

> [thread 1] mount a btrfs image, a kernel thread of uuid_rescan will be created
> 
> btrfs_uuid_rescan_kthread
>   btrfs_end_transaction
>     __btrfs_end_transaction
>       btrfs_trans_release_metadata
>         btrfs_block_rsv_release
>           __btrfs_block_rsv_release
>             --> [READ] else if (block_rsv != global_rsv && !delayed_rsv->full)
>                                                             ^^^^^^^^^^^^^^^^^
> 
> 
> [thread 2] do a mkdir syscall on the mounted image
> 
> __do_sys_mkdir
>   do_mkdirat
>     vfs_mkdir
>       btrfs_mkdir
>         btrfs_new_inode
>           btrfs_insert_empty_items
>             btrfs_cow_block
>               __btrfs_cow_block
>                 alloc_tree_block_no_bg_flush
>                   btrfs_alloc_tree_block
>                     btrfs_add_delayed_tree_ref
>                       btrfs_update_delayed_refs_rsv
>                         --> [WRITE] delayed_rsv->full = 0;
>                                     ^^^^^^^^^^^^^^^^^^^^^
> 
> 
> I could confirm that this is a data race by manually adding and adjusting
> delays before the read and write statements although I am not very sure
> about the implication of such a data race (e.g., crashing btrfs or causing
> violations of assumptions). I would appreciate if you could help check on
> this potential bug and advise whether this is a harmful data race or it
> is intended.

The race is there, as the access is unprotected, but it does not seem to
have serious implications. The race is for space, which happens all the
time, and if the reservations cannot be satisfied there goes ENOSPC.

In this particular case I wonder if the uuid thread is important or if
this would happen with anything that calls btrfs_end_transaction.

Depending on the value of ->full, __btrfs_block_rsv_release decides
where to return the reservation, and block_rsv_release_bytes handles a
NULL pointer for block_rsv and if it's not NULL then it double checks
the full status under a lock.

So the unlocked and racy access is only advisory and in the worst case
the block reserve is found !full and becomes full in the meantime,
but properly handled.

I've CCed Josef to review the analysis.