Re: [PATCH 4/9] btrfs: rework btrfs_space_info_add_old_bytes

[Nikolay Borisov <nborisov@suse.com>]

On 22.08.19 г. 22:10 ч., Josef Bacik wrote:
> If there are pending tickets and we are overcommitted we will simply
> return free'd reservations to space_info->bytes_may_use if we cannot
> overcommit any more.  This is problematic because we assume any free
> space would have been added to the tickets, and so if we go from an
> overcommitted state to not overcommitted we could have plenty of space
> in our space_info but be unable to make progress on our tickets because
> we only refill tickets from previous reservations.
> 
> Consider the following example.  We were allowed to overcommit to
> space_info->total_bytes + 2mib.  Now we've allocated all of our chunks
> and are no longer allowed to overcommit those extra 2mib.  Assume there
> is a 3mib reservation we are now freeing.  Because we can no longer
> overcommit we do not partially refill the ticket with the 3mib, instead
> we subtract it from space_info->bytes_may_use.  Now the total reserved
> space is 1mib less than total_bytes, meaning we have 1mib of space we
> could reserve.  Now assume that our ticket is 2mib, and we only have
> 1mib of space to reclaim, so we have a partial refilling to 1mib.  We
> keep trying to flush and eventually give up and ENOSPC the ticket, when
> there was the remaining 1mib left in the space_info for usage.

The wording of this paragraph makes it a bit hard to understand. How
about something like:

Consider an example where a request is allowed to overcommit
space_info->total_bytes + 2mib. At this point it's no longer possible to
overcommit extra space. At the same time there is an existing 3mib
reservation which is being freed. Due to the existing check failing:

if (check_overcommit &&
  !can_overcommit(fs_info, space_info, 0, flush, false))

btrfs_space_info_add_old_bytes won't partially refill tickets with those
3mib, instead it will subtract them from space_info->bytes_may_use. This
results in the total reserved space being 1mib below
space_info->total_bytes. <You need to expand on where the 2mib ticket
came - was it part of the original reservation that caused the
overcommit or is it a new reservation that comes while we are at 1mb
below space_info->total_bytes>

> 
> Instead of doing this partial filling of tickets dance we need to simply
> add our space to the space_info, and then do the normal check to see if
> we can satisfy the whole reservation.  If we can then we wake up the
> ticket and carry on.  This solves the above problem and makes it much
> more straightforward to understand how the tickets are satisfied.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/space-info.c | 43 ++++++++++++++++---------------------------
>  1 file changed, 16 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index a0a36d5768e1..357fe7548e07 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -233,52 +233,41 @@ void btrfs_space_info_add_old_bytes(struct btrfs_fs_info *fs_info,
>  				    struct btrfs_space_info *space_info,
>  				    u64 num_bytes)
>  {
> -	struct reserve_ticket *ticket;
>  	struct list_head *head;
> -	u64 used;
>  	enum btrfs_reserve_flush_enum flush = BTRFS_RESERVE_NO_FLUSH;
> -	bool check_overcommit = false;
>  
>  	spin_lock(&space_info->lock);
>  	head = &space_info->priority_tickets;
> +	btrfs_space_info_update_bytes_may_use(fs_info, space_info, -num_bytes);
>  
> -	/*
> -	 * If we are over our limit then we need to check and see if we can
> -	 * overcommit, and if we can't then we just need to free up our space
> -	 * and not satisfy any requests.
> -	 */
> -	used = btrfs_space_info_used(space_info, true);
> -	if (used - num_bytes >= space_info->total_bytes)
> -		check_overcommit = true;
>  again:
> -	while (!list_empty(head) && num_bytes) {
> -		ticket = list_first_entry(head, struct reserve_ticket,
> -					  list);
> -		/*
> -		 * We use 0 bytes because this space is already reserved, so
> -		 * adding the ticket space would be a double count.
> -		 */
> -		if (check_overcommit &&
> -		    !can_overcommit(fs_info, space_info, 0, flush, false))
> -			break;
> -		if (num_bytes >= ticket->bytes) {
> +	while (!list_empty(head)) {
> +		struct reserve_ticket *ticket;
> +		u64 used = btrfs_space_info_used(space_info, true);
> +
> +		ticket = list_first_entry(head, struct reserve_ticket, list);
> +
> +		/* Check and see if our ticket can be satisified now. */
> +		if ((used + ticket->bytes <= space_info->total_bytes) ||
> +		    can_overcommit(fs_info, space_info, ticket->bytes, flush,
> +				   false)) {
> +			btrfs_space_info_update_bytes_may_use(fs_info,
> +							      space_info,
> +							      ticket->bytes);
>  			list_del_init(&ticket->list);
> -			num_bytes -= ticket->bytes;
>  			ticket->bytes = 0;
>  			space_info->tickets_id++;
>  			wake_up(&ticket->wait);
>  		} else {
> -			ticket->bytes -= num_bytes;
> -			num_bytes = 0;
> +			break;
>  		}
>  	}
>  
> -	if (num_bytes && head == &space_info->priority_tickets) {
> +	if (head == &space_info->priority_tickets) {
>  		head = &space_info->tickets;
>  		flush = BTRFS_RESERVE_FLUSH_ALL;
>  		goto again;
>  	}
> -	btrfs_space_info_update_bytes_may_use(fs_info, space_info, -num_bytes);
>  	spin_unlock(&space_info->lock);
>  }
>  
>