On Thu, Nov 11, 2021 at 04:50:48PM +0200, Nikolay Borisov wrote:

On 11.11.21 г. 16:13, Josef Bacik wrote:

quoted

On Thu, Nov 11, 2021 at 03:14:20PM +0200, Nikolay Borisov wrote:

quoted

On 9.11.21 г. 17:12, Josef Bacik wrote:

quoted

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 <redacted>

quoted

--->  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,

IMO such scenario description should be put in the changelog.

Agreed. I'll paste and edit it from this answer, no need to resend the
whole patch but I'd appreciate proofreading once it's in misc-next.