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

From: Josef Bacik <josef@toxicpanda.com>
To: Nikolay Borisov <nborisov@suse.com>,
	linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH 2/9] btrfs: improve preemptive background space flushing
Date: Thu, 1 Oct 2020 17:35:58 -0400	[thread overview]
Message-ID: <01e8925f-5b4b-74aa-f3e6-0faae52a4d3b@toxicpanda.com> (raw)
In-Reply-To: <efe49176-1eba-df6a-ffdf-47031c5acf36@suse.com>

On 10/1/20 9:19 AM, Nikolay Borisov wrote:
> 
> 
> 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".
> 

Yup I'll reword.

> <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);
> 

Because that's the total data bytes, not how much metadata is reserved for the 
data bytes, which can be very grossly different things.  For example, one 
contiguous MAX_EXTENT_SIZE data extent is only like 1mib of metadata 
reservations, but if we used delalloc_bytes here we'd think the delalloc size 
was dominating the metadata reservations.  Thanks,

Josef