[PATCH v2 0/2] btrfs: scrub: update last_physical more frequently

[PATCH v2 0/2] btrfs: scrub: update last_physical more frequently

[Qu Wenruo]

[CHANGELOG]
v2:
- Rebased to the latest for-next branch
  No conflict at all

- Rewording the second patch
  There are two problems, one is serious (no last_physical update at
  all for almost full data chunks), the other one is just inconvenient
  (slow update on "btrfs scrub status").

- Add the missing spinlock
  It's mentioned in the commit messages but not in the code

There is a report in the mailling list that scrub only updates its
@last_physical at the end of a chunk.
In fact, it can be worse if there is a used stripe (aka, some extents
exist in the stripe) at the chunk boundary.
As it would skip the @last_physical for that chunk at all.
And for large fses, there are ensured to be several almost full data 

With @last_physical not update for a long time, if we cancel the scrub
halfway and resume, the resumed one scrub would only start at
@last_physical, meaning a lot of scrubbed extents would be re-scrubbed,
wasting quite some IO and CPU.

This patchset would fix it by updateing @last_physical for each finished
stripe (including both P/Q stripe of RAID56, and all data stripes for
all profiles), so that even if the scrub is cancelled, we at most
re-scrub one stripe.

Qu Wenruo (2):
  btrfs: extract the stripe length calculation into a helper
  btrfs: scrub: update last_physical after scrubing one stripe

 fs/btrfs/scrub.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

-- 
2.45.2

[PATCH v2 1/2] btrfs: extract the stripe length calculation into a helper

[Qu Wenruo]

Currently there are two location which needs to calculate the real
length of a stripe (which can be at the end of a chunk, and the chunk
size may not always be 64K aligned).

Extract them into a helper as we're going to have a third user soon.

Reviewed-by: Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/scrub.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 14a8d7100018..d42a58386415 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -1648,14 +1648,21 @@ static void scrub_reset_stripe(struct scrub_stripe *stripe)
 	}
 }
 
+static u32 stripe_length(struct scrub_stripe *stripe)
+{
+	ASSERT(stripe->bg);
+
+	return min(BTRFS_STRIPE_LEN,
+		   stripe->bg->start + stripe->bg->length - stripe->logical);
+
+}
+
 static void scrub_submit_extent_sector_read(struct scrub_ctx *sctx,
 					    struct scrub_stripe *stripe)
 {
 	struct btrfs_fs_info *fs_info = stripe->bg->fs_info;
 	struct btrfs_bio *bbio = NULL;
-	unsigned int nr_sectors = min(BTRFS_STRIPE_LEN, stripe->bg->start +
-				      stripe->bg->length - stripe->logical) >>
-				  fs_info->sectorsize_bits;
+	unsigned int nr_sectors = stripe_length(stripe) >> fs_info->sectorsize_bits;
 	u64 stripe_len = BTRFS_STRIPE_LEN;
 	int mirror = stripe->mirror_num;
 	int i;
@@ -1729,9 +1736,7 @@ static void scrub_submit_initial_read(struct scrub_ctx *sctx,
 {
 	struct btrfs_fs_info *fs_info = sctx->fs_info;
 	struct btrfs_bio *bbio;
-	unsigned int nr_sectors = min(BTRFS_STRIPE_LEN, stripe->bg->start +
-				      stripe->bg->length - stripe->logical) >>
-				  fs_info->sectorsize_bits;
+	unsigned int nr_sectors = stripe_length(stripe) >> fs_info->sectorsize_bits;
 	int mirror = stripe->mirror_num;
 
 	ASSERT(stripe->bg);
-- 
2.45.2

[PATCH v2 2/2] btrfs: scrub: update last_physical after scrubing one stripe

[Qu Wenruo]

Currently sctx->stat.last_physical only got updated in the following
cases:

- When the last stripe of a non-RAID56 chunk is scrubbed
  This implies a pitfall, if the last stripe is at the chunk boundary,
  and we finished the scrub of the whole chunk, we won't update
  last_physical at all until the next chunk.

- When a P/Q stripe of a RAID56 chunk is scrubbed

This leads the following two problems:

- sctx->stat.last_physical is not updated for a almost full chunk
  This is especially bad, affecting scrub resume, as the resume would
  start from last_physical, causing unnecessary re-scrub.

- "btrfs scrub stat" will not report any progress for a long time

Fix the problem by properly updating @last_physical after each stripe is
scrubbed.

And since we're here, for the sake of consistency, use spin lock to
protect the update of @last_physical, just like all the remaining
call sites touching sctx->stat.

Reported-by: Michel Palleau <michel.palleau@gmail.com>
Link: https://lore.kernel.org/linux-btrfs/CAMFk-+igFTv2E8svg=cQ6o3e6CrR5QwgQ3Ok9EyRaEvvthpqCQ@mail.gmail.com/
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/scrub.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index d42a58386415..0f15f5139896 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -1876,6 +1876,10 @@ static int flush_scrub_stripes(struct scrub_ctx *sctx)
 		stripe = &sctx->stripes[i];
 
 		wait_scrub_stripe_io(stripe);
+		spin_lock(&sctx->stat_lock);
+		sctx->stat.last_physical = stripe->physical +
+					   stripe_length(stripe);
+		spin_unlock(&sctx->stat_lock);
 		scrub_reset_stripe(stripe);
 	}
 out:
@@ -2144,7 +2148,9 @@ static int scrub_simple_mirror(struct scrub_ctx *sctx,
 					 cur_physical, &found_logical);
 		if (ret > 0) {
 			/* No more extent, just update the accounting */
+			spin_lock(&sctx->stat_lock);
 			sctx->stat.last_physical = physical + logical_length;
+			spin_unlock(&sctx->stat_lock);
 			ret = 0;
 			break;
 		}
@@ -2341,6 +2347,10 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 			stripe_logical += chunk_logical;
 			ret = scrub_raid56_parity_stripe(sctx, scrub_dev, bg,
 							 map, stripe_logical);
+			spin_lock(&sctx->stat_lock);
+			sctx->stat.last_physical = min(physical + BTRFS_STRIPE_LEN,
+						       physical_end);
+			spin_unlock(&sctx->stat_lock);
 			if (ret)
 				goto out;
 			goto next;
-- 
2.45.2

Re: [PATCH v2 2/2] btrfs: scrub: update last_physical after scrubing one stripe

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 0/2] btrfs: scrub: update last_physical more frequently

[David Sterba]

On Mon, Jul 22, 2024 at 03:25:47PM +0930, Qu Wenruo wrote:
> [CHANGELOG]
> v2:
> - Rebased to the latest for-next branch
>   No conflict at all
> 
> - Rewording the second patch
>   There are two problems, one is serious (no last_physical update at
>   all for almost full data chunks), the other one is just inconvenient
>   (slow update on "btrfs scrub status").
> 
> - Add the missing spinlock
>   It's mentioned in the commit messages but not in the code
> 
> There is a report in the mailling list that scrub only updates its
> @last_physical at the end of a chunk.
> In fact, it can be worse if there is a used stripe (aka, some extents
> exist in the stripe) at the chunk boundary.
> As it would skip the @last_physical for that chunk at all.
> And for large fses, there are ensured to be several almost full data 
> 
> With @last_physical not update for a long time, if we cancel the scrub
> halfway and resume, the resumed one scrub would only start at
> @last_physical, meaning a lot of scrubbed extents would be re-scrubbed,
> wasting quite some IO and CPU.
> 
> This patchset would fix it by updateing @last_physical for each finished
> stripe (including both P/Q stripe of RAID56, and all data stripes for
> all profiles), so that even if the scrub is cancelled, we at most
> re-scrub one stripe.
> 
> Qu Wenruo (2):
>   btrfs: extract the stripe length calculation into a helper
>   btrfs: scrub: update last_physical after scrubing one stripe

Reviewed-by: David Sterba <dsterba@suse.com>