Scrub resume regression

Scrub resume regression

[Graham Cobb]

OK, I have bisected the problem with scrub resume being broken by the
scrub ioctl ABI being changed.

The bad commit is:

Fail
06fe39ab15a6a47d4979460fcc17d33b1d72ccf9 is the first bad commit
commit 06fe39ab15a6a47d4979460fcc17d33b1d72ccf9
Author: Filipe Manana <fdmanana@suse.com>
Date:   Fri Dec 14 19:50:17 2018 +0000

    Btrfs: do not overwrite scrub error with fault error in scrub ioctl

    If scrub returned an error and then the copy_to_user() call did not
    succeed, we would overwrite the error returned by scrub with -EFAULT.
    Fix that by calling copy_to_user() only if btrfs_scrub_dev() returned
    success.

    Signed-off-by: Filipe Manana <fdmanana@suse.com>
    Reviewed-by: David Sterba <dsterba@suse.com>
    Signed-off-by: David Sterba <dsterba@suse.com>

 fs/btrfs/ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
bisect run success

It is important that scrub always returns the stats, even when it
returns an error. This is critical for cancel, as that is how
cancel/resume works, but it should also apply in case of other errors so
that the user can see how much of the scrub was done before the fatal error.

I am not sure in which kernel release this commit appeared but as this
breaks the "scrub resume" command completely, I think the fix for this
needs to be backported and may want to be considered by distro kernel
maintainers.

I will reply later with the simple reproducer program I created for the
bisection in case it is useful for testing.

Re: Scrub resume regression

[Filipe Manana]

On Wed, Jan 15, 2020 at 9:04 AM Graham Cobb <g.btrfs@cobb.uk.net> wrote:
>
> OK, I have bisected the problem with scrub resume being broken by the
> scrub ioctl ABI being changed.
>
> The bad commit is:
>
> Fail
> 06fe39ab15a6a47d4979460fcc17d33b1d72ccf9 is the first bad commit
> commit 06fe39ab15a6a47d4979460fcc17d33b1d72ccf9
> Author: Filipe Manana <fdmanana@suse.com>
> Date:   Fri Dec 14 19:50:17 2018 +0000
>
>     Btrfs: do not overwrite scrub error with fault error in scrub ioctl
>
>     If scrub returned an error and then the copy_to_user() call did not
>     succeed, we would overwrite the error returned by scrub with -EFAULT.
>     Fix that by calling copy_to_user() only if btrfs_scrub_dev() returned
>     success.
>
>     Signed-off-by: Filipe Manana <fdmanana@suse.com>
>     Reviewed-by: David Sterba <dsterba@suse.com>
>     Signed-off-by: David Sterba <dsterba@suse.com>
>
>  fs/btrfs/ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> bisect run success
>
> It is important that scrub always returns the stats, even when it
> returns an error. This is critical for cancel, as that is how
> cancel/resume works, but it should also apply in case of other errors so
> that the user can see how much of the scrub was done before the fatal error.
>
> I am not sure in which kernel release this commit appeared but as this
> breaks the "scrub resume" command completely, I think the fix for this
> needs to be backported and may want to be considered by distro kernel
> maintainers.
>
> I will reply later with the simple reproducer program I created for the
> bisection in case it is useful for testing.

No need to, it is simple to understand why it happens and the not
copying that stats in error case was not intentional.

Try this:

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 3a4bd5cd67fa..611dfe8cdbb1 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4253,8 +4253,10 @@ static long btrfs_ioctl_scrub(struct file
*file, void __user *arg)
                              &sa->progress, sa->flags & BTRFS_SCRUB_READONLY,
                              0);

-       if (ret == 0 && copy_to_user(arg, sa, sizeof(*sa)))
-               ret = -EFAULT;
+       if (copy_to_user(arg, sa, sizeof(*sa))) {
+               if (!ret)
+                       ret = -EFAULT;
+       }

        if (!(sa->flags & BTRFS_SCRUB_READONLY))
                mnt_drop_write_file(file);

I'll later send a patch with a changelog to the list.
Thanks.



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: Scrub resume regression

[Graham Cobb]

On 15/01/2020 09:33, Filipe Manana wrote:
> On Wed, Jan 15, 2020 at 9:04 AM Graham Cobb <g.btrfs@cobb.uk.net> wrote:
>>
>> OK, I have bisected the problem with scrub resume being broken by the
>> scrub ioctl ABI being changed.
[snip]
> 
> Try this:
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 3a4bd5cd67fa..611dfe8cdbb1 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4253,8 +4253,10 @@ static long btrfs_ioctl_scrub(struct file
> *file, void __user *arg)
>                               &sa->progress, sa->flags & BTRFS_SCRUB_READONLY,
>                               0);
> 
> -       if (ret == 0 && copy_to_user(arg, sa, sizeof(*sa)))
> -               ret = -EFAULT;
> +       if (copy_to_user(arg, sa, sizeof(*sa))) {
> +               if (!ret)
> +                       ret = -EFAULT;
> +       }
> 
>         if (!(sa->flags & BTRFS_SCRUB_READONLY))
>                 mnt_drop_write_file(file);
> 
> I'll later send a patch with a changelog to the list.
> Thanks.

Thanks Filipe. I have tested that the patch builds and works with my
reproducer on a 5.4 kernel.

Later I will build a distro kernel with the patch and try my monthly
scrub-with-interruptions script. That will take a day or two.

Re: Scrub resume regression

[David Sterba]

On Wed, Jan 15, 2020 at 09:03:13AM +0000, Graham Cobb wrote:
> OK, I have bisected the problem with scrub resume being broken by the
> scrub ioctl ABI being changed.

Thanks for the bisecting, I'll forward the fix to 5.5-rc so it can be
picked by stable trees. The regression has been there since 5.1, which
at the moment is only for 5.4.x.

> The bad commit is:
> 
> 06fe39ab15a6a47d4979460fcc17d33b1d72ccf9 is the first bad commit
> commit 06fe39ab15a6a47d4979460fcc17d33b1d72ccf9
> Author: Filipe Manana <fdmanana@suse.com>
> Date:   Fri Dec 14 19:50:17 2018 +0000
> 
>     Btrfs: do not overwrite scrub error with fault error in scrub ioctl
[...]

> It is important that scrub always returns the stats, even when it
> returns an error. This is critical for cancel, as that is how
> cancel/resume works, but it should also apply in case of other errors so
> that the user can see how much of the scrub was done before the fatal error.

That's something we need to document in code and perhaps in the manual
pages too.

Re: Scrub resume regression

[Holger Hoffstätte]

On 1/15/20 1:51 PM, David Sterba wrote:
>> It is important that scrub always returns the stats, even when it
>> returns an error. This is critical for cancel, as that is how
>> cancel/resume works, but it should also apply in case of other errors so
>> that the user can see how much of the scrub was done before the fatal error.
> 
> That's something we need to document in code and perhaps in the manual
> pages too.

Isn't the real problem that cancel does not actually mean cancel,
but rather also implies "..and maybe continue"? IMHO cancel should cancel
(and say how much work was performed), while the intention to resume should
be called e.g. "pause". This makes the behaviour clear and prevents
accidental semantic overlap.

-h

Re: Scrub resume regression

[Sebastian Döring]

> Isn't the real problem that cancel does not actually mean cancel,
> but rather also implies "..and maybe continue"? IMHO cancel should cancel
> (and say how much work was performed), while the intention to resume should
> be called e.g. "pause". This makes the behaviour clear and prevents
> accidental semantic overlap.
>

IMHO a better choice of wording might also be "interrupt" and "abort",
but maybe that's just being pedantic (cancel could then stay in place
for backwards compatibility). In any case the current man page for
btrfs-progs makes it somewhat clear what's supposed to happen and my
confusion was mostly caused by the bug.

BTW: Thanks to everyone who helped tracking down the cause of this
bug. Haven't had a chance to test the proposed patch yet, but am
looking forward to schedule large scrubs across multiple nights.

Re: Scrub resume regression

[David Sterba]

On Wed, Jan 15, 2020 at 02:10:42PM +0100, Holger Hoffstätte wrote:
> On 1/15/20 1:51 PM, David Sterba wrote:
> >> It is important that scrub always returns the stats, even when it
> >> returns an error. This is critical for cancel, as that is how
> >> cancel/resume works, but it should also apply in case of other errors so
> >> that the user can see how much of the scrub was done before the fatal error.
> > 
> > That's something we need to document in code and perhaps in the manual
> > pages too.
> 
> Isn't the real problem that cancel does not actually mean cancel,
> but rather also implies "..and maybe continue"? IMHO cancel should cancel
> (and say how much work was performed), while the intention to resume should
> be called e.g. "pause". This makes the behaviour clear and prevents
> accidental semantic overlap.

We can add 'pause', but for backward compatibility, cancel has to stay
as is. I personally think that saving the last position after cancel is
not a big deal. With 'pause' it will be less confusing for users and
will have also parity with balance commands.

start - pause - resume
start - cancel

One difference is that cancelling balance will also delete the state
(stored inside the filesystem metadata). If scrub start follows cancel,
the state is reset at the beginning. I'm not sure if adding an extra
option eg. 'scrub cancel --reset' is worth.

Re: Scrub resume regression

[Zygo Blaxell]

[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]

On Thu, Jan 16, 2020 at 03:02:27PM +0100, David Sterba wrote:
> On Wed, Jan 15, 2020 at 02:10:42PM +0100, Holger Hoffstätte wrote:
> > On 1/15/20 1:51 PM, David Sterba wrote:
> > >> It is important that scrub always returns the stats, even when it
> > >> returns an error. This is critical for cancel, as that is how
> > >> cancel/resume works, but it should also apply in case of other errors so
> > >> that the user can see how much of the scrub was done before the fatal error.
> > > 
> > > That's something we need to document in code and perhaps in the manual
> > > pages too.
> > 
> > Isn't the real problem that cancel does not actually mean cancel,
> > but rather also implies "..and maybe continue"? IMHO cancel should cancel
> > (and say how much work was performed), while the intention to resume should
> > be called e.g. "pause". This makes the behaviour clear and prevents
> > accidental semantic overlap.
> 
> We can add 'pause', but for backward compatibility, cancel has to stay
> as is. I personally think that saving the last position after cancel is
> not a big deal. With 'pause' it will be less confusing for users and
> will have also parity with balance commands.
> 
> start - pause - resume
> start - cancel
> 
> One difference is that cancelling balance will also delete the state
> (stored inside the filesystem metadata). If scrub start follows cancel,
> the state is reset at the beginning. I'm not sure if adding an extra
> option eg. 'scrub cancel --reset' is worth.

Currently 'scrub cancel' doesn't reset the state, so existing scripts will
be broken if 'cancel' is not exactly the same as 'pause'.  I have such
scripts, they call 'cancel' and 'resume' blindly according to load or
maintenance window boundaries, and they leave it to btrfs userspace to
sort out whether any new work should be done.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Scrub resume regression

[Chris Murphy]

It's no one's fault, it's just confusing. :P

Cancel word origin means more than stop, implies resetting state, to
obliterate or invalidate.

Pause and stop word origin suggests they're interchangeable, but in
practice with digital audio and video consumer gear, stop has come to
mean a kind of cancel. (I'm gonna ignore tape.) Where a start from a
stop will start at the very beginning. Whereas pause saves state and
unpause means resume.

Lightweight change, add new command stop, which saves state, and
cancel is an alias for backward compatibility. No other change.

Moderate change:
start = alias resume
stop = alias cancel
i.e. a stop then start does the same thing as a cancel then resume,
unless new command 'reset' is used
reset = stops, and resets state to the beginning

Heavier change that's linguistically sane, but breaks expectations of
today's cancel:
pause and unpause (alias resume), and start and stop (alias cancel).
The former is stateful, and the latter is stateless.

But this problem should be looked at by i18n folks before changing anything.

---
Chris Murphy

Re: Scrub resume regression

[Graham Cobb]

On 17/01/2020 18:39, Chris Murphy wrote:
> It's no one's fault, it's just confusing. :P
> 
> Cancel word origin means more than stop, implies resetting state, to
> obliterate or invalidate.
> 
> Pause and stop word origin suggests they're interchangeable, but in
> practice with digital audio and video consumer gear, stop has come to
> mean a kind of cancel. (I'm gonna ignore tape.) Where a start from a
> stop will start at the very beginning. Whereas pause saves state and
> unpause means resume.
> 
> Lightweight change, add new command stop, which saves state, and
> cancel is an alias for backward compatibility. No other change.

That seems fairly pointless.

> Moderate change:
> start = alias resume

start and resume do different things today. The distinction is important
as the "saved state" after a cancel stays around until the next scrub
starts (it could be months old).

> stop = alias cancel
> i.e. a stop then start does the same thing as a cancel then resume,
> unless new command 'reset' is used
> reset = stops, and resets state to the beginning

That really isn't useful. It is much more useful to be able to decide
whether to use the saved state at start time than it is to decide
whether to save state at stop time. To build on Zygo's example, I might
have a script which pauses the scrub when the system load goes up and
then decides whether to resume it or restart from scratch depending on
how long it has been paused for. Also, the saved state can be inspected
while the scrub is paused to allow the operator to estimate how long it
might take to complete.

> Heavier change that's linguistically sane, but breaks expectations of
> today's cancel:
> pause and unpause (alias resume), and start and stop (alias cancel).
> The former is stateful, and the latter is stateless.

Changing the meaning of the current start, resume, or cancel commands
isn't an option - these are built into user scripts.