On 24.09.19 г. 23:50 ч., Josef Bacik wrote:
> When free'ing extents in a block group we check to see if the block
> group is not cached, and then cache it if we need to. However we'll
> just carry on as long as we're loading the cache. This is problematic
> because we are dirtying the block group here. If we are fast enough we
> could do a transaction commit and clear the free space cache while we're
This would imply a race condition between loading the space cache and
running delayed refs - because there where
__btrfs_free_extent->btrfs_update_blockgroup(alloc=0) is called from, no ?
> still loading the space cache in another thread. This truncates the
> free space inode, which will keep it from loading the space cache.
>
> Fix this by using the btrfs_block_group_cache_done helper so that we try
> to load the space cache unconditionally here, which will result in the
> caller waiting for the fast caching to complete and keep us from
> truncating the free space inode.
So the problem was that cache->cached == BTRFS_CACHE_NO misses other
interim states e.g. CACHE_STARTED/CACHE_FAST. Which leads to the
aforementioned race, correct?
>
> Signed-off-by: Josef Bacik <jo...@toxicpanda.com>
Based on our offlist discussion:
Reviewed-by: Nikolay Borisov <nbori...@suse.com>
> ---
> fs/btrfs/block-group.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index bf7e3f23bba7..d7b3a516f27a 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -2661,7 +2661,7 @@ int btrfs_update_block_group(struct btrfs_trans_handle
> *trans,
> * is because we need the unpinning stage to actually add the
> * space back to the block group, otherwise we will leak space.
> */
> - if (!alloc && cache->cached == BTRFS_CACHE_NO)
> + if (!alloc && !btrfs_block_group_cache_done(cache))
> btrfs_cache_block_group(cache, 1);
>
> byte_in_group = bytenr - cache->key.objectid;
>
