[PATCH] Btrfs: disable online scrub repair on ro cases

[PATCH] Btrfs: disable online scrub repair on ro cases

[Liu Bo]

This disables repair process on ro cases as it can cause system
to be unresponsive on the ASSERT() in repair_io_failure().

This can happen when scrub is running and a hardware error pops up,
we should fallback to ro mounts gracefully instead of being unresponsive.

Reported-by: Codebird <codebird@birds-are-nice.me>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/scrub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 2907a77..d6dd2f5 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -682,6 +682,9 @@ static int scrub_fixup_readpage(u64 inum, u64 offset, u64 root, void *fixup_ctx)
 	struct btrfs_root *local_root;
 	int srcu_index;
 
+	if (root->fs_info->sb->s_flags & MS_RDONLY)
+		return -EROFS;
+
 	key.objectid = root;
 	key.type = BTRFS_ROOT_ITEM_KEY;
 	key.offset = (u64)-1;
-- 
2.5.0

Re: [PATCH] Btrfs: disable online scrub repair on ro cases

[kbuild test robot]

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

Hi Liu,

[auto build test ERROR on btrfs/next]
[also build test ERROR on v4.4-rc3 next-20151203]

url:    https://github.com/0day-ci/linux/commits/Liu-Bo/Btrfs-disable-online-scrub-repair-on-ro-cases/20151204-205115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mason/linux-btrfs.git next
config: powerpc-defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   fs/btrfs/scrub.c: In function 'scrub_fixup_readpage':
>> fs/btrfs/scrub.c:703:10: error: invalid type argument of '->' (have 'u64 {aka long long unsigned int}')
     if (root->fs_info->sb->s_flags & MS_RDONLY)
             ^

vim +703 fs/btrfs/scrub.c

   697		struct inode *inode = NULL;
   698		struct btrfs_fs_info *fs_info;
   699		u64 end = offset + PAGE_SIZE - 1;
   700		struct btrfs_root *local_root;
   701		int srcu_index;
   702	
 > 703		if (root->fs_info->sb->s_flags & MS_RDONLY)
   704			return -EROFS;
   705	
   706		key.objectid = root;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 21137 bytes --]

Re: [PATCH] Btrfs: disable online scrub repair on ro cases

[kbuild test robot]

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

Hi Liu,

[auto build test WARNING on btrfs/next]
[also build test WARNING on v4.4-rc3 next-20151203]

url:    https://github.com/0day-ci/linux/commits/Liu-Bo/Btrfs-disable-online-scrub-repair-on-ro-cases/20151204-205115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mason/linux-btrfs.git next
config: i386-randconfig-c0-12042053 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/uapi/linux/capability.h:16,
                    from include/linux/capability.h:15,
                    from include/linux/sched.h:15,
                    from include/linux/blkdev.h:4,
                    from fs/btrfs/scrub.c:19:
   fs/btrfs/scrub.c: In function 'scrub_fixup_readpage':
   fs/btrfs/scrub.c:703:10: error: invalid type argument of '->' (have 'u64 {aka long long unsigned int}')
     if (root->fs_info->sb->s_flags & MS_RDONLY)
             ^
   include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                               ^
>> fs/btrfs/scrub.c:703:2: note: in expansion of macro 'if'
     if (root->fs_info->sb->s_flags & MS_RDONLY)
     ^
   fs/btrfs/scrub.c:703:10: error: invalid type argument of '->' (have 'u64 {aka long long unsigned int}')
     if (root->fs_info->sb->s_flags & MS_RDONLY)
             ^
   include/linux/compiler.h:147:40: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                                           ^
>> fs/btrfs/scrub.c:703:2: note: in expansion of macro 'if'
     if (root->fs_info->sb->s_flags & MS_RDONLY)
     ^
   fs/btrfs/scrub.c:703:10: error: invalid type argument of '->' (have 'u64 {aka long long unsigned int}')
     if (root->fs_info->sb->s_flags & MS_RDONLY)
             ^
   include/linux/compiler.h:158:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^
>> fs/btrfs/scrub.c:703:2: note: in expansion of macro 'if'
     if (root->fs_info->sb->s_flags & MS_RDONLY)
     ^

vim +/if +703 fs/btrfs/scrub.c

   687	}
   688	
   689	static int scrub_fixup_readpage(u64 inum, u64 offset, u64 root, void *fixup_ctx)
   690	{
   691		struct page *page = NULL;
   692		unsigned long index;
   693		struct scrub_fixup_nodatasum *fixup = fixup_ctx;
   694		int ret;
   695		int corrected = 0;
   696		struct btrfs_key key;
   697		struct inode *inode = NULL;
   698		struct btrfs_fs_info *fs_info;
   699		u64 end = offset + PAGE_SIZE - 1;
   700		struct btrfs_root *local_root;
   701		int srcu_index;
   702	
 > 703		if (root->fs_info->sb->s_flags & MS_RDONLY)
   704			return -EROFS;
   705	
   706		key.objectid = root;
   707		key.type = BTRFS_ROOT_ITEM_KEY;
   708		key.offset = (u64)-1;
   709	
   710		fs_info = fixup->root->fs_info;
   711		srcu_index = srcu_read_lock(&fs_info->subvol_srcu);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 28068 bytes --]

[PATCH V2] Btrfs: disable online scrub repair on ro cases

[Liu Bo]

This disables repair process on ro cases as it can cause system
to be unresponsive on the ASSERT() in repair_io_failure().

This can happen when scrub is running and a hardware error pops up,
we should fallback to ro mounts gracefully instead of being unresponsive.

Reported-by: Codebird <codebird@birds-are-nice.me>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
v2: Get @fs_info from a real pointer instead of a confusing-name u64 root.

 fs/btrfs/scrub.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 2907a77..cb8a4e0 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -682,11 +682,14 @@ static int scrub_fixup_readpage(u64 inum, u64 offset, u64 root, void *fixup_ctx)
 	struct btrfs_root *local_root;
 	int srcu_index;
 
+	fs_info = fixup->root->fs_info;
+	if (fs_info->sb->s_flags & MS_RDONLY)
+		return -EROFS;
+
 	key.objectid = root;
 	key.type = BTRFS_ROOT_ITEM_KEY;
 	key.offset = (u64)-1;
 
-	fs_info = fixup->root->fs_info;
 	srcu_index = srcu_read_lock(&fs_info->subvol_srcu);
 
 	local_root = btrfs_read_fs_root_no_name(fs_info, &key);
-- 
2.5.0

Re: [PATCH V2] Btrfs: disable online scrub repair on ro cases

[David Sterba]

On Fri, Dec 04, 2015 at 09:58:04AM -0800, Liu Bo wrote:
> This disables repair process on ro cases as it can cause system
> to be unresponsive on the ASSERT() in repair_io_failure().
> 
> This can happen when scrub is running and a hardware error pops up,
> we should fallback to ro mounts gracefully instead of being unresponsive.

So this will also report the error as uncorrectable. This might be a bit
misleading, if a device error happens first and then some potentially
corectable errors are detected. This could be accounted as 'unverified'
error, that has closet maning.

Re: [PATCH V2] Btrfs: disable online scrub repair on ro cases

[Liu Bo]

On Mon, Dec 07, 2015 at 03:37:43PM +0100, David Sterba wrote:
> On Fri, Dec 04, 2015 at 09:58:04AM -0800, Liu Bo wrote:
> > This disables repair process on ro cases as it can cause system
> > to be unresponsive on the ASSERT() in repair_io_failure().
> > 
> > This can happen when scrub is running and a hardware error pops up,
> > we should fallback to ro mounts gracefully instead of being unresponsive.
> 
> So this will also report the error as uncorrectable. This might be a bit
> misleading, if a device error happens first and then some potentially
> corectable errors are detected. This could be accounted as 'unverified'
> error, that has closet maning.

Make sense, we can do
if (ret < 0 && ret == -EROFS)
	spin_lock();
	unverified++;
	spin_unlock()

However, in scrub_fixup_nodatasum() all errors including ENOMEM of path
allocation and failure of trans are interpreted to 'uncorrectable', So I
wander it means this 'uncorrectable' is only valid in this scrub process?

Thanks,

-liubo

Re: [PATCH V2] Btrfs: disable online scrub repair on ro cases

[David Sterba]

On Mon, Dec 07, 2015 at 10:26:05AM -0800, Liu Bo wrote:
> On Mon, Dec 07, 2015 at 03:37:43PM +0100, David Sterba wrote:
> > On Fri, Dec 04, 2015 at 09:58:04AM -0800, Liu Bo wrote:
> > > This disables repair process on ro cases as it can cause system
> > > to be unresponsive on the ASSERT() in repair_io_failure().
> > > 
> > > This can happen when scrub is running and a hardware error pops up,
> > > we should fallback to ro mounts gracefully instead of being unresponsive.
> > 
> > So this will also report the error as uncorrectable. This might be a bit
> > misleading, if a device error happens first and then some potentially
> > corectable errors are detected. This could be accounted as 'unverified'
> > error, that has closet maning.
> 
> Make sense, we can do
> if (ret < 0 && ret == -EROFS)
> 	spin_lock();
> 	unverified++;
> 	spin_unlock()
> 
> However, in scrub_fixup_nodatasum() all errors including ENOMEM of path
> allocation and failure of trans are interpreted to 'uncorrectable', So I
> wander it means this 'uncorrectable' is only valid in this scrub process?

I'm not sure we have a proper definition of the various stats. My user
expectation is that 'uncorrectable' refers to permament errors, so we
should try to match the type of error everywhere.