diff mbox

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

Message ID 1413175333-26095-1-git-send-email-guaneryu@gmail.com (mailing list archive)
State Accepted
Headers show

Commit Message

Eryu Guan Oct. 13, 2014, 4:42 a.m. UTC
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(-)

Comments

Anand Jain Oct. 13, 2014, 6:23 a.m. UTC | #1
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
  btrfs_dev_replace_finishing(). In btrfs_dev_replace_finishing()
  which specific func call is returning EINPROGRESS ? I didn't go
  deep enough.

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

  sorry if I missed something.

Anand


> -	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 */
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eryu Guan Oct. 13, 2014, 6:59 a.m. UTC | #2
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 */
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Anand Jain Oct. 13, 2014, 10:18 a.m. UTC | #3
On 10/13/14 14:59, Eryu Guan wrote:
> 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);
>>> +	}


  I am bit concerned, why these racing threads here aren't excluding
  each other using "mutually_exclusive_operation_running" ? as most
  of the other device operation thread does.

Thanks, Anand

>>   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 */
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eryu Guan Oct. 14, 2014, 3:35 a.m. UTC | #4
On Mon, Oct 13, 2014 at 06:18:04PM +0800, Anand Jain wrote:
> 
> 
> On 10/13/14 14:59, Eryu Guan wrote:
> >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);
> >>>+	}
> 
> 
>  I am bit concerned, why these racing threads here aren't excluding
>  each other using "mutually_exclusive_operation_running" ? as most
>  of the other device operation thread does.
> 
> Thanks, Anand

btrfs_ioctl_scrub() doesn't use mutually_exclusive_operation_running
as other device operations do, I'm not sure if it should(seems scrub
should do it too to me).

But I think that's a different problem from the one I'm trying to fix
here. The main purpose is to return error to userspace when
btrfs_scrub_dev() hit some error. Dealing with -EINPROGRESS is to
match the current behavior(replace and scrub could run at the same
time).

Thanks,
Eryu
> 
> >>  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 */
> >>>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);
+	}
 
-	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 */