On 2018/11/29 上午4:08, Filipe Manana wrote: > On Wed, Nov 28, 2018 at 7:09 PM David Sterba <dsterba@xxxxxxx> 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
