Re: [PATCH 2/3] btrfs: wakeup cleaner thread when adding delayed iput

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2018/11/29 上午4:08, Filipe Manana wrote:
> On Wed, Nov 28, 2018 at 7:09 PM David Sterba <dsterba@suse.cz> wrote:
>>
>> On Tue, Nov 27, 2018 at 03:08:08PM -0500, Josef Bacik wrote:
>>> On Tue, Nov 27, 2018 at 07:59:42PM +0000, Chris Mason wrote:
>>>> On 27 Nov 2018, at 14:54, Josef Bacik wrote:
>>>>
>>>>> On Tue, Nov 27, 2018 at 10:26:15AM +0200, Nikolay Borisov wrote:
>>>>>>
>>>>>>
>>>>>> On 21.11.18 г. 21:09 ч., Josef Bacik wrote:
>>>>>>> The cleaner thread usually takes care of delayed iputs, with the
>>>>>>> exception of the btrfs_end_transaction_throttle path.  The cleaner
>>>>>>> thread only gets woken up every 30 seconds, so instead wake it up to
>>>>>>> do
>>>>>>> it's work so that we can free up that space as quickly as possible.
>>>>>>
>>>>>> Have you done any measurements how this affects the overall system. I
>>>>>> suspect this introduces a lot of noise since now we are going to be
>>>>>> doing a thread wakeup on every iput, does this give a chance to have
>>>>>> nice, large batches of iputs that  the cost of wake up can be
>>>>>> amortized
>>>>>> across?
>>>>>
>>>>> I ran the whole patchset with our A/B testing stuff and the patchset
>>>>> was a 5%
>>>>> win overall, so I'm inclined to think it's fine.  Thanks,
>>>>
>>>> It's a good point though, a delayed wakeup may be less overhead.
>>>
>>> Sure, but how do we go about that without it sometimes messing up?  In practice
>>> the only time we're doing this is at the end of finish_ordered_io, so likely to
>>> not be a constant stream.  I suppose since we have places where we force it to
>>> run that we don't really need this.  IDK, I'm fine with dropping it.  Thanks,
>>
>> The transaction thread wakes up cleaner periodically (commit interval,
>> 30s by default), so the time to process iputs is not unbounded.
>>
>> I have the same concerns as Nikolay, coupling the wakeup to all delayed
>> iputs could result in smaller batches. But some of the callers of
>> btrfs_add_delayed_iput might benefit from the extra wakeup, like
>> btrfs_remove_block_group, so I don't want to leave the idea yet.
> 
> I'm curious, why do you think it would benefit btrfs_remove_block_group()?

Just as Filipe said, I'm not sure why btrfs_remove_block_group() would
get some benefit from more frequent cleaner thread wake up.

For an empty block group to really be removed, it also needs to have 0
pinned bytenr, which is only possible after a transaction get committed.

Thanks,
Qu