Re: [PATCH v3 2/7] btrfs: check for priority ticket granting before flushing

[Josef Bacik <josef@toxicpanda.com>]

On Thu, Nov 11, 2021 at 03:14:20PM +0200, Nikolay Borisov wrote:
> 
> 
> On 9.11.21 г. 17:12, Josef Bacik wrote:
> > Since we're dropping locks before we enter the priority flushing loops
> > we could have had our ticket granted before we got the space_info->lock.
> > So add this check to avoid doing some extra flushing in the priority
> > flushing cases.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> > --->  fs/btrfs/space-info.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> > index 9d6048f54097..9a362f3a6df4 100644
> > --- a/fs/btrfs/space-info.c
> > +++ b/fs/btrfs/space-info.c
> > @@ -1264,7 +1264,7 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
> >  
> >  	spin_lock(&space_info->lock);
> >  	to_reclaim = btrfs_calc_reclaim_metadata_size(fs_info, space_info);
> > -	if (!to_reclaim) {
> > +	if (!to_reclaim || ticket->bytes == 0) {
> 
> nit: This is purely an optimization, handling the case where a prio
> ticket N is being added to the list, but at the same time we might have
> had ticket N-1 just satisfied (or failed) and having called
> try_granting_ticket might have satisfied concurrently added ticket N,
> right? And this is a completely independent change of the other cleanups
> being done here?
> 

It's definitely just an optimization, but it can be less specific than this.
Think we came in to reserve, we didn't have the space, we added our ticket to
the list.  But at the same time somebody was waiting on the space_info lock to
add space and do btrfs_try_granting_ticket(), so we drop the lock, get
satisfied, come in to do our loop, and we have been satisified.

This is the priority reclaim path, so to_reclaim could be !0 still because we
may have only satisified the priority tickets and still left non priority
tickets on the list.  We would then have to_reclaim but ->bytes == 0.

Clearly not a huge deal, I just noticied it when I was redoing the locking for
the cleanups and it annoyed me.  Thanks,

Josef