[v2,2/2] btrfs: relocation: review the call sites which can be interruped by signal
diff mbox series

Message ID 20200709083333.137927-2-wqu@suse.com
State New
Headers show
Series
  • [v2,1/2] btrfs: avoid possible signal interruption for btrfs_drop_snapshot() on relocation tree
Related show

Commit Message

Qu Wenruo July 9, 2020, 8:33 a.m. UTC
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 | 13 +++++++++++--
 fs/btrfs/volumes.c    |  2 +-
 2 files changed, 12 insertions(+), 3 deletions(-)

Comments

David Sterba July 9, 2020, 9:54 a.m. UTC | #1
On Thu, Jul 09, 2020 at 04:33:33PM +0800, 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.
> 
> - btrfs_block_rsv_refill() in reserve_metadata_space()
>   It calls with BTRFS_RESERVE_FLUSH_LIMIT, which won't get interrupred
>   by signal.

This semantics of BTRFS_RESERVE_FLUSH_LIMIT regarding signals should be
documented, right now there's a comment but says something about avoidig
deadlocks.

> - 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 | 13 +++++++++++--
>  fs/btrfs/volumes.c    |  2 +-
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 2b869fb2e62c..23914edd4710 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -1686,12 +1686,21 @@ 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;
> +	/*
> +	 * In merge_reloc_root(), we modify the upper level pointer to swap
> +	 * the tree blocks between reloc tree and subvolume tree.
> +	 * Thus for tree block COW, we COW at most from level 1 to root level
> +	 * for each tree.
> +	 *
> +	 * Thus the needed metadata space is at most root_level * nodesize,
> +	 * and * 2 since we have two trees to COW.
> +	 */
> +	min_reserved = fs_info->nodesize * btrfs_root_level(root_item) * 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)

Why do you remove atomic_read(&fs_info->balance_cancel_req) ?

>  		btrfs_info(fs_info, "balance: canceled");

I'm not sure if it would be useful to print the reason, like

- 'canceled: user request'
- 'canceled: interrupted'

>  	else
>  		btrfs_info(fs_info, "balance: ended with status: %d", ret);
> -- 
> 2.27.0
Qu Wenruo July 9, 2020, 10:15 a.m. UTC | #2
On 2020/7/9 下午5:54, David Sterba wrote:
> On Thu, Jul 09, 2020 at 04:33:33PM +0800, 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.
>>
>> - btrfs_block_rsv_refill() in reserve_metadata_space()
>>   It calls with BTRFS_RESERVE_FLUSH_LIMIT, which won't get interrupred
>>   by signal.
> 
> This semantics of BTRFS_RESERVE_FLUSH_LIMIT regarding signals should be
> documented, right now there's a comment but says something about avoidig
> deadlocks.
> 
>> - 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 | 13 +++++++++++--
>>  fs/btrfs/volumes.c    |  2 +-
>>  2 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index 2b869fb2e62c..23914edd4710 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -1686,12 +1686,21 @@ 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;
>> +	/*
>> +	 * In merge_reloc_root(), we modify the upper level pointer to swap
>> +	 * the tree blocks between reloc tree and subvolume tree.
>> +	 * Thus for tree block COW, we COW at most from level 1 to root level
>> +	 * for each tree.
>> +	 *
>> +	 * Thus the needed metadata space is at most root_level * nodesize,
>> +	 * and * 2 since we have two trees to COW.
>> +	 */
>> +	min_reserved = fs_info->nodesize * btrfs_root_level(root_item) * 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)
> 
> Why do you remove atomic_read(&fs_info->balance_cancel_req) ?

Because now btrfs_should_cancel_balance() can return ECANCELED without
balance_cancel_req increased due to pending fatal signal.

> 
>>  		btrfs_info(fs_info, "balance: canceled");
> 
> I'm not sure if it would be useful to print the reason, like
> 
> - 'canceled: user request'
> - 'canceled: interrupted'

To me, if user interrupt the balance progress, it's obvious they want to
cancel it.
Thus no need to distinguish btrfs balance cancel and signal cancel.

Thanks,
Qu

> 
>>  	else
>>  		btrfs_info(fs_info, "balance: ended with status: %d", ret);
>> -- 
>> 2.27.0
David Sterba July 9, 2020, 10:25 a.m. UTC | #3
On Thu, Jul 09, 2020 at 06:15:09PM +0800, Qu Wenruo wrote:
> On 2020/7/9 下午5:54, David Sterba wrote:
> > On Thu, Jul 09, 2020 at 04:33:33PM +0800, Qu Wenruo wrote:
> >> @@ -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)
> > 
> > Why do you remove atomic_read(&fs_info->balance_cancel_req) ?
> 
> Because now btrfs_should_cancel_balance() can return ECANCELED without
> balance_cancel_req increased due to pending fatal signal.

Ah right, I misread it as || which would remove one reason for
cancellation.
Qu Wenruo July 9, 2020, 10:46 a.m. UTC | #4
On 2020/7/9 下午5:54, David Sterba wrote:
> On Thu, Jul 09, 2020 at 04:33:33PM +0800, 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.
>>
>> - btrfs_block_rsv_refill() in reserve_metadata_space()
>>   It calls with BTRFS_RESERVE_FLUSH_LIMIT, which won't get interrupred
>>   by signal.
> 
> This semantics of BTRFS_RESERVE_FLUSH_LIMIT regarding signals should be
> documented, right now there's a comment but says something about avoidig
> deadlocks.

For this, I tend to add one or more patches to add some comment for all
FLUSH enums, and all their callers.

I hate when some infrastructure hit me by surprise, and since ticketing
system is hidden from a lot of functions, we also need to mention that
-EINTR case.

Thanks,
Qu

> 
>> - 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 | 13 +++++++++++--
>>  fs/btrfs/volumes.c    |  2 +-
>>  2 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index 2b869fb2e62c..23914edd4710 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -1686,12 +1686,21 @@ 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;
>> +	/*
>> +	 * In merge_reloc_root(), we modify the upper level pointer to swap
>> +	 * the tree blocks between reloc tree and subvolume tree.
>> +	 * Thus for tree block COW, we COW at most from level 1 to root level
>> +	 * for each tree.
>> +	 *
>> +	 * Thus the needed metadata space is at most root_level * nodesize,
>> +	 * and * 2 since we have two trees to COW.
>> +	 */
>> +	min_reserved = fs_info->nodesize * btrfs_root_level(root_item) * 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)
> 
> Why do you remove atomic_read(&fs_info->balance_cancel_req) ?
> 
>>  		btrfs_info(fs_info, "balance: canceled");
> 
> I'm not sure if it would be useful to print the reason, like
> 
> - 'canceled: user request'
> - 'canceled: interrupted'
> 
>>  	else
>>  		btrfs_info(fs_info, "balance: ended with status: %d", ret);
>> -- 
>> 2.27.0

Patch
diff mbox series

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 2b869fb2e62c..23914edd4710 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1686,12 +1686,21 @@  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;
+	/*
+	 * In merge_reloc_root(), we modify the upper level pointer to swap
+	 * the tree blocks between reloc tree and subvolume tree.
+	 * Thus for tree block COW, we COW at most from level 1 to root level
+	 * for each tree.
+	 *
+	 * Thus the needed metadata space is at most root_level * nodesize,
+	 * and * 2 since we have two trees to COW.
+	 */
+	min_reserved = fs_info->nodesize * btrfs_root_level(root_item) * 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);