On 28 Nov 2018, at 14:06, David Sterba 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.
Yeah, I love the idea, I'm just worried about a tiny bit of rate limiting. Since we're only waking up a fixed process and not creating new work queue items every time, it's probably fine, but a delay of HZ/2 would probably be even better. -chris
