Re: [PATCH 01/21] btrfs: fix race when refilling delayed refs block reserve

[Josef Bacik <josef@toxicpanda.com>]

On Fri, Sep 08, 2023 at 01:09:03PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If we have two (or more) tasks attempting to refill the delayed refs block
> reserve we can end up with the delayed block reserve being over reserved,
> that is, with a reserved space greater than its size. If this happens, we
> are holding to more reserved space than necessary, however it may or may
> not be harmless. In case the delayed refs block reserve size later
> increases to match or go beyond the reserved space, then nothing bad
> happens, we end up simply avoiding doing a space reservation later at the
> expense of doing it now. However if the delayed refs block reserve size
> never increases to match its reserved size, then at unmount time we will
> trigger the following warning from btrfs_release_global_block_rsv():
> 
>    WARN_ON(fs_info->delayed_block_rsv.reserved > 0);
> 
> The race happens like this:
> 
> 1) The delayed refs block reserve has a size of 8M and a reserved space of
>    6M for example;
> 
> 2) Task A calls btrfs_delayed_refs_rsv_refill();
> 
> 3) Task B also calls btrfs_delayed_refs_rsv_refill();
> 
> 4) Task A sees there's a 2M difference between the size and the reserved
>    space of the delayed refs rsv, so it will reserve 2M of space by
>    calling btrfs_reserve_metadata_bytes();
> 
> 5) Task B also sees that 2M difference, and like task A, it reserves
>    another 2M of metadata space;
> 
> 6) Both task A and task B increase the reserved space of block reserve
>    by 2M, by calling btrfs_block_rsv_add_bytes(), so the block reserve
>    ends up with a size of 8M and a reserved space of 10M;
> 
> 7) As delayed refs are run and their space released, the delayed refs
>    block reserve ends up with a size of 0 and a reserved space of 2M;
> 

This part shouldn't happen, delayed refs space only manipulates
delayed_refs_rsv->size, so when we drop their space, we do

btrfs_delayed_refs_rsv_release
 -> btrfs_block_rsv_release
  -> block_rsv_release_bytes

which does

        spin_lock(&block_rsv->lock);
        if (num_bytes == (u64)-1) {
                num_bytes = block_rsv->size;
                qgroup_to_release = block_rsv->qgroup_rsv_size;
        }
        block_rsv->size -= num_bytes;
        if (block_rsv->reserved >= block_rsv->size) {
                num_bytes = block_rsv->reserved - block_rsv->size;
                block_rsv->reserved = block_rsv->size;
                block_rsv->full = true;
        }

so if A and B race and the reservation is now much larger than size, the extra
will be cut off at the end when we call btrfs_delayed_refs_rsv_release.

Now I'm all for not holding the over-reserve for that long, but this analysis is
incorrect, we would have to somehow do the btrfs_delayed_refs_rsv_refill() and
then not call btrfs_delayed_refs_rsv_release() at some point.

This *could* happen I suppose by starting a trans handle and not modifying
anything while the delayed refs are run and finish before we do our reserve, and
in that case we didn't generate a delayed ref that could later on call
btrfs_delayed_refs_rsv_release() to clear the excess reservation, but that's a
decidedly different problem with a different solution.  Thanks,

Josef