Re: [PATCH 6/9] btrfs: rework wake_all_tickets

[David Sterba <dsterba@suse.cz>]

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.