Re: [PATCH 08/16] btrfs: add sanity check when resuming balance after mount

[David Sterba]

On Mon, Apr 16, 2018 at 02:10:08PM +0800, Anand Jain wrote:
> 
> 
> On 04/04/2018 02:34 AM, David Sterba wrote:
> > Replace a WARN_ON with a proper check and message in case something goes
> > really wrong and resumed balance cannot set up its exclusive status.
> 
> > The check is a user friendly assertion, I don't expect to ever happen
> > under normal circumstances.
> > 
> > Also document that the paused balance starts here and owns the exclusive
> > op status.
> 
> 
> > Signed-off-by: David Sterba 
> > ---
> >   fs/btrfs/volumes.c | 16 ++--
> >   1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index eb78c1d0ce2b..843982a2cbdb 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -3982,6 +3982,20 @@ int btrfs_recover_balance(struct btrfs_fs_info 
> > *fs_info)
> > struct btrfs_key key;
> > int ret;
> >   
> > +   /*
> > +* This should never happen, as the paused balance state is recovered
> > +* during mount without any chance of other exclusive ops to collide.
> > +* Let it fail early and do not continue mount.
> > +*
> > +* Otherwise, this gives the exclusive op status to balance and keeps
> > +* in paused state until user intervention (cancel or umount).
> > +*/
> > +   if (test_and_set_bit(BTRFS_FS_EXCL_OP, _info->flags)) {
> > +   btrfs_err(fs_info,
> > +   "cannot set exclusive op status to resume balance");
> > +   return -EINVAL;
> > +   }
> >
> 
>   We need the test_and_set_bit(BTRFS_FS_EXCL_OP) only if we confirm that
>   there is a pending balance. Its better to test and set at the same
>   place as WARN_ON before.

Right.  And the patch is wrong, because we need to set up
fs_info::balance_ctl in all cases, otherwise unpausing balance would not
work as expected. The only change should be a better message and maybe
not even that, as it's just preparing the state but the exclusive op is
claimed later in btrfs_resume_balance_async.

I'll revisit how the error handling is done after resuming balance or
dev-replace, this is considered a hard failure (mount, rw remount) but I
don't think it's necessary.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/16] btrfs: add sanity check when resuming balance after mount

[Anand Jain]

On 04/04/2018 02:34 AM, David Sterba wrote:

Replace a WARN_ON with a proper check and message in case something goes
really wrong and resumed balance cannot set up its exclusive status.



The check is a user friendly assertion, I don't expect to ever happen
under normal circumstances.

Also document that the paused balance starts here and owns the exclusive
op status.




Signed-off-by: David Sterba 
---
  fs/btrfs/volumes.c | 16 ++--
  1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index eb78c1d0ce2b..843982a2cbdb 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3982,6 +3982,20 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
struct btrfs_key key;
int ret;
  
+	/*

+* This should never happen, as the paused balance state is recovered
+* during mount without any chance of other exclusive ops to collide.
+* Let it fail early and do not continue mount.
+*
+* Otherwise, this gives the exclusive op status to balance and keeps
+* in paused state until user intervention (cancel or umount).
+*/
+   if (test_and_set_bit(BTRFS_FS_EXCL_OP, _info->flags)) {
+   btrfs_err(fs_info,
+   "cannot set exclusive op status to resume balance");
+   return -EINVAL;
+   }



 We need the test_and_set_bit(BTRFS_FS_EXCL_OP) only if we confirm that
 there is a pending balance. Its better to test and set at the same
 place as WARN_ON before.

Thanks, Anand



path = btrfs_alloc_path();
if (!path)
return -ENOMEM;
@@ -4018,8 +4032,6 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
btrfs_balance_sys(leaf, item, _bargs);
btrfs_disk_balance_args_to_cpu(>sys, _bargs);
  
-	WARN_ON(test_and_set_bit(BTRFS_FS_EXCL_OP, _info->flags));

-
mutex_lock(_info->volume_mutex);
mutex_lock(_info->balance_mutex);
  


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/16] btrfs: add sanity check when resuming balance after mount

[Anand Jain]

On 04/04/2018 02:34 AM, David Sterba wrote:

Replace a WARN_ON with a proper check and message in case something goes
really wrong and resumed balance cannot set up its exclusive status.
The check is a user friendly assertion, I don't expect to ever happen
under normal circumstances.

Also document that the paused balance starts here and owns the exclusive
op status.

Signed-off-by: David Sterba 
---
  fs/btrfs/volumes.c | 16 ++--
  1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index eb78c1d0ce2b..843982a2cbdb 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3982,6 +3982,20 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
struct btrfs_key key;
int ret;
  
+	/*

+* This should never happen, as the paused balance state is recovered
+* during mount without any chance of other exclusive ops to collide.
+* Let it fail early and do not continue mount.
+*
+* Otherwise, this gives the exclusive op status to balance and keeps
+* in paused state until user intervention (cancel or umount).
+*/
+   if (test_and_set_bit(BTRFS_FS_EXCL_OP, _info->flags)) {
+   btrfs_err(fs_info,
+   "cannot set exclusive op status to resume balance");
+   return -EINVAL;
+   }
+



 Reviewed-by: Anand Jain 

Thanks, Anand



path = btrfs_alloc_path();
if (!path)
return -ENOMEM;
@@ -4018,8 +4032,6 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
btrfs_balance_sys(leaf, item, _bargs);
btrfs_disk_balance_args_to_cpu(>sys, _bargs);
  
-	WARN_ON(test_and_set_bit(BTRFS_FS_EXCL_OP, _info->flags));

-
mutex_lock(_info->volume_mutex);
mutex_lock(_info->balance_mutex);
  


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/16] btrfs: add sanity check when resuming balance after mount

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed by the -stable helper bot and determined
to be a high probability candidate for -stable trees. (score: 16.7330)

The bot has tested the following trees: v4.16, v4.15.15, v4.14.32, v4.9.92, 
v4.4.126.

v4.16: Build OK!
v4.15.15: Build OK!
v4.14.32: Build OK!
v4.9.92: Failed to apply! Possible dependencies:
509cdd5c938a ("btrfs: add sanity check when resuming balance after mount")

v4.4.126: Failed to apply! Possible dependencies:
509cdd5c938a ("btrfs: add sanity check when resuming balance after mount")


Please let us know if you'd like to have this patch included in a stable tree.

--
Thanks,
Sasha--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html