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

[Meng Xu <mengxu.gatech@gmail.com>]

Hi David,

Thank you for the confirmation and the additional information.

I feel the same that this race may not lead to serious issues, but would 
rather prefer a confirmation from the developers. Thank you again for 
your time!

Best Regards,
Meng


On 11/1/19 11:45 AM, David Sterba wrote:
> 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.