[PATCH 1/2] btrfs: avoid possible signal interruption for btrfs_drop_snapshot() on relocation tree

[PATCH 1/2] btrfs: avoid possible signal interruption for btrfs_drop_snapshot() on relocation tree

[Qu Wenruo]

[BUG]
There is a bug report about bad signal timing could lead to read-only
fs during balance:

  BTRFS info (device xvdb): balance: start -d -m -s
  BTRFS info (device xvdb): relocating block group 73001861120 flags metadata
  BTRFS info (device xvdb): found 12236 extents, stage: move data extents
  BTRFS info (device xvdb): relocating block group 71928119296 flags data
  BTRFS info (device xvdb): found 3 extents, stage: move data extents
  BTRFS info (device xvdb): found 3 extents, stage: update data pointers
  BTRFS info (device xvdb): relocating block group 60922265600 flags metadata
  BTRFS: error (device xvdb) in btrfs_drop_snapshot:5505: errno=-4 unknown
  BTRFS info (device xvdb): forced readonly
  BTRFS info (device xvdb): balance: ended with status: -4

[CAUSE]
The direct cause is the -EINTR from the following call chain when a
fatal signal is pending:

 relocate_block_group()
 |- clean_dirty_subvols()
    |- btrfs_drop_snapshot()
       |- btrfs_start_transaction()
          |- btrfs_delayed_refs_rsv_refill()
             |- btrfs_reserve_metadata_bytes()
                |- __reserve_metadata_bytes()
                   |- wait_reserve_ticket()
                      |- prepare_to_wait_event();
                      |- ticket->error = -EINTR;

Normally this behavior is fine for most btrfs_start_transaction()
callers, as they need to catch the fatal signal and exit asap.

However to balance, especially for the clean_dirty_subvols() case, we're
already doing cleanup works, such -EINTR from btrfs_drop_snapshot()
could cause a lot of unexpected problems.

From the mentioned forced read-only, to later balance error due to half
dropped reloc trees.

[FIX]
Fix this problem by using btrfs_join_transaction() if
btrfs_drop_snapshot() is called from relocation context.

As btrfs_join_transaction() won't wait full tickets, it won't get
interrupted from signal.

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index c0bc35f932bf..d8ef48a807d1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5298,7 +5298,10 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 		goto out;
 	}
 
-	trans = btrfs_start_transaction(tree_root, 0);
+	if (for_reloc)
+		trans = btrfs_join_transaction(tree_root);
+	else
+		trans = btrfs_start_transaction(tree_root, 0);
 	if (IS_ERR(trans)) {
 		err = PTR_ERR(trans);
 		goto out_free;
-- 
2.27.0

[PATCH 2/2] btrfs: relocation: review the call sites which can be interruped by signal

[Qu Wenruo]

Since most metadata reservation calls can return -EINTR when get
interruped by fatal signal, we need to review the all the metadata
reservation call sites.

In relocation code, the metadata reservation happens in the following
sites:
- btrfs_block_rsv_refill() in merge_reloc_root()
  merge_reloc_root() is a pretty critial section, we don't want get
  interrupted by signal, so change the flush status to
  BTRFS_RESERVE_FLUSH_LIMIT, so it won't get interrupted by signal.
  Since such change can be ENPSPC-prone, also shrink the amount of
  metadata to reserve a little to avoid deadly ENOSPC there.

- btrfs_block_rsv_refill() in reserve_metadata_space()
  It calls with BTRFS_RESERVE_FLUSH_LIMIT, which won't get interrupred
  by signal.

- btrfs_block_rsv_refill() in prepare_to_relocate()
- btrfs_block_rsv_add() in prepare_to_relocate()
- btrfs_block_rsv_refill() in relocate_block_group()
- btrfs_delalloc_reserve_metadata() in relocate_file_extent_cluster()
- btrfs_start_transaction() in relocate_block_group()
- btrfs_start_transaction() in create_reloc_inode()
  Can be interruped by fatal signal and we can handle it easily.
  For these call sites, just catch the -EINTR value in btrfs_balance()
  and count them as canceled.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/relocation.c | 4 ++--
 fs/btrfs/volumes.c    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 2b869fb2e62c..29bbead29be5 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1686,12 +1686,12 @@ static noinline_for_stack int merge_reloc_root(struct reloc_control *rc,
 		btrfs_unlock_up_safe(path, 0);
 	}
 
-	min_reserved = fs_info->nodesize * (BTRFS_MAX_LEVEL - 1) * 2;
+	min_reserved = fs_info->nodesize * level * 2;
 	memset(&next_key, 0, sizeof(next_key));
 
 	while (1) {
 		ret = btrfs_block_rsv_refill(root, rc->block_rsv, min_reserved,
-					     BTRFS_RESERVE_FLUSH_ALL);
+					     BTRFS_RESERVE_FLUSH_LIMIT);
 		if (ret) {
 			err = ret;
 			goto out;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index aabc6c922e04..d60df30bdc47 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4135,7 +4135,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 	mutex_lock(&fs_info->balance_mutex);
 	if (ret == -ECANCELED && atomic_read(&fs_info->balance_pause_req))
 		btrfs_info(fs_info, "balance: paused");
-	else if (ret == -ECANCELED && atomic_read(&fs_info->balance_cancel_req))
+	else if (ret == -ECANCELED  || ret == -EINTR)
 		btrfs_info(fs_info, "balance: canceled");
 	else
 		btrfs_info(fs_info, "balance: ended with status: %d", ret);
-- 
2.27.0

Re: [PATCH 2/2] btrfs: relocation: review the call sites which can be interruped by signal

[Nikolay Borisov]

On 8.07.20 г. 13:00 ч., Qu Wenruo wrote:
> Since most metadata reservation calls can return -EINTR when get
> interruped by fatal signal, we need to review the all the metadata
> reservation call sites.
> 
> In relocation code, the metadata reservation happens in the following
> sites:
> - btrfs_block_rsv_refill() in merge_reloc_root()
>   merge_reloc_root() is a pretty critial section, we don't want get
>   interrupted by signal, so change the flush status to
>   BTRFS_RESERVE_FLUSH_LIMIT, so it won't get interrupted by signal.
>   Since such change can be ENPSPC-prone, also shrink the amount of
>   metadata to reserve a little to avoid deadly ENOSPC there.

You need to either document why do you think this smaller reservation is
fine in the changelog or document it it with a comment above it.

> 
> - btrfs_block_rsv_refill() in reserve_metadata_space()
>   It calls with BTRFS_RESERVE_FLUSH_LIMIT, which won't get interrupred
>   by signal.
> 
> - btrfs_block_rsv_refill() in prepare_to_relocate()
> - btrfs_block_rsv_add() in prepare_to_relocate()
> - btrfs_block_rsv_refill() in relocate_block_group()
> - btrfs_delalloc_reserve_metadata() in relocate_file_extent_cluster()
> - btrfs_start_transaction() in relocate_block_group()
> - btrfs_start_transaction() in create_reloc_inode()
>   Can be interruped by fatal signal and we can handle it easily.
>   For these call sites, just catch the -EINTR value in btrfs_balance()
>   and count them as canceled.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/relocation.c | 4 ++--
>  fs/btrfs/volumes.c    | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 2b869fb2e62c..29bbead29be5 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -1686,12 +1686,12 @@ static noinline_for_stack int merge_reloc_root(struct reloc_control *rc,
>  		btrfs_unlock_up_safe(path, 0);
>  	}
>  
> -	min_reserved = fs_info->nodesize * (BTRFS_MAX_LEVEL - 1) * 2;
> +	min_reserved = fs_info->nodesize * level * 2;
>  	memset(&next_key, 0, sizeof(next_key));
>  
>  	while (1) {
>  		ret = btrfs_block_rsv_refill(root, rc->block_rsv, min_reserved,
> -					     BTRFS_RESERVE_FLUSH_ALL);
> +					     BTRFS_RESERVE_FLUSH_LIMIT);
>  		if (ret) {
>  			err = ret;
>  			goto out;
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index aabc6c922e04..d60df30bdc47 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4135,7 +4135,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  	mutex_lock(&fs_info->balance_mutex);
>  	if (ret == -ECANCELED && atomic_read(&fs_info->balance_pause_req))
>  		btrfs_info(fs_info, "balance: paused");
> -	else if (ret == -ECANCELED && atomic_read(&fs_info->balance_cancel_req))
> +	else if (ret == -ECANCELED  || ret == -EINTR)
>  		btrfs_info(fs_info, "balance: canceled");
>  	else
>  		btrfs_info(fs_info, "balance: ended with status: %d", ret);
> 

Re: [PATCH 1/2] btrfs: avoid possible signal interruption for btrfs_drop_snapshot() on relocation tree

[Josef Bacik]

On 7/8/20 6:00 AM, Qu Wenruo wrote:
> [BUG]
> There is a bug report about bad signal timing could lead to read-only
> fs during balance:
> 
>    BTRFS info (device xvdb): balance: start -d -m -s
>    BTRFS info (device xvdb): relocating block group 73001861120 flags metadata
>    BTRFS info (device xvdb): found 12236 extents, stage: move data extents
>    BTRFS info (device xvdb): relocating block group 71928119296 flags data
>    BTRFS info (device xvdb): found 3 extents, stage: move data extents
>    BTRFS info (device xvdb): found 3 extents, stage: update data pointers
>    BTRFS info (device xvdb): relocating block group 60922265600 flags metadata
>    BTRFS: error (device xvdb) in btrfs_drop_snapshot:5505: errno=-4 unknown
>    BTRFS info (device xvdb): forced readonly
>    BTRFS info (device xvdb): balance: ended with status: -4
> 
> [CAUSE]
> The direct cause is the -EINTR from the following call chain when a
> fatal signal is pending:
> 
>   relocate_block_group()
>   |- clean_dirty_subvols()
>      |- btrfs_drop_snapshot()
>         |- btrfs_start_transaction()
>            |- btrfs_delayed_refs_rsv_refill()
>               |- btrfs_reserve_metadata_bytes()
>                  |- __reserve_metadata_bytes()
>                     |- wait_reserve_ticket()
>                        |- prepare_to_wait_event();
>                        |- ticket->error = -EINTR;
> 
> Normally this behavior is fine for most btrfs_start_transaction()
> callers, as they need to catch the fatal signal and exit asap.
> 
> However to balance, especially for the clean_dirty_subvols() case, we're
> already doing cleanup works, such -EINTR from btrfs_drop_snapshot()
> could cause a lot of unexpected problems.
> 
>  From the mentioned forced read-only, to later balance error due to half
> dropped reloc trees.
> 
> [FIX]
> Fix this problem by using btrfs_join_transaction() if
> btrfs_drop_snapshot() is called from relocation context.
> 
> As btrfs_join_transaction() won't wait full tickets, it won't get
> interrupted from signal.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

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

Thanks,

Josef

[PATCH 1/2] btrfs: avoid possible signal interruption for btrfs_drop_snapshot() on relocation tree

[Qu Wenruo]

[BUG]
There is a bug report about bad signal timing could lead to read-only
fs during balance:

  BTRFS info (device xvdb): balance: start -d -m -s
  BTRFS info (device xvdb): relocating block group 73001861120 flags metadata
  BTRFS info (device xvdb): found 12236 extents, stage: move data extents
  BTRFS info (device xvdb): relocating block group 71928119296 flags data
  BTRFS info (device xvdb): found 3 extents, stage: move data extents
  BTRFS info (device xvdb): found 3 extents, stage: update data pointers
  BTRFS info (device xvdb): relocating block group 60922265600 flags metadata
  BTRFS: error (device xvdb) in btrfs_drop_snapshot:5505: errno=-4 unknown
  BTRFS info (device xvdb): forced readonly
  BTRFS info (device xvdb): balance: ended with status: -4

[CAUSE]
The direct cause is the -EINTR from the following call chain when a
fatal signal is pending:

 relocate_block_group()
 |- clean_dirty_subvols()
    |- btrfs_drop_snapshot()
       |- btrfs_start_transaction()
          |- btrfs_delayed_refs_rsv_refill()
             |- btrfs_reserve_metadata_bytes()
                |- __reserve_metadata_bytes()
                   |- wait_reserve_ticket()
                      |- prepare_to_wait_event();
                      |- ticket->error = -EINTR;

Normally this behavior is fine for most btrfs_start_transaction()
callers, as they need to catch the fatal signal and exit asap.

However to balance, especially for the clean_dirty_subvols() case, we're
already doing cleanup works, such -EINTR from btrfs_drop_snapshot()
could cause a lot of unexpected problems.

From the mentioned forced read-only, to later balance error due to half
dropped reloc trees.

[FIX]
Fix this problem by using btrfs_join_transaction() if
btrfs_drop_snapshot() is called from relocation context.

As btrfs_join_transaction() won't wait full tickets, it won't get
interrupted from signal.

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index c0bc35f932bf..d8ef48a807d1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5298,7 +5298,10 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 		goto out;
 	}
 
-	trans = btrfs_start_transaction(tree_root, 0);
+	if (for_reloc)
+		trans = btrfs_join_transaction(tree_root);
+	else
+		trans = btrfs_start_transaction(tree_root, 0);
 	if (IS_ERR(trans)) {
 		err = PTR_ERR(trans);
 		goto out_free;
-- 
2.27.0