Re: Scrub resume regression

[Filipe Manana <fdmanana@gmail.com>]

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.”