On Fri, Aug 23, 2019 at 11:17:06AM +0300, Nikolay Borisov wrote:
> > @@ -759,7 +786,7 @@ static void btrfs_async_reclaim_metadata_space(struct
> > work_struct *work)
> > if (flush_state > COMMIT_TRANS) {
> > commit_cycles++;
> > if (commit_cycles > 2) {
> > - if (wake_all_tickets(&space_info->tickets)) {
> > + if (maybe_fail_all_tickets(fs_info,
> > space_info)) {
>
> This looks odd. A function called "maybe_fail" which if it returns true
> then we are sure we haven't failed all tickets, instead make another go
> through the flushing machinery. I think the problem stems from the fact
> it's doing 3 things, namely:
>
> 1. Failing all tickets, that aren't smaller than the initial one
> 2. Trying to satisfy other tickets apart from the one failed
> 3. If it succeeded it signals to the flushing machinery to make another go
>
> The function's name really reflects what's going on in 1. But 2 and 3
> are also major part of the logic. I think there is 'impedance mismatch'
> here. I'm at a loss what to do here, honestly.
The function is quite short and splitting it may not be an improvement,
so the semantics should be at least documented, the 3 points you write
look comprehensible so I'd stick that to the function. As this is not
functional change documentation is probably best we can do now to move
forward.
