Re: [PATCH 1/5] btrfs: Improve global reserve stealing logic

Nikolay Borisov
2020-03-10T14:27:43Z

> For unlink transactions and block group removal
> btrfs_start_transaction_fallback_global_rsv will first try to start
> an ordinary transaction and if it fails it will fall back to reserving
> the required amount by stealing from the global reserve. This is sound
> in theory but current code doesn't perform any locking or throttling so
> if there are multiple concurrent unlink() callers they can deplete
> the global reservation which will result in ENOSPC.
>
> Fix this behavior by introducing BTRFS_RESERVE_FLUSH_ALL_STEAL. It's
> used to mark unlink reservation. The flushing machinery is modified to
> steal from global reservation when it sees such reservation being on the
> brink of failure (in maybe_fail_all_tickets).
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> fs/btrfs/block-group.c | 2 +-
> fs/btrfs/ctree.h | 1 +
> fs/btrfs/inode.c | 2 +-
> fs/btrfs/space-info.c | 38 +++++++++++++++++++++++++++++++++++++-
> fs/btrfs/space-info.h | 1 +
> fs/btrfs/transaction.c | 42 +++++-------------------------------------
> fs/btrfs/transaction.h | 3 +--
> 7 files changed, 47 insertions(+), 42 deletions(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 60e9bb136f34..faa04093b6b5 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1171,7 +1171,7 @@ struct btrfs_trans_handle *btrfs_start_trans_remove_block_group(
> free_extent_map(em);
>
> return btrfs_start_transaction_fallback_global_rsv(fs_info->extent_root,
> - num_items, 1);
> + num_items);
> }
>
> /*
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 2ccb2a090782..782c63f213e9 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2528,6 +2528,7 @@ enum btrfs_reserve_flush_enum {
> BTRFS_RESERVE_FLUSH_DATA,
> BTRFS_RESERVE_FLUSH_FREE_SPACE_INODE,
> BTRFS_RESERVE_FLUSH_ALL,
> + BTRFS_RESERVE_FLUSH_ALL_STEAL,
> };
>
> enum btrfs_flush_state {
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index b8dabffac767..4e3b115ef1d7 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3617,7 +3617,7 @@ static struct btrfs_trans_handle *__unlink_start_trans(struct inode *dir)
> * 1 for the inode ref
> * 1 for the inode
> */
> - return btrfs_start_transaction_fallback_global_rsv(root, 5, 5);
> + return btrfs_start_transaction_fallback_global_rsv(root, 5);
> }
>
> static int btrfs_unlink(struct inode *dir, struct dentry *dentry)
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index 26e1c492b9b5..9c9a4933f72b 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -810,6 +810,35 @@ static inline int need_do_async_reclaim(struct btrfs_fs_info *fs_info,
> !test_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state));
> }
>
> +static bool steal_from_global_rsv(struct btrfs_fs_info *fs_info,
> + struct btrfs_space_info *space_info,
> + struct reserve_ticket *ticket)
> +{
> + struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
> + u64 min_bytes;
> +
> + if (global_rsv->space_info != space_info)
> + return false;
> +
> + spin_lock(&global_rsv->lock);
> + min_bytes = div_factor(global_rsv->size, 1);
> + if (global_rsv->reserved < min_bytes + ticket->bytes) {
> + spin_unlock(&global_rsv->lock);
> + return false;
> + }
> + global_rsv->reserved -= ticket->bytes;
> + ticket->bytes = 0;
> + trace_printk("Satisfied ticket from global rsv\n");

nit: that's a left-over from my debugging of this patch, it must be
removed before being merged :)