[PATCH v2 0/4] btrfs: Make balance cancelling response faster

[PATCH v2 0/4] btrfs: Make balance cancelling response faster

[Qu Wenruo]

[PROBLEM]
There are quite some users reporting that 'btrfs balance cancel' slow to
cancel current running balance, or even doesn't work for certain dead
balance loop.

With the following script showing how long it takes to fully stop a
balance:
  #!/bin/bash
  dev=/dev/test/test
  mnt=/mnt/btrfs

  umount $mnt &> /dev/null
  umount $dev &> /dev/null

  mkfs.btrfs -f $dev
  mount $dev -o nospace_cache $mnt

  dd if=/dev/zero bs=1M of=$mnt/large &
  dd_pid=$!

  sleep 3
  kill -KILL $dd_pid
  sync

  btrfs balance start --bg --full $mnt &
  sleep 1

  echo "cancel request" >> /dev/kmsg
  time btrfs balance cancel $mnt
  umount $mnt

It takes around 7~10s to cancel the running balance in my test
environment.

[CAUSE]
Btrfs uses btrfs_fs_info::balance_cancel_req to record how many cancel
request are queued.
However that cancelling request is only checked after relocating a block
group.

That behavior is far from optimal to provide a faster cancelling.

[FIX]
This patchset will add more cancelling check points, to make cancelling
faster.

And also, introduce a new error injection points to cover these newly
introduced and future check points.


For the canceled balance during relocate_block_group(), we are re-using
the existing error handling path.
It will mark all existing reloc_roots as orphan in prepare_to_merge(),
then queue all of them for cleanup in merge_reloc_roots().
Thus it shouldn't cause any problem.

Changelog:
v2:
- Rebased to v5.6-rc1
  There is a small conflicts caused by extra finished stage output.
  Other than that, everything is pretty straightforward

- Add explanation for the error handling path in cover letter.

Qu Wenruo (4):
  btrfs: relocation: Introduce error injection points for cancelling
    balance
  btrfs: relocation: Check cancel request after each data page read
  btrfs: relocation: Check cancel request after each extent found
  btrfs: relocation: Work around dead relocation stage loop

 fs/btrfs/ctree.h      |  1 +
 fs/btrfs/relocation.c | 24 ++++++++++++++++++++++++
 fs/btrfs/volumes.c    |  2 +-
 3 files changed, 26 insertions(+), 1 deletion(-)

-- 
2.25.0

[PATCH v2 1/4] btrfs: relocation: Introduce error injection points for cancelling balance

[Qu Wenruo]

Introduce a new error injection point, should_cancel_balance().

It's just a wrapper of atomic_read(&fs_info->balance_cancel_req), but
allows us to override the return value.

Currently there are only one locations using this function:
- btrfs_balance()
  It checks cancel before each block group.

There are other locations checking fs_info->balance_cancel_req, but they
are not used as an indicator to exit, so there is no need to use the
wrapper.

But there will be more locations coming, and some locations can cause
kernel panic if not handled properly.

So introduce this error injection to provide better test interface.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.h      |  1 +
 fs/btrfs/relocation.c | 11 +++++++++++
 fs/btrfs/volumes.c    |  2 +-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 36df977b64d9..d8e12ca15bb6 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3401,6 +3401,7 @@ void btrfs_reloc_pre_snapshot(struct btrfs_pending_snapshot *pending,
 			      u64 *bytes_to_reserve);
 int btrfs_reloc_post_snapshot(struct btrfs_trans_handle *trans,
 			      struct btrfs_pending_snapshot *pending);
+int should_cancel_balance(struct btrfs_fs_info *fs_info);
 
 /* scrub.c */
 int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start,
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 995d4b8b1cfd..475479d50cc3 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -9,6 +9,7 @@
 #include <linux/blkdev.h>
 #include <linux/rbtree.h>
 #include <linux/slab.h>
+#include <linux/error-injection.h>
 #include "ctree.h"
 #include "disk-io.h"
 #include "transaction.h"
@@ -3264,6 +3265,16 @@ int setup_extent_mapping(struct inode *inode, u64 start, u64 end,
 	return ret;
 }
 
+/*
+ * This wrapper is to allow error injection to fully test all cancel
+ * call sites.
+ */
+int should_cancel_balance(struct btrfs_fs_info *fs_info)
+{
+	return atomic_read(&fs_info->balance_cancel_req);
+}
+ALLOW_ERROR_INJECTION(should_cancel_balance, TRUE);
+
 static int relocate_file_extent_cluster(struct inode *inode,
 					struct file_extent_cluster *cluster)
 {
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9cfc668f91f4..cd61f4e29d9e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3904,7 +3904,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 
 	if (btrfs_fs_closing(fs_info) ||
 	    atomic_read(&fs_info->balance_pause_req) ||
-	    atomic_read(&fs_info->balance_cancel_req)) {
+	    should_cancel_balance(fs_info)) {
 		ret = -EINVAL;
 		goto out;
 	}
-- 
2.25.0

Re: [PATCH v2 1/4] btrfs: relocation: Introduce error injection points for cancelling balance

[Josef Bacik]

On 2/11/20 12:37 AM, Qu Wenruo wrote:
> Introduce a new error injection point, should_cancel_balance().
> 
> It's just a wrapper of atomic_read(&fs_info->balance_cancel_req), but
> allows us to override the return value.
> 
> Currently there are only one locations using this function:
> - btrfs_balance()
>    It checks cancel before each block group.
> 
> There are other locations checking fs_info->balance_cancel_req, but they
> are not used as an indicator to exit, so there is no need to use the
> wrapper.
> 
> But there will be more locations coming, and some locations can cause
> kernel panic if not handled properly.
> 
> So introduce this error injection to provide better test interface.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

[PATCH v2 2/4] btrfs: relocation: Check cancel request after each data page read

[Qu Wenruo]

When relocating a data extents with large large data extents, we spend
most of our time in relocate_file_extent_cluster() at stage "moving data
extents":
 1)               |  btrfs_relocate_block_group [btrfs]() {
 1)               |    relocate_file_extent_cluster [btrfs]() {
 1) $ 6586769 us  |    }
 1) + 18.260 us   |    relocate_file_extent_cluster [btrfs]();
 1) + 15.770 us   |    relocate_file_extent_cluster [btrfs]();
 1) $ 8916340 us  |  }
 1)               |  btrfs_relocate_block_group [btrfs]() {
 1)               |    relocate_file_extent_cluster [btrfs]() {
 1) $ 11611586 us |    }
 1) + 16.930 us   |    relocate_file_extent_cluster [btrfs]();
 1) + 15.870 us   |    relocate_file_extent_cluster [btrfs]();
 1) $ 14986130 us |  }

So to make data relocation cancelling quicker, here add extra balance
cancelling check after each page read in relocate_file_extent_cluster().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/relocation.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 475479d50cc3..23b279872641 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3396,6 +3396,10 @@ static int relocate_file_extent_cluster(struct inode *inode,
 		btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE);
 		balance_dirty_pages_ratelimited(inode->i_mapping);
 		btrfs_throttle(fs_info);
+		if (should_cancel_balance(fs_info)) {
+			ret = -ECANCELED;
+			goto out;
+		}
 	}
 	WARN_ON(nr != cluster->nr);
 out:
-- 
2.25.0

Re: [PATCH v2 2/4] btrfs: relocation: Check cancel request after each data page read

[Josef Bacik]

On 2/11/20 12:37 AM, Qu Wenruo wrote:
> When relocating a data extents with large large data extents, we spend
> most of our time in relocate_file_extent_cluster() at stage "moving data
> extents":
>   1)               |  btrfs_relocate_block_group [btrfs]() {
>   1)               |    relocate_file_extent_cluster [btrfs]() {
>   1) $ 6586769 us  |    }
>   1) + 18.260 us   |    relocate_file_extent_cluster [btrfs]();
>   1) + 15.770 us   |    relocate_file_extent_cluster [btrfs]();
>   1) $ 8916340 us  |  }
>   1)               |  btrfs_relocate_block_group [btrfs]() {
>   1)               |    relocate_file_extent_cluster [btrfs]() {
>   1) $ 11611586 us |    }
>   1) + 16.930 us   |    relocate_file_extent_cluster [btrfs]();
>   1) + 15.870 us   |    relocate_file_extent_cluster [btrfs]();
>   1) $ 14986130 us |  }
> 
> So to make data relocation cancelling quicker, here add extra balance
> cancelling check after each page read in relocate_file_extent_cluster().
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

If you respin, can you note that with this cancellation we'll break out and 
merge the reloc roots, its not like everything will just be left over to be 
completed at the next mount.

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH v2 2/4] btrfs: relocation: Check cancel request after each data page read

[David Sterba]

On Thu, Feb 13, 2020 at 03:03:55PM -0500, Josef Bacik wrote:
> On 2/11/20 12:37 AM, Qu Wenruo wrote:
> > When relocating a data extents with large large data extents, we spend
> > most of our time in relocate_file_extent_cluster() at stage "moving data
> > extents":
> >   1)               |  btrfs_relocate_block_group [btrfs]() {
> >   1)               |    relocate_file_extent_cluster [btrfs]() {
> >   1) $ 6586769 us  |    }
> >   1) + 18.260 us   |    relocate_file_extent_cluster [btrfs]();
> >   1) + 15.770 us   |    relocate_file_extent_cluster [btrfs]();
> >   1) $ 8916340 us  |  }
> >   1)               |  btrfs_relocate_block_group [btrfs]() {
> >   1)               |    relocate_file_extent_cluster [btrfs]() {
> >   1) $ 11611586 us |    }
> >   1) + 16.930 us   |    relocate_file_extent_cluster [btrfs]();
> >   1) + 15.870 us   |    relocate_file_extent_cluster [btrfs]();
> >   1) $ 14986130 us |  }
> > 
> > So to make data relocation cancelling quicker, here add extra balance
> > cancelling check after each page read in relocate_file_extent_cluster().
> > 
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> If you respin, can you note that with this cancellation we'll break out and 
> merge the reloc roots, its not like everything will just be left over to be 
> completed at the next mount.

Yes that's what I miss here too and asked about it in v1 thread. No
resend is needed, I can update changelog if the new text is sent to me.

[PATCH v2 3/4] btrfs: relocation: Check cancel request after each extent found

[Qu Wenruo]

When relocating data block groups with tons of small extents, or
large metadata block groups, there can be over 200,000 extents.

We will iterate all extents of such block group in relocate_block_group(),
where iteration itself can be kinda time-consuming.

So when user want to cancel the balance, the extent iteration loop can
be another target.

This patch will add the cancelling check in the extent iteration loop of
relocate_block_group() to make balance cancelling faster.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/relocation.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 23b279872641..3379850d7695 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4181,6 +4181,10 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 				break;
 			}
 		}
+		if (should_cancel_balance(fs_info)) {
+			err = -ECANCELED;
+			break;
+		}
 	}
 	if (trans && progress && err == -ENOSPC) {
 		ret = btrfs_force_chunk_alloc(trans, rc->block_group->flags);
-- 
2.25.0

Re: [PATCH v2 3/4] btrfs: relocation: Check cancel request after each extent found

[Josef Bacik]

On 2/11/20 12:37 AM, Qu Wenruo wrote:
> When relocating data block groups with tons of small extents, or
> large metadata block groups, there can be over 200,000 extents.
> 
> We will iterate all extents of such block group in relocate_block_group(),
> where iteration itself can be kinda time-consuming.
> 
> So when user want to cancel the balance, the extent iteration loop can
> be another target.
> 
> This patch will add the cancelling check in the extent iteration loop of
> relocate_block_group() to make balance cancelling faster.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

[PATCH v2 4/4] btrfs: relocation: Work around dead relocation stage loop

[Qu Wenruo]

There are some reports of dead relocation stage loop, where dmesg is
flooded by "Found X extents".

The root cause of it is still uncertain, but we can work around such bug
by checking cancelling request so user can at least cancel such dead
loop.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/relocation.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 3379850d7695..b31d582a2ca1 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4470,6 +4470,11 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
 
 		btrfs_info(fs_info, "found %llu extents, stage: %s",
 			   rc->extents_found, stage_to_string(finishes_stage));
+
+		if (should_cancel_balance(fs_info)) {
+			err = -ECANCELED;
+			goto out;
+		}
 	}
 
 	WARN_ON(rc->block_group->pinned > 0);
-- 
2.25.0

Re: [PATCH v2 4/4] btrfs: relocation: Work around dead relocation stage loop

[Josef Bacik]

On 2/11/20 12:37 AM, Qu Wenruo wrote:
> There are some reports of dead relocation stage loop, where dmesg is
> flooded by "Found X extents".
> 
> The root cause of it is still uncertain, but we can work around such bug
> by checking cancelling request so user can at least cancel such dead
> loop.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---

Why?  It'll get picked up by one of the other cancel checks right?  I'd rather 
know why things are going wrong than put something in for a just in case 
scenario, especially since the other cancel points actually make sense and will 
accomplish the same goal.  Thanks,

Josef

Re: [PATCH v2 4/4] btrfs: relocation: Work around dead relocation stage loop

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 857 bytes --]



On 2020/2/14 上午4:08, Josef Bacik wrote:
> On 2/11/20 12:37 AM, Qu Wenruo wrote:
>> There are some reports of dead relocation stage loop, where dmesg is
>> flooded by "Found X extents".
>>
>> The root cause of it is still uncertain, but we can work around such bug
>> by checking cancelling request so user can at least cancel such dead
>> loop.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
> 
> Why?  It'll get picked up by one of the other cancel checks right?  I'd
> rather know why things are going wrong than put something in for a just
> in case scenario, especially since the other cancel points actually make
> sense and will accomplish the same goal.  Thanks,

Yes, you're right.

Please discard this one, as there aren't that many reports allowing us
to investigate it further.

Thanks,
Qu
> 
> Josef


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/4] btrfs: Make balance cancelling response faster

[David Sterba]

On Tue, Feb 11, 2020 at 01:37:25PM +0800, Qu Wenruo wrote:
> For the canceled balance during relocate_block_group(), we are re-using
> the existing error handling path.
> It will mark all existing reloc_roots as orphan in prepare_to_merge(),
> then queue all of them for cleanup in merge_reloc_roots().
> Thus it shouldn't cause any problem.
> 
> Changelog:
> v2:
> - Rebased to v5.6-rc1
>   There is a small conflicts caused by extra finished stage output.
>   Other than that, everything is pretty straightforward
> 
> - Add explanation for the error handling path in cover letter.

Ah so the text is here, but we want it in the changelog and perhaps in
the code too as it's breaking out of a loop that does a lot of things.

As further changes are not in code I'll add the patches (without 4th) to
for-next for more testing. Thanks.