Re: [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed

[Eryu Guan <guaneryu@gmail.com>]

On Mon, Oct 13, 2014 at 02:23:57PM +0800, Anand Jain wrote:
> 
> 
> comments below..
> 
> 
> On 10/13/14 12:42, Eryu Guan wrote:
> >device replace could fail due to another running scrub process or any
> >other errors btrfs_scrub_dev() may hit, but this failure doesn't get
> >returned to userspace.
> >
> >The following steps could reproduce this issue
> >
> >	mkfs -t btrfs -f /dev/sdb1 /dev/sdb2
> >	mount /dev/sdb1 /mnt/btrfs
> >	while true; do btrfs scrub start -B /mnt/btrfs >/dev/null 2>&1; done &
> >	btrfs replace start -Bf /dev/sdb2 /dev/sdb3 /mnt/btrfs
> >	# if this replace succeeded, do the following and repeat until
> >	# you see this log in dmesg
> >	# BTRFS: btrfs_scrub_dev(/dev/sdb2, 2, /dev/sdb3) failed -115
> >	#btrfs replace start -Bf /dev/sdb3 /dev/sdb2 /mnt/btrfs
> >
> >	# once you see the error log in dmesg, check return value of
> >	# replace
> >	echo $?
> >
> >Introduce a new dev replace result
> >
> >BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS
> >
> >to catch -EINPROGRESS explicitly and return other errors directly to
> >userspace.
> >
> >Signed-off-by: Eryu Guan <guaneryu@gmail.com>
> >---
> >
> >v2:
> >- set result to SCRUB_INPROGRESS if btrfs_scrub_dev returned -EINPROGRESS
> >   and return 0 as Miao Xie suggested
> >
> >  fs/btrfs/dev-replace.c     | 12 +++++++++---
> >  include/uapi/linux/btrfs.h |  1 +
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> >
> >diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> >index eea26e1..a141f8b 100644
> >--- a/fs/btrfs/dev-replace.c
> >+++ b/fs/btrfs/dev-replace.c
> >@@ -418,9 +418,15 @@ int btrfs_dev_replace_start(struct btrfs_root *root,
> >  			      &dev_replace->scrub_progress, 0, 1);
> >
> >  	ret = btrfs_dev_replace_finishing(root->fs_info, ret);
> >-	WARN_ON(ret);
> >+	/* don't warn if EINPROGRESS, someone else might be running scrub */
> >+	if (ret == -EINPROGRESS) {
> >+		args->result = BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS;
> >+		ret = 0;
> >+	} else {
> >+		WARN_ON(ret);
> >+	}
> 
> 
>  looks like was are trying to manage EINPROGRESS returned by

Yes, that's right.

>  btrfs_dev_replace_finishing(). In btrfs_dev_replace_finishing()
>  which specific func call is returning EINPROGRESS ? I didn't go
>  deep enough.

btrfs_dev_replace_finishing() will check the scrub_ret(the last
argument), and return scrub_ret if (!scrub_ret). It was returning 0
unconditionally before this patch.

btrfs_dev_replace_start@fs/btrfs/dev-replace.c
   416          ret = btrfs_scrub_dev(fs_info, src_device->devid, 0,
   417                                src_device->total_bytes,
   418                                &dev_replace->scrub_progress, 0, 1);
   419
   420          ret = btrfs_dev_replace_finishing(root->fs_info, ret);

and btrfs_dev_replace_finishing@fs/btrfs/dev-replace.c
   529          if (!scrub_ret) {
   530                  btrfs_dev_replace_update_device_in_mapping_tree(fs_info,
   531                                                                  src_device,
   532                                                                  tgt_device);
   533          } else {
......
   547                  return scrub_ret;
   548          }

> 
>  And how do we handle if replace is intervened by balance
>  instead of scrub ?

Based on my test, replace ioctl would return -ENOENT if balance is
running

ERROR: ioctl(DEV_REPLACE_START) failed on "/mnt/testarea/scratch": No such file or directory, no error

(I haven't gone through this codepath yet and don't know where -ENOENT
comes from, but I don't think it's a proper errno,
/mnt/testarea/scratch is definitely there)
> 
>  sorry if I missed something.
> 
> Anand

Thanks for the review!

Eryu
> 
> 
> >-	return 0;
> >+	return ret;
> >
> >  leave:
> >  	dev_replace->srcdev = NULL;
> >@@ -538,7 +544,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
> >  			btrfs_destroy_dev_replace_tgtdev(fs_info, tgt_device);
> >  		mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
> >
> >-		return 0;
> >+		return scrub_ret;
> >  	}
> >
> >  	printk_in_rcu(KERN_INFO
> >diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> >index 2f47824..611e1c5 100644
> >--- a/include/uapi/linux/btrfs.h
> >+++ b/include/uapi/linux/btrfs.h
> >@@ -157,6 +157,7 @@ struct btrfs_ioctl_dev_replace_status_params {
> >  #define BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR			0
> >  #define BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED		1
> >  #define BTRFS_IOCTL_DEV_REPLACE_RESULT_ALREADY_STARTED		2
> >+#define BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS		3
> >  struct btrfs_ioctl_dev_replace_args {
> >  	__u64 cmd;	/* in */
> >  	__u64 result;	/* out */
> >