Re: [PATCH 4/5] btrfs: only check priority tickets for priority flushing

[Nikolay Borisov <nborisov@suse.com>]

On 9.03.20 г. 22:23 ч., Josef Bacik wrote:
> In debugging a generic/320 failure on ppc64, Nikolay noticed that
> sometimes we'd ENOSPC out with plenty of space to reclaim if we had
> committed the transaction.  He further discovered that this was because
> there was a priority ticket that was small enough to fit in the free
> space currently in the space_info.
> 
> This is problematic because we prioritize priority tickets, refilling
> them first as new space becomes available.  However this leaves a corner
> where we could fail to satisfy a priority ticket when we would have
> otherwise succeeded.

This warrants an example.

> 
> Consider the case where there's no flushing left to happen other than
> commit the transaction, and there are tickets on the normal flushing
> list. 

^ does this refer to the ordinary flush
btrfs_async_reclaim_metadata_space or priority_reclaim_metadata_space
with evict_flush_states (which also contains a COMMIT_TRANS state).

 The priority flusher comes in, and assume there's enough space
> left in the space_info to satisfy this request.  

This happens _after_ we've been added to the prio list, so perhahps this
and the next sentence need to be transposed, reworded to explicitly
state that despite us having space to satisfy an incoming prio request
if it see pending prio requests it will simply add this to the list.

We will still be added
> to the priority list and go through the flushing motions, and eventually
> fail returning an ENOSPC.
> 
> Instead we should only add ourselves to the list if there's something on
> the priority_list already.  This way we avoid the incorrect ENOSPC
> scenario.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/space-info.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index d198cfd45cf7..77ea204f0b6a 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -1276,6 +1276,17 @@ static int handle_reserve_ticket(struct btrfs_fs_info *fs_info,
>  	return ret;
>  }
>  
> +/*
> + * This returns true if this flush state will go through the ordinary flushing
> + * code.
> + */
> +static inline bool is_normal_flushing(enum btrfs_reserve_flush_enum flush)
> +{
> +	return (flush == BTRFS_RESERVE_FLUSH_DATA) ||
> +		(flush == BTRFS_RESERVE_FLUSH_ALL) ||
> +		(flush == BTRFS_RESERVE_FLUSH_ALL_STEAL);
> +}
> +
>  /**
>   * reserve_metadata_bytes - try to reserve bytes from the block_rsv's space
>   * @root - the root we're allocating for
> @@ -1311,8 +1322,17 @@ static int __reserve_bytes(struct btrfs_fs_info *fs_info,
>  	spin_lock(&space_info->lock);
>  	ret = -ENOSPC;
>  	used = btrfs_space_info_used(space_info, true);
> -	pending_tickets = !list_empty(&space_info->tickets) ||
> -		!list_empty(&space_info->priority_tickets);
> +
> +	/*
> +	 * We don't want NO_FLUSH allocations to jump everybody, they can
> +	 * generally handle ENOSPC in a different way, so treat them the same as
> +	 * normal flushers when it comes to skipping pending tickets.
> +	 */
> +	if (is_normal_flushing(flush) || (flush == BTRFS_RESERVE_NO_FLUSH))
> +		pending_tickets = !list_empty(&space_info->tickets) ||
> +			!list_empty(&space_info->priority_tickets);
> +	else
> +		pending_tickets = !list_empty(&space_info->priority_tickets);
>  
>  	/*
>  	 * Carry on if we have enough space (short-circuit) OR call
> @@ -1338,9 +1358,7 @@ static int __reserve_bytes(struct btrfs_fs_info *fs_info,
>  		ticket.error = 0;
>  		init_waitqueue_head(&ticket.wait);
>  		ticket.steal = (flush == BTRFS_RESERVE_FLUSH_ALL_STEAL);
> -		if (flush == BTRFS_RESERVE_FLUSH_ALL ||
> -		    flush == BTRFS_RESERVE_FLUSH_DATA ||
> -		    flush == BTRFS_RESERVE_FLUSH_ALL_STEAL) {
> +		if (is_normal_flushing(flush)) {
>  			list_add_tail(&ticket.list, &space_info->tickets);
>  			if (!space_info->flush) {
>  				space_info->flush = 1;
>