Re: [PATCH 2/9] btrfs: improve preemptive background space flushing

[Nikolay Borisov <nborisov@suse.com>]

On 30.09.20 г. 23:01 ч., Josef Bacik wrote:
<snip>

> When I introduced the ticketed ENOSPC stuff this broke slightly in the
> fact that we were using tickets to indicate if we were done flushing.
> No tickets, no more flushing.  However this meant that we essentially
> never preemptively flushed.  This caused a write performance regression
> that Nikolay noticed in an unrelated patch that removed the committing
> of the transaction during btrfs_end_transaction.

I see, so basically the patch which I biseceted this to was really
papering over the initial bug since the logic in end_transaction, sort
of, simulated pre-emptive flushing... how subtle!

<snip>

> +	spin_lock(&space_info->lock);
> +	used = btrfs_space_info_used(space_info, true);
> +	while (need_do_async_reclaim(fs_info, space_info, used)) {
> +		enum btrfs_reserve_flush_enum flush;
> +		u64 delalloc_size = 0;
> +		u64 to_reclaim, block_rsv_size;
> +		u64 global_rsv_size = global_rsv->reserved;
> +
> +		/*
> +		 * If we're just full of pinned, commit the transaction.  We
> +		 * don't call flush_space(COMMIT_TRANS) here because that has
> +		 * logic to decide whether we need to commit the transaction to
> +		 * satisfy the ticket to keep us from live locking the box by
> +		 * committing over and over again.  Here we don't care about
> +		 * that, we know we are using a lot of space and most of it is
> +		 * pinned, just commit.

nit: That comment is a mouthful, I think what you are describing here is
really this line in may_commit_transaction:

if (!bytes_needed) return 0;

Which triggers if we don't have a ticket, if so there simply say :

"We can't call flush_commit because it will flush iff there is a pending
ticket".

<snip>

> +		/*
> +		 * We don't have a precise counter for delalloc, so we'll
> +		 * approximate it by subtracting out the block rsv's space from
> +		 * the bytes_may_use.  If that amount is higher than the
> +		 * individual reserves, then we can assume it's tied up in
> +		 * delalloc reservations.
> +		 */
> +		block_rsv_size = global_rsv_size +
> +			delayed_block_rsv->reserved +
> +			delayed_refs_rsv->reserved +
> +			trans_rsv->reserved;
> +		if (block_rsv_size < space_info->bytes_may_use)
> +			delalloc_size = space_info->bytes_may_use -
> +				block_rsv_size;

What about  :

percpu_counter_add_batch(&fs_info->delalloc_bytes, len,
                      fs_info->delalloc_batch);

> +		spin_unlock(&space_info->lock);
> +

<snip>