* [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed @ 2014-10-13 4:42 Eryu Guan 2014-10-13 4:42 ` [PATCH] btrfs-progs: add new dev replace result Eryu Guan 2014-10-13 6:23 ` [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed Anand Jain 0 siblings, 2 replies; 6+ messages in thread From: Eryu Guan @ 2014-10-13 4:42 UTC (permalink / raw) To: linux-btrfs; +Cc: Eryu Guan 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); + } - 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 */ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] btrfs-progs: add new dev replace result 2014-10-13 4:42 [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed Eryu Guan @ 2014-10-13 4:42 ` Eryu Guan 2014-10-13 6:23 ` [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed Anand Jain 1 sibling, 0 replies; 6+ messages in thread From: Eryu Guan @ 2014-10-13 4:42 UTC (permalink / raw) To: linux-btrfs; +Cc: Eryu Guan A new dev replace result was introduced by kernel commit Btrfs: return failure if btrfs_dev_replace_finishing() failed Make the userspace know about the new result too. Signed-off-by: Eryu Guan <guaneryu@gmail.com> --- cmds-replace.c | 2 ++ ioctl.h | 1 + 2 files changed, 3 insertions(+) diff --git a/cmds-replace.c b/cmds-replace.c index 9fe7ad8..7a45cef 100644 --- a/cmds-replace.c +++ b/cmds-replace.c @@ -53,6 +53,8 @@ static const char *replace_dev_result2string(__u64 result) return "not started"; case BTRFS_IOCTL_DEV_REPLACE_RESULT_ALREADY_STARTED: return "already started"; + case BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS: + return "scrub is in progress"; default: return "<illegal result value>"; } diff --git a/ioctl.h b/ioctl.h index f0fc060..0e02fae 100644 --- a/ioctl.h +++ b/ioctl.h @@ -144,6 +144,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 */ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed 2014-10-13 4:42 [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed Eryu Guan 2014-10-13 4:42 ` [PATCH] btrfs-progs: add new dev replace result Eryu Guan @ 2014-10-13 6:23 ` Anand Jain 2014-10-13 6:59 ` Eryu Guan 1 sibling, 1 reply; 6+ messages in thread From: Anand Jain @ 2014-10-13 6:23 UTC (permalink / raw) To: Eryu Guan; +Cc: linux-btrfs 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 */ > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed 2014-10-13 6:23 ` [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed Anand Jain @ 2014-10-13 6:59 ` Eryu Guan 2014-10-13 10:18 ` Anand Jain 0 siblings, 1 reply; 6+ messages in thread From: Eryu Guan @ 2014-10-13 6:59 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs 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 */ > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed 2014-10-13 6:59 ` Eryu Guan @ 2014-10-13 10:18 ` Anand Jain 2014-10-14 3:35 ` Eryu Guan 0 siblings, 1 reply; 6+ messages in thread From: Anand Jain @ 2014-10-13 10:18 UTC (permalink / raw) To: Eryu Guan; +Cc: linux-btrfs 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 */ >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed 2014-10-13 10:18 ` Anand Jain @ 2014-10-14 3:35 ` Eryu Guan 0 siblings, 0 replies; 6+ messages in thread From: Eryu Guan @ 2014-10-14 3:35 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs 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 */ > >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-14 3:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-13 4:42 [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed Eryu Guan 2014-10-13 4:42 ` [PATCH] btrfs-progs: add new dev replace result Eryu Guan 2014-10-13 6:23 ` [PATCH v2] Btrfs: return failure if btrfs_dev_replace_finishing() failed Anand Jain 2014-10-13 6:59 ` Eryu Guan 2014-10-13 10:18 ` Anand Jain 2014-10-14 3:35 ` Eryu Guan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).