On 17.11.2017 21:50, Josef Bacik wrote:
> From: Josef Bacik <jbacik@xxxxxx>
>
> We discovered a box that had double allocations, and suspected the space
> cache may be to blame. While auditing the write out path I noticed that
> if we've already setup the space cache we will just carry on. This
> means that any error we hit after cache_save_setup before we go to
> actually write the cache out we won't reset the inode generation, so
> whatever was already written will be considered correct, except it'll be
> stale. Fix this by _always_ resetting the generation on the block group
> inode, this way we only ever have valid or invalid cache.
>
> With this patch I was no longer able to reproduce cache corruption with
> dm-log-writes and my bpf error injection tool.
>
Is it normal that no function checks the return value of cache_save_setup ?
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Josef Bacik <jbacik@xxxxxx>
> ---
> fs/btrfs/extent-tree.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index e2d7e86b51d1..67b26b209e23 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3526,13 +3526,6 @@ static int cache_save_setup(struct btrfs_block_group_cache *block_group,
> goto again;
> }
>
> - /* We've already setup this transaction, go ahead and exit */
> - if (block_group->cache_generation == trans->transid &&
> - i_size_read(inode)) {
> - dcs = BTRFS_DC_SETUP;
> - goto out_put;
> - }
> -
> /*
> * We want to set the generation to 0, that way if anything goes wrong
> * from here on out we know not to trust this cache when we load up next
> @@ -3556,6 +3549,13 @@ static int cache_save_setup(struct btrfs_block_group_cache *block_group,
> }
> WARN_ON(ret);
>
> + /* We've already setup this transaction, go ahead and exit */
> + if (block_group->cache_generation == trans->transid &&
> + i_size_read(inode)) {
> + dcs = BTRFS_DC_SETUP;
> + goto out_put;
> + }
> +
> if (i_size_read(inode) > 0) {
> ret = btrfs_check_trunc_cache_free_space(fs_info,
> &fs_info->global_block_rsv);
>