All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/9] fix replace-start and replace-cancel racing
@ 2018-11-07 11:43 Anand Jain
  2018-11-07 11:43 ` [PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static Anand Jain
                   ` (8 more replies)
  0 siblings, 9 replies; 24+ messages in thread
From: Anand Jain @ 2018-11-07 11:43 UTC (permalink / raw)
  To: linux-btrfs

Replace-start and replace-cancel threads can race to create a messy
situation leading to UAF. We use the scrub code to write
the blocks on the replace target. So if we haven't have set the
replace-scrub-running yet, without this patch we just ignore the error
and free the target device. When this happens the system panics with
UAF error.

Its nice to see that btrfs_dev_replace_finishing() already handles
the ECANCELED (replace canceled) situation, but for an unknown reason
we aren't using it to cleanup the replace cancel situation, instead
we just let the replace cancel ioctl thread to cleanup the target
device and return and out of synchronous with the scrub code.

This patch 4/9, 5/9 and 6/9 uses the return code of btrfs_scrub_cancel()
to check if the scrub was really running. And if its not then shall
return an error to the user (replace not started error) so that user
can retry replace cancel. And uses btrfs_dev_replace_finishing() code
to cleanup after successful cancel of the replace scrub.

Further, a suspended replace, when tries to restart, and if it fails
(for example target device missing, or excl ops running) it goes to the
started state, and so the cli 'btrfs replace status /mnt' hangs with no
progress. So patches 2/9 and 3/9 fixes that.

As the originals code idea of ECANCELED was limited to the situation of
the error only and not user requested, there are unnecessary error log
and warn log which 7/9 and 8/9 patches fixes.

Patches 1/9 and 9/9 are good to have fixes. Makes a function static and
code readability good.

Testing: (I did some attempt to convert these into xfstests but need a
mechanism where kernel thread can wait for user land script. I thought
I could do it using ebfp, but needs more digging on how).
As of now hand tested with using procfs to hold kernel thread at
(wait_for_user(..)) until user land issues go.


1.
umount /btrfs; wipefs -a /dev/sd[b-f] && mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs && fillfs /btrfs 10000
btrfs replace start /dev/sdb /dev/sdc /btrfs
  wait_for_user("scrub running is set..waiting"); AND OR
  wait_for_user("scrub running is NOT set..waiting");
btrfs replace cancel /btrfs
  wait_for_user_go();
btrfs replace status /btrfs

2.
umount /btrfs; wipefs -a /dev/sd[b-f] && mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs && fillfs /btrfs 10000
btrfs replace start /dev/sdb /dev/sdc /btrfs
  wait_for_user("scrub running is set..waiting"); AND OR
  wait_for_user("scrub running is NOT set..waiting");
reboot
mount -o device=/dev/sdc /dev/sdb /btrfs
  wait_for_user_go();
btrfs replace status /btrfs
btrfs replace cancel /btrfs
btrfs replace status /btrfs

3.
umount /btrfs; wipefs -a /dev/sd[b-f] && mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs && fillfs /btrfs 10000
btrfs replace start /dev/sdb /dev/sdc /btrfs
  wait_for_user("scrub running is set..waiting"); AND OR
  wait_for_user("scrub running is NOT set..waiting");
reboot
mount -o degraded /dev/sdb /btrfs
btrfs replace status /btrfs
btrfs replace cancel /btrfs
btrfs replace status /btrfs
umount /btrfs
mount /dev/sdb /btrfs


Anand Jain (9):
  btrfs: mark btrfs_dev_replace_start() as static
  btrfs: replace go back to suspended if target missing
  btrfs: replace back to suspend state if EXCL OP is running
  btrfs: fix UAF due to race between replace start and cancel
  btrfs: replace cancel is successful if scrub cancel is successful
  btrfs: replace's scrub must not be running in replace suspended state
  btrfs: quiten warn if the replace is canceled at finish
  btrfs: user requsted replace cancel is not an error
  btrfs: add explicit check for replace result no error

 fs/btrfs/dev-replace.c | 90 ++++++++++++++++++++++++++++++++++----------------
 fs/btrfs/dev-replace.h |  3 --
 2 files changed, 62 insertions(+), 31 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static
  2018-11-07 11:43 [PATCH 0/9] fix replace-start and replace-cancel racing Anand Jain
@ 2018-11-07 11:43 ` Anand Jain
  2018-11-07 12:04   ` Nikolay Borisov
  2018-11-07 11:43 ` [PATCH 2/9] btrfs: replace go back to suspended if target missing Anand Jain
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 24+ messages in thread
From: Anand Jain @ 2018-11-07 11:43 UTC (permalink / raw)
  To: linux-btrfs

There isn't any other consumer other than in its own file dev-replace.c.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 2 +-
 fs/btrfs/dev-replace.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 2aa48aecc52b..59991165e126 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -390,7 +390,7 @@ static char* btrfs_dev_name(struct btrfs_device *device)
 		return rcu_str_deref(device->name);
 }
 
-int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
+static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 		const char *tgtdev_name, u64 srcdevid, const char *srcdev_name,
 		int read_src)
 {
diff --git a/fs/btrfs/dev-replace.h b/fs/btrfs/dev-replace.h
index 795c551f5b5e..27e3bb0cca11 100644
--- a/fs/btrfs/dev-replace.h
+++ b/fs/btrfs/dev-replace.h
@@ -13,9 +13,6 @@ int btrfs_run_dev_replace(struct btrfs_trans_handle *trans,
 			  struct btrfs_fs_info *fs_info);
 int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
 			    struct btrfs_ioctl_dev_replace_args *args);
-int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
-		const char *tgtdev_name, u64 srcdevid, const char *srcdev_name,
-		int read_src);
 void btrfs_dev_replace_status(struct btrfs_fs_info *fs_info,
 			      struct btrfs_ioctl_dev_replace_args *args);
 int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/9] btrfs: replace go back to suspended if target missing
  2018-11-07 11:43 [PATCH 0/9] fix replace-start and replace-cancel racing Anand Jain
  2018-11-07 11:43 ` [PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static Anand Jain
@ 2018-11-07 11:43 ` Anand Jain
  2018-11-07 12:35   ` Nikolay Borisov
  2018-11-07 11:43 ` [PATCH 3/9] btrfs: replace back to suspend state if EXCL OP is running Anand Jain
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 24+ messages in thread
From: Anand Jain @ 2018-11-07 11:43 UTC (permalink / raw)
  To: linux-btrfs

At the time of forced unmount we place the running replace to
BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state, so when the system comes
back and suppose the target device is missing, then let the replace
state continue to be in BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state
instead of BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED as there isn't any
matching scrub running as part of replace.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 59991165e126..47d6768a9cde 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -884,6 +884,9 @@ int btrfs_resume_dev_replace_async(struct btrfs_fs_info *fs_info)
 			   "cannot continue dev_replace, tgtdev is missing");
 		btrfs_info(fs_info,
 			   "you may cancel the operation after 'mount -o degraded'");
+		dev_replace->replace_state =
+					BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED;
+		dev_replace->item_needs_writeback = 1;
 		btrfs_dev_replace_write_unlock(dev_replace);
 		return 0;
 	}
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 3/9] btrfs: replace back to suspend state if EXCL OP is running
  2018-11-07 11:43 [PATCH 0/9] fix replace-start and replace-cancel racing Anand Jain
  2018-11-07 11:43 ` [PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static Anand Jain
  2018-11-07 11:43 ` [PATCH 2/9] btrfs: replace go back to suspended if target missing Anand Jain
@ 2018-11-07 11:43 ` Anand Jain
  2018-11-07 11:43 ` [PATCH 4/9] btrfs: fix UAF due to race between replace start and cancel Anand Jain
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2018-11-07 11:43 UTC (permalink / raw)
  To: linux-btrfs

In a secnario where balance and replace co-exists as below,
  start balance; pause balance; start replace; reboot
and when system restarts balance restarts first and the
replace restart will fail as EXCL OP lock is already held by
the balance. If so place the replace state back to
BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 47d6768a9cde..e001c2418940 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -898,6 +898,11 @@ int btrfs_resume_dev_replace_async(struct btrfs_fs_info *fs_info)
 	 * dev-replace to start anyway.
 	 */
 	if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags)) {
+		btrfs_dev_replace_write_lock(dev_replace);
+		dev_replace->replace_state =
+					BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED;
+		dev_replace->item_needs_writeback = 1;
+		btrfs_dev_replace_write_unlock(dev_replace);
 		btrfs_info(fs_info,
 		"cannot resume dev-replace, other exclusive operation running");
 		return 0;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 4/9] btrfs: fix UAF due to race between replace start and cancel
  2018-11-07 11:43 [PATCH 0/9] fix replace-start and replace-cancel racing Anand Jain
                   ` (2 preceding siblings ...)
  2018-11-07 11:43 ` [PATCH 3/9] btrfs: replace back to suspend state if EXCL OP is running Anand Jain
@ 2018-11-07 11:43 ` Anand Jain
  2018-11-07 11:43 ` [PATCH 5/9] btrfs: replace cancel is successful if scrub cancel is successful Anand Jain
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2018-11-07 11:43 UTC (permalink / raw)
  To: linux-btrfs

replace cancel thread can race with the replace start thread and if
fs_info::scrubs_running is not yet set the btrfs_scrub_cancel() will fail
to stop the scrub thread, so the scrub thread continue with the scrub for
replace which then shall try to write to the target device and which is
already freed by the cancel thread. Please ref to the logs below.

So scrub_setup_ctx() warns as tgtdev is null.

static noinline_for_stack
struct scrub_ctx *scrub_setup_ctx(struct btrfs_device *dev, int
is_dev_replace)
{
::
        if (is_dev_replace) {
                WARN_ON(!fs_info->dev_replace.tgtdev);  <===
                sctx->pages_per_wr_bio = SCRUB_PAGES_PER_WR_BIO;
                sctx->wr_tgtdev = fs_info->dev_replace.tgtdev;
                sctx->flush_all_writes = false;
        }

[ 6724.497655] BTRFS info (device sdb): dev_replace from /dev/sdb (devid 1) to /dev/sdc started
[ 6753.945017] BTRFS info (device sdb): dev_replace from /dev/sdb (devid 1) to /dev/sdc canceled
[ 6852.426700] WARNING: CPU: 0 PID: 4494 at fs/btrfs/scrub.c:622 scrub_setup_ctx.isra.19+0x220/0x230 [btrfs]
::
[ 6852.428928] RIP: 0010:scrub_setup_ctx.isra.19+0x220/0x230 [btrfs]
::
[ 6852.432970] Call Trace:
[ 6852.433202]  btrfs_scrub_dev+0x19b/0x5c0 [btrfs]
[ 6852.433471]  btrfs_dev_replace_start+0x48c/0x6a0 [btrfs]
[ 6852.433800]  btrfs_dev_replace_by_ioctl+0x3a/0x60 [btrfs]
[ 6852.434097]  btrfs_ioctl+0x2476/0x2d20 [btrfs]
[ 6852.434365]  ? do_sigaction+0x7d/0x1e0
[ 6852.434623]  do_vfs_ioctl+0xa9/0x6c0
[ 6852.434865]  ? syscall_trace_enter+0x1c8/0x310
[ 6852.435124]  ? syscall_trace_enter+0x1c8/0x310
[ 6852.435387]  ksys_ioctl+0x60/0x90
[ 6852.435663]  __x64_sys_ioctl+0x16/0x20
[ 6852.435907]  do_syscall_64+0x50/0x180
[ 6852.436150]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Further, as the replace thread enters the
scrub_write_page_to_dev_replace() without the target device it panics

static int scrub_add_page_to_wr_bio(struct scrub_ctx *sctx,
                                    struct scrub_page *spage)
{
::
      bio_set_dev(bio, sbio->dev->bdev); <======

[ 6929.715145] BUG: unable to handle kernel NULL pointer dereference at 00000000000000a0
::
[ 6929.717106] Workqueue: btrfs-scrub btrfs_scrub_helper [btrfs]
[ 6929.717420] RIP: 0010:scrub_write_page_to_dev_replace+0xb4/0x260
[btrfs]
::
[ 6929.721430] Call Trace:
[ 6929.721663]  scrub_write_block_to_dev_replace+0x3f/0x60 [btrfs]
[ 6929.721975]  scrub_bio_end_io_worker+0x1af/0x490 [btrfs]
[ 6929.722277]  normal_work_helper+0xf0/0x4c0 [btrfs]
[ 6929.722552]  process_one_work+0x1f4/0x520
[ 6929.722805]  ? process_one_work+0x16e/0x520
[ 6929.723063]  worker_thread+0x46/0x3d0
[ 6929.723313]  kthread+0xf8/0x130
[ 6929.723544]  ? process_one_work+0x520/0x520
[ 6929.723800]  ? kthread_delayed_work_timer_fn+0x80/0x80
[ 6929.724081]  ret_from_fork+0x3a/0x50

Fix this by letting the btrfs_dev_replace_finishing() to do the job of
cleaning after the cancel, including freeing of the target device.
btrfs_dev_replace_finishing() is called when btrfs_scub_dev() returns
along with the scrub return status.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 61 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index e001c2418940..90c124edec76 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -797,39 +797,56 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
 		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED;
 		btrfs_dev_replace_write_unlock(dev_replace);
-		goto leave;
+		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
+		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
+		tgt_device = dev_replace->tgtdev;
+		src_device = dev_replace->srcdev;
+		btrfs_dev_replace_write_unlock(dev_replace);
+		btrfs_scrub_cancel(fs_info);
+		/*
+		 * btrfs_dev_replace_finishing() will handle the cleanup part
+		 */
+		btrfs_info_in_rcu(fs_info,
+			"dev_replace from %s (devid %llu) to %s canceled",
+			btrfs_dev_name(src_device), src_device->devid,
+			btrfs_dev_name(tgt_device));
+		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
+		/*
+		 * scrub doing the replace isn't running so do the cleanup here
+		 */
 		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
 		tgt_device = dev_replace->tgtdev;
 		src_device = dev_replace->srcdev;
 		dev_replace->tgtdev = NULL;
 		dev_replace->srcdev = NULL;
-		break;
-	}
-	dev_replace->replace_state = BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED;
-	dev_replace->time_stopped = ktime_get_real_seconds();
-	dev_replace->item_needs_writeback = 1;
-	btrfs_dev_replace_write_unlock(dev_replace);
-	btrfs_scrub_cancel(fs_info);
+		dev_replace->replace_state = BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED;
+		dev_replace->time_stopped = ktime_get_real_seconds();
+		dev_replace->item_needs_writeback = 1;
 
-	trans = btrfs_start_transaction(root, 0);
-	if (IS_ERR(trans)) {
-		mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
-		return PTR_ERR(trans);
-	}
-	ret = btrfs_commit_transaction(trans);
-	WARN_ON(ret);
+		btrfs_dev_replace_write_unlock(dev_replace);
 
-	btrfs_info_in_rcu(fs_info,
-		"dev_replace from %s (devid %llu) to %s canceled",
-		btrfs_dev_name(src_device), src_device->devid,
-		btrfs_dev_name(tgt_device));
+		btrfs_scrub_cancel(fs_info);
+
+		trans = btrfs_start_transaction(root, 0);
+		if (IS_ERR(trans)) {
+			mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
+			return PTR_ERR(trans);
+		}
+		ret = btrfs_commit_transaction(trans);
+		WARN_ON(ret);
 
-	if (tgt_device)
-		btrfs_destroy_dev_replace_tgtdev(tgt_device);
+		btrfs_info_in_rcu(fs_info,
+		"suspended dev_replace from %s (devid %llu) to %s canceled",
+			btrfs_dev_name(src_device), src_device->devid,
+			btrfs_dev_name(tgt_device));
+
+		if (tgt_device)
+			btrfs_destroy_dev_replace_tgtdev(tgt_device);
+		break;
+	}
 
-leave:
 	mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
 	return result;
 }
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 5/9] btrfs: replace cancel is successful if scrub cancel is successful
  2018-11-07 11:43 [PATCH 0/9] fix replace-start and replace-cancel racing Anand Jain
                   ` (3 preceding siblings ...)
  2018-11-07 11:43 ` [PATCH 4/9] btrfs: fix UAF due to race between replace start and cancel Anand Jain
@ 2018-11-07 11:43 ` Anand Jain
  2018-11-07 12:25   ` Nikolay Borisov
  2018-11-07 11:43 ` [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state Anand Jain
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 24+ messages in thread
From: Anand Jain @ 2018-11-07 11:43 UTC (permalink / raw)
  To: linux-btrfs

In btrfs_dev_replace_cancel() we should check if the
btrfs_scrub_cancel() is successful. If the btrfs_scrub_cancel() fails, return
BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED so that user can try to
cancel the replace again.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 90c124edec76..c092ed559714 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -799,18 +799,22 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
 		btrfs_dev_replace_write_unlock(dev_replace);
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
-		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
 		tgt_device = dev_replace->tgtdev;
 		src_device = dev_replace->srcdev;
 		btrfs_dev_replace_write_unlock(dev_replace);
-		btrfs_scrub_cancel(fs_info);
-		/*
-		 * btrfs_dev_replace_finishing() will handle the cleanup part
-		 */
-		btrfs_info_in_rcu(fs_info,
-			"dev_replace from %s (devid %llu) to %s canceled",
-			btrfs_dev_name(src_device), src_device->devid,
-			btrfs_dev_name(tgt_device));
+		ret = btrfs_scrub_cancel(fs_info);
+		if (ret) {
+			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED;
+		} else {
+			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
+			/*
+			 * btrfs_dev_replace_finishing() will handle the cleanup part
+			 */
+			btrfs_info_in_rcu(fs_info,
+				"dev_replace from %s (devid %llu) to %s canceled",
+				btrfs_dev_name(src_device), src_device->devid,
+				btrfs_dev_name(tgt_device));
+		}
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
 		/*
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state
  2018-11-07 11:43 [PATCH 0/9] fix replace-start and replace-cancel racing Anand Jain
                   ` (4 preceding siblings ...)
  2018-11-07 11:43 ` [PATCH 5/9] btrfs: replace cancel is successful if scrub cancel is successful Anand Jain
@ 2018-11-07 11:43 ` Anand Jain
  2018-11-07 12:19   ` Nikolay Borisov
  2018-11-07 11:43 ` [PATCH 7/9] btrfs: quiten warn if the replace is canceled at finish Anand Jain
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 24+ messages in thread
From: Anand Jain @ 2018-11-07 11:43 UTC (permalink / raw)
  To: linux-btrfs

When the replace state is placed in the suspended state,
btrfs_scrub_cancel() should fail with -ENOTCONN as there is no
scrub running, as a safety catch check if btrfs_scrub_cancel()
returns -ENOTCONN and assert if it doesn't.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index c092ed559714..dae2b920f1a9 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -831,7 +831,9 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
 
 		btrfs_dev_replace_write_unlock(dev_replace);
 
-		btrfs_scrub_cancel(fs_info);
+		/* scrub for replace must not be running in suspended state */
+		if (btrfs_scrub_cancel(fs_info) != -ENOTCONN)
+			ASSERT(0);
 
 		trans = btrfs_start_transaction(root, 0);
 		if (IS_ERR(trans)) {
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 7/9] btrfs: quiten warn if the replace is canceled at finish
  2018-11-07 11:43 [PATCH 0/9] fix replace-start and replace-cancel racing Anand Jain
                   ` (5 preceding siblings ...)
  2018-11-07 11:43 ` [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state Anand Jain
@ 2018-11-07 11:43 ` Anand Jain
  2018-11-07 12:17   ` Nikolay Borisov
  2018-11-07 11:43 ` [PATCH 8/9] btrfs: user requsted replace cancel is not an error Anand Jain
  2018-11-07 11:43 ` [PATCH 9/9] btrfs: add explicit check for replace result no error Anand Jain
  8 siblings, 1 reply; 24+ messages in thread
From: Anand Jain @ 2018-11-07 11:43 UTC (permalink / raw)
  To: linux-btrfs

When we successfully cancel the replace its scrub returns -ECANCELED,
which then passed to btrfs_dev_replace_finishing(), it cleans up based
on the scrub returned status and propagates the same -ECANCELED back
the parent function. So skip the -ECANCELED error to log the WARN.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index dae2b920f1a9..c14c41b70287 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -497,7 +497,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 	ret = btrfs_dev_replace_finishing(fs_info, ret);
 	if (ret == -EINPROGRESS) {
 		ret = BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS;
-	} else {
+	} else if (ret != -ECANCELED) {
 		WARN_ON(ret);
 	}
 
@@ -956,7 +956,8 @@ static int btrfs_dev_replace_kthread(void *data)
 			      btrfs_device_get_total_bytes(dev_replace->srcdev),
 			      &dev_replace->scrub_progress, 0, 1);
 	ret = btrfs_dev_replace_finishing(fs_info, ret);
-	WARN_ON(ret);
+	if (ret != -ECANCELED)
+		WARN_ON(ret);
 
 	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
 	return 0;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 8/9] btrfs: user requsted replace cancel is not an error
  2018-11-07 11:43 [PATCH 0/9] fix replace-start and replace-cancel racing Anand Jain
                   ` (6 preceding siblings ...)
  2018-11-07 11:43 ` [PATCH 7/9] btrfs: quiten warn if the replace is canceled at finish Anand Jain
@ 2018-11-07 11:43 ` Anand Jain
  2018-11-07 11:43 ` [PATCH 9/9] btrfs: add explicit check for replace result no error Anand Jain
  8 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2018-11-07 11:43 UTC (permalink / raw)
  To: linux-btrfs

As of now only user requested replace cancel can cancel the replace-scrub
so no need to log error for it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index c14c41b70287..cf3554554616 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -622,7 +622,8 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 								src_device,
 								tgt_device);
 	} else {
-		btrfs_err_in_rcu(fs_info,
+		if (scrub_ret != -ECANCELED)
+			btrfs_err_in_rcu(fs_info,
 				 "btrfs_scrub_dev(%s, %llu, %s) failed %d",
 				 btrfs_dev_name(src_device),
 				 src_device->devid,
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 9/9] btrfs: add explicit check for replace result no error
  2018-11-07 11:43 [PATCH 0/9] fix replace-start and replace-cancel racing Anand Jain
                   ` (7 preceding siblings ...)
  2018-11-07 11:43 ` [PATCH 8/9] btrfs: user requsted replace cancel is not an error Anand Jain
@ 2018-11-07 11:43 ` Anand Jain
  2018-11-07 12:15   ` Nikolay Borisov
  8 siblings, 1 reply; 24+ messages in thread
From: Anand Jain @ 2018-11-07 11:43 UTC (permalink / raw)
  To: linux-btrfs

We recast the replace return status
BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS to 0, to indicate no
error.
And since BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR should also return 0,
which is also declared as 0, so we just return. Instead add it to the if
statement so that there is enough clarity while reading the code.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index cf3554554616..ca44998189c7 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -533,8 +533,9 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
 					args->start.cont_reading_from_srcdev_mode);
 	args->result = ret;
 	/* don't warn if EINPROGRESS, someone else might be running scrub */
-	if (ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS)
-		ret = 0;
+	if (ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS ||
+	    ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR)
+		return 0;
 
 	return ret;
 }
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static
  2018-11-07 11:43 ` [PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static Anand Jain
@ 2018-11-07 12:04   ` Nikolay Borisov
  0 siblings, 0 replies; 24+ messages in thread
From: Nikolay Borisov @ 2018-11-07 12:04 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs



On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> There isn't any other consumer other than in its own file dev-replace.c.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/dev-replace.c | 2 +-
>  fs/btrfs/dev-replace.h | 3 ---
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 2aa48aecc52b..59991165e126 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -390,7 +390,7 @@ static char* btrfs_dev_name(struct btrfs_device *device)
>  		return rcu_str_deref(device->name);
>  }
>  
> -int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
> +static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
>  		const char *tgtdev_name, u64 srcdevid, const char *srcdev_name,
>  		int read_src)
>  {
> diff --git a/fs/btrfs/dev-replace.h b/fs/btrfs/dev-replace.h
> index 795c551f5b5e..27e3bb0cca11 100644
> --- a/fs/btrfs/dev-replace.h
> +++ b/fs/btrfs/dev-replace.h
> @@ -13,9 +13,6 @@ int btrfs_run_dev_replace(struct btrfs_trans_handle *trans,
>  			  struct btrfs_fs_info *fs_info);
>  int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
>  			    struct btrfs_ioctl_dev_replace_args *args);
> -int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
> -		const char *tgtdev_name, u64 srcdevid, const char *srcdev_name,
> -		int read_src);
>  void btrfs_dev_replace_status(struct btrfs_fs_info *fs_info,
>  			      struct btrfs_ioctl_dev_replace_args *args);
>  int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info);
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 9/9] btrfs: add explicit check for replace result no error
  2018-11-07 11:43 ` [PATCH 9/9] btrfs: add explicit check for replace result no error Anand Jain
@ 2018-11-07 12:15   ` Nikolay Borisov
  2018-11-08  7:26     ` Anand Jain
  0 siblings, 1 reply; 24+ messages in thread
From: Nikolay Borisov @ 2018-11-07 12:15 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs



On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> We recast the replace return status
> BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS to 0, to indicate no
> error.
> And since BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR should also return 0,
> which is also declared as 0, so we just return. Instead add it to the if
> statement so that there is enough clarity while reading the code.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/dev-replace.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index cf3554554616..ca44998189c7 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -533,8 +533,9 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
>  					args->start.cont_reading_from_srcdev_mode);
>  	args->result = ret;
>  	/* don't warn if EINPROGRESS, someone else might be running scrub */
> -	if (ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS)
> -		ret = 0;
> +	if (ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS ||
> +	    ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR)

BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR can only be returned from
btrfs_dev_replace_cancel, so what you are doing with checking ret ==
BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR is redundant. So using
BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR is at the very least misleading
here.

I suggest you drop this patch.

> +		return 0;
>  
>  	return ret;
>  }
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 7/9] btrfs: quiten warn if the replace is canceled at finish
  2018-11-07 11:43 ` [PATCH 7/9] btrfs: quiten warn if the replace is canceled at finish Anand Jain
@ 2018-11-07 12:17   ` Nikolay Borisov
  2018-11-08  8:06     ` Anand Jain
  0 siblings, 1 reply; 24+ messages in thread
From: Nikolay Borisov @ 2018-11-07 12:17 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs



On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> -	WARN_ON(ret);
> +	if (ret != -ECANCELED)
> +		WARN_ON(ret);

WARN_ON(ret && ret != -ECANCELED)

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state
  2018-11-07 11:43 ` [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state Anand Jain
@ 2018-11-07 12:19   ` Nikolay Borisov
  2018-11-08  8:33     ` Anand Jain
  0 siblings, 1 reply; 24+ messages in thread
From: Nikolay Borisov @ 2018-11-07 12:19 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs



On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> +		/* scrub for replace must not be running in suspended state */
> +		if (btrfs_scrub_cancel(fs_info) != -ENOTCONN)
> +			ASSERT(0);

ASSERT(btrfs_scrub_cancel(fs_info) == -ENOTCONN)

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 5/9] btrfs: replace cancel is successful if scrub cancel is successful
  2018-11-07 11:43 ` [PATCH 5/9] btrfs: replace cancel is successful if scrub cancel is successful Anand Jain
@ 2018-11-07 12:25   ` Nikolay Borisov
  2018-11-07 12:26     ` Nikolay Borisov
  0 siblings, 1 reply; 24+ messages in thread
From: Nikolay Borisov @ 2018-11-07 12:25 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs



On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> In btrfs_dev_replace_cancel() we should check if the
> btrfs_scrub_cancel() is successful. If the btrfs_scrub_cancel() fails, return
> BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED so that user can try to
> cancel the replace again.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/dev-replace.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 90c124edec76..c092ed559714 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -799,18 +799,22 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
>  		btrfs_dev_replace_write_unlock(dev_replace);
>  		break;
>  	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
> -		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
>  		tgt_device = dev_replace->tgtdev;
>  		src_device = dev_replace->srcdev;
>  		btrfs_dev_replace_write_unlock(dev_replace);
> -		btrfs_scrub_cancel(fs_info);
> -		/*
> -		 * btrfs_dev_replace_finishing() will handle the cleanup part
> -		 */
> -		btrfs_info_in_rcu(fs_info,
> -			"dev_replace from %s (devid %llu) to %s canceled",
> -			btrfs_dev_name(src_device), src_device->devid,
> -			btrfs_dev_name(tgt_device));
> +		ret = btrfs_scrub_cancel(fs_info);
> +		if (ret) {
> +			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED;
> +		} else {
> +			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
> +			/*
> +			 * btrfs_dev_replace_finishing() will handle the cleanup part
> +			 */
> +			btrfs_info_in_rcu(fs_info,
> +				"dev_replace from %s (devid %llu) to %s canceled",
> +				btrfs_dev_name(src_device), src_device->devid,
> +				btrfs_dev_name(tgt_device));

This is identical to the btrfs_info_in_rcu several lines further down.
So if btrfs_scrub_cancel is successful you will print this messages
twice. Furthermore, there is already an unconditinal call to
btrfs_scrub_cancel. You are duplicating this function, this definitely
needs another look...

> +		}
>  		break;
>  	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
>  		/*
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 5/9] btrfs: replace cancel is successful if scrub cancel is successful
  2018-11-07 12:25   ` Nikolay Borisov
@ 2018-11-07 12:26     ` Nikolay Borisov
  0 siblings, 0 replies; 24+ messages in thread
From: Nikolay Borisov @ 2018-11-07 12:26 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs



On 7.11.18 г. 14:25 ч., Nikolay Borisov wrote:
> 
> 
> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>> In btrfs_dev_replace_cancel() we should check if the
>> btrfs_scrub_cancel() is successful. If the btrfs_scrub_cancel() fails, return
>> BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED so that user can try to
>> cancel the replace again.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>  fs/btrfs/dev-replace.c | 22 +++++++++++++---------
>>  1 file changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>> index 90c124edec76..c092ed559714 100644
>> --- a/fs/btrfs/dev-replace.c
>> +++ b/fs/btrfs/dev-replace.c
>> @@ -799,18 +799,22 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
>>  		btrfs_dev_replace_write_unlock(dev_replace);
>>  		break;
>>  	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
>> -		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
>>  		tgt_device = dev_replace->tgtdev;
>>  		src_device = dev_replace->srcdev;
>>  		btrfs_dev_replace_write_unlock(dev_replace);
>> -		btrfs_scrub_cancel(fs_info);
>> -		/*
>> -		 * btrfs_dev_replace_finishing() will handle the cleanup part
>> -		 */
>> -		btrfs_info_in_rcu(fs_info,
>> -			"dev_replace from %s (devid %llu) to %s canceled",
>> -			btrfs_dev_name(src_device), src_device->devid,
>> -			btrfs_dev_name(tgt_device));
>> +		ret = btrfs_scrub_cancel(fs_info);
>> +		if (ret) {
>> +			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED;
>> +		} else {
>> +			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
>> +			/*
>> +			 * btrfs_dev_replace_finishing() will handle the cleanup part
>> +			 */
>> +			btrfs_info_in_rcu(fs_info,
>> +				"dev_replace from %s (devid %llu) to %s canceled",
>> +				btrfs_dev_name(src_device), src_device->devid,
>> +				btrfs_dev_name(tgt_device));
> 
> This is identical to the btrfs_info_in_rcu several lines further down.
> So if btrfs_scrub_cancel is successful you will print this messages
> twice. Furthermore, there is already an unconditinal call to
> btrfs_scrub_cancel. You are duplicating this function, this definitely
> needs another look...

Ah, it builds on top of the previous patch which I still haven't
reviewed. So ignore this, my bad.
> 
>> +		}
>>  		break;
>>  	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
>>  		/*
>>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/9] btrfs: replace go back to suspended if target missing
  2018-11-07 11:43 ` [PATCH 2/9] btrfs: replace go back to suspended if target missing Anand Jain
@ 2018-11-07 12:35   ` Nikolay Borisov
  2018-11-11 14:00     ` Anand Jain
  0 siblings, 1 reply; 24+ messages in thread
From: Nikolay Borisov @ 2018-11-07 12:35 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs



On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> At the time of forced unmount we place the running replace to
> BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state, so when the system comes
> back and suppose the target device is missing, then let the replace
> state continue to be in BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state
> instead of BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED as there isn't any
> matching scrub running as part of replace.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/dev-replace.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 59991165e126..47d6768a9cde 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -884,6 +884,9 @@ int btrfs_resume_dev_replace_async(struct btrfs_fs_info *fs_info)
>  			   "cannot continue dev_replace, tgtdev is missing");
>  		btrfs_info(fs_info,
>  			   "you may cancel the operation after 'mount -o degraded'");
> +		dev_replace->replace_state =
> +					BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED;
> +		dev_replace->item_needs_writeback = 1;

Why do we need items_needs_writeback = 1 here, nothing is changed w.r.t
on-disk data?

>  		btrfs_dev_replace_write_unlock(dev_replace);
>  		return 0;
>  	}
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 9/9] btrfs: add explicit check for replace result no error
  2018-11-07 12:15   ` Nikolay Borisov
@ 2018-11-08  7:26     ` Anand Jain
  0 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2018-11-08  7:26 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs



On 11/07/2018 08:15 PM, Nikolay Borisov wrote:
> 
> 
> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>> We recast the replace return status
>> BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS to 0, to indicate no
>> error.
>> And since BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR should also return 0,
>> which is also declared as 0, so we just return. Instead add it to the if
>> statement so that there is enough clarity while reading the code.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/dev-replace.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>> index cf3554554616..ca44998189c7 100644
>> --- a/fs/btrfs/dev-replace.c
>> +++ b/fs/btrfs/dev-replace.c
>> @@ -533,8 +533,9 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
>>   					args->start.cont_reading_from_srcdev_mode);
>>   	args->result = ret;
>>   	/* don't warn if EINPROGRESS, someone else might be running scrub */
>> -	if (ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS)
>> -		ret = 0;
>> +	if (ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS ||
>> +	    ret == BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR)
> 
> BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR can only be returned from
> btrfs_dev_replace_cancel,

  It can return 0 and which is also
  BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR which this patch makes it
  explicit.

  Looking at this again tells me that, as btrfs_dev_replace_start()
  is internal helper function, its better if it free from all usage
  of BTRFS_IOCTL_DEV_REPLACE_RESULT*.

Thanks, Anand

> so what you are doing with checking ret ==
> BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR is redundant. So using
> BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR is at the very least misleading
> here.

> I suggest you drop this patch.
> 
>> +		return 0;
>>   
>>   	return ret;
>>   }
>>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 7/9] btrfs: quiten warn if the replace is canceled at finish
  2018-11-07 12:17   ` Nikolay Borisov
@ 2018-11-08  8:06     ` Anand Jain
  0 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2018-11-08  8:06 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs



On 11/07/2018 08:17 PM, Nikolay Borisov wrote:
> 
> 
> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>> -	WARN_ON(ret);
>> +	if (ret != -ECANCELED)
>> +		WARN_ON(ret);
> 
> WARN_ON(ret && ret != -ECANCELED)
> 

Will fix.
Thanks, Anand

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state
  2018-11-07 12:19   ` Nikolay Borisov
@ 2018-11-08  8:33     ` Anand Jain
  2018-11-08  8:52       ` Nikolay Borisov
  0 siblings, 1 reply; 24+ messages in thread
From: Anand Jain @ 2018-11-08  8:33 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs



On 11/07/2018 08:19 PM, Nikolay Borisov wrote:
> 
> 
> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>> +		/* scrub for replace must not be running in suspended state */
>> +		if (btrfs_scrub_cancel(fs_info) != -ENOTCONN)
>> +			ASSERT(0);
> 
> ASSERT(btrfs_scrub_cancel(fs_info) == -ENOTCONN)
> 

There will be substantial difference in code when compiled with and 
without CONFIG_BTRFS_ASSERT [1]. That is, btrfs_scrub_cancel(fs_info)
won't be run at all,  I would like to keep it as it is.

[1]
------
./fs/btrfs/ctree.h
#ifdef CONFIG_BTRFS_ASSERT

__cold
static inline void assfail(const char *expr, const char *file, int line)
{
         pr_err("assertion failed: %s, file: %s, line: %d\n",
                expr, file, line);
         BUG();
}

#define ASSERT(expr)    \
         (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
#else
#define ASSERT(expr)    ((void)0)
#endif
-------

Thanks, Anand



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state
  2018-11-08  8:33     ` Anand Jain
@ 2018-11-08  8:52       ` Nikolay Borisov
  2018-11-08  9:07         ` Anand Jain
  0 siblings, 1 reply; 24+ messages in thread
From: Nikolay Borisov @ 2018-11-08  8:52 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs



On 8.11.18 г. 10:33 ч., Anand Jain wrote:
> 
> 
> On 11/07/2018 08:19 PM, Nikolay Borisov wrote:
>>
>>
>> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>>> +        /* scrub for replace must not be running in suspended state */
>>> +        if (btrfs_scrub_cancel(fs_info) != -ENOTCONN)
>>> +            ASSERT(0);
>>
>> ASSERT(btrfs_scrub_cancel(fs_info) == -ENOTCONN)
>>
> 
> There will be substantial difference in code when compiled with and
> without CONFIG_BTRFS_ASSERT [1]. That is, btrfs_scrub_cancel(fs_info)
> won't be run at all,  I would like to keep it as it is.

Fair point, in that case do:

ret = btrfs_scrub_cancel(fs_info);
ASSERT(ret != -ENOTCONN);

result


> 
> [1]
> ------
> ./fs/btrfs/ctree.h
> #ifdef CONFIG_BTRFS_ASSERT
> 
> __cold
> static inline void assfail(const char *expr, const char *file, int line)
> {
>         pr_err("assertion failed: %s, file: %s, line: %d\n",
>                expr, file, line);
>         BUG();
> }
> 
> #define ASSERT(expr)    \
>         (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
> #else
> #define ASSERT(expr)    ((void)0)
> #endif
> -------
> 
> Thanks, Anand
> 
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state
  2018-11-08  8:52       ` Nikolay Borisov
@ 2018-11-08  9:07         ` Anand Jain
  0 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2018-11-08  9:07 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs



On 11/08/2018 04:52 PM, Nikolay Borisov wrote:
> 
> 
> On 8.11.18 г. 10:33 ч., Anand Jain wrote:
>>
>>
>> On 11/07/2018 08:19 PM, Nikolay Borisov wrote:
>>>
>>>
>>> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>>>> +        /* scrub for replace must not be running in suspended state */
>>>> +        if (btrfs_scrub_cancel(fs_info) != -ENOTCONN)
>>>> +            ASSERT(0);
>>>
>>> ASSERT(btrfs_scrub_cancel(fs_info) == -ENOTCONN)
>>>
>>
>> There will be substantial difference in code when compiled with and
>> without CONFIG_BTRFS_ASSERT [1]. That is, btrfs_scrub_cancel(fs_info)
>> won't be run at all,  I would like to keep it as it is.
> 
> Fair point, in that case do:
> 
> ret = btrfs_scrub_cancel(fs_info);
> ASSERT(ret != -ENOTCONN);

Fixed.

Thanks, Anand

> result
> 
> 
>>
>> [1]
>> ------
>> ./fs/btrfs/ctree.h
>> #ifdef CONFIG_BTRFS_ASSERT
>>
>> __cold
>> static inline void assfail(const char *expr, const char *file, int line)
>> {
>>          pr_err("assertion failed: %s, file: %s, line: %d\n",
>>                 expr, file, line);
>>          BUG();
>> }
>>
>> #define ASSERT(expr)    \
>>          (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
>> #else
>> #define ASSERT(expr)    ((void)0)
>> #endif
>> -------
>>
>> Thanks, Anand
>>
>>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/9] btrfs: replace go back to suspended if target missing
  2018-11-07 12:35   ` Nikolay Borisov
@ 2018-11-11 14:00     ` Anand Jain
  0 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2018-11-11 14:00 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs



On 11/07/2018 08:35 PM, Nikolay Borisov wrote:
> 
> 
> On 7.11.18 г. 13:43 ч., Anand Jain wrote:
>> At the time of forced unmount we place the running replace to
>> BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state, so when the system comes
>> back and suppose the target device is missing, then let the replace
>> state continue to be in BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED state
>> instead of BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED as there isn't any
>> matching scrub running as part of replace.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/dev-replace.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>> index 59991165e126..47d6768a9cde 100644
>> --- a/fs/btrfs/dev-replace.c
>> +++ b/fs/btrfs/dev-replace.c
>> @@ -884,6 +884,9 @@ int btrfs_resume_dev_replace_async(struct btrfs_fs_info *fs_info)
>>   			   "cannot continue dev_replace, tgtdev is missing");
>>   		btrfs_info(fs_info,
>>   			   "you may cancel the operation after 'mount -o degraded'");
>> +		dev_replace->replace_state =
>> +					BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED;
>> +		dev_replace->item_needs_writeback = 1;
> 
> Why do we need items_needs_writeback = 1 here, nothing is changed w.r.t
> on-disk data?

  You are right. We don't need writeback. Will fix.

Thanks, Anand


>>   		btrfs_dev_replace_write_unlock(dev_replace);
>>   		return 0;
>>   	}
>>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static
  2018-11-11 14:22 [PATCH 0/9 v2] fix replace-start and replace-cancel racing Anand Jain
@ 2018-11-11 14:22 ` Anand Jain
  0 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2018-11-11 14:22 UTC (permalink / raw)
  To: linux-btrfs

There isn't any other consumer other than in its own file dev-replace.c.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/dev-replace.c | 2 +-
 fs/btrfs/dev-replace.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 2aa48aecc52b..59991165e126 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -390,7 +390,7 @@ static char* btrfs_dev_name(struct btrfs_device *device)
 		return rcu_str_deref(device->name);
 }
 
-int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
+static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 		const char *tgtdev_name, u64 srcdevid, const char *srcdev_name,
 		int read_src)
 {
diff --git a/fs/btrfs/dev-replace.h b/fs/btrfs/dev-replace.h
index 795c551f5b5e..27e3bb0cca11 100644
--- a/fs/btrfs/dev-replace.h
+++ b/fs/btrfs/dev-replace.h
@@ -13,9 +13,6 @@ int btrfs_run_dev_replace(struct btrfs_trans_handle *trans,
 			  struct btrfs_fs_info *fs_info);
 int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
 			    struct btrfs_ioctl_dev_replace_args *args);
-int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
-		const char *tgtdev_name, u64 srcdevid, const char *srcdev_name,
-		int read_src);
 void btrfs_dev_replace_status(struct btrfs_fs_info *fs_info,
 			      struct btrfs_ioctl_dev_replace_args *args);
 int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2018-11-11 14:22 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-07 11:43 [PATCH 0/9] fix replace-start and replace-cancel racing Anand Jain
2018-11-07 11:43 ` [PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static Anand Jain
2018-11-07 12:04   ` Nikolay Borisov
2018-11-07 11:43 ` [PATCH 2/9] btrfs: replace go back to suspended if target missing Anand Jain
2018-11-07 12:35   ` Nikolay Borisov
2018-11-11 14:00     ` Anand Jain
2018-11-07 11:43 ` [PATCH 3/9] btrfs: replace back to suspend state if EXCL OP is running Anand Jain
2018-11-07 11:43 ` [PATCH 4/9] btrfs: fix UAF due to race between replace start and cancel Anand Jain
2018-11-07 11:43 ` [PATCH 5/9] btrfs: replace cancel is successful if scrub cancel is successful Anand Jain
2018-11-07 12:25   ` Nikolay Borisov
2018-11-07 12:26     ` Nikolay Borisov
2018-11-07 11:43 ` [PATCH 6/9] btrfs: replace's scrub must not be running in replace suspended state Anand Jain
2018-11-07 12:19   ` Nikolay Borisov
2018-11-08  8:33     ` Anand Jain
2018-11-08  8:52       ` Nikolay Borisov
2018-11-08  9:07         ` Anand Jain
2018-11-07 11:43 ` [PATCH 7/9] btrfs: quiten warn if the replace is canceled at finish Anand Jain
2018-11-07 12:17   ` Nikolay Borisov
2018-11-08  8:06     ` Anand Jain
2018-11-07 11:43 ` [PATCH 8/9] btrfs: user requsted replace cancel is not an error Anand Jain
2018-11-07 11:43 ` [PATCH 9/9] btrfs: add explicit check for replace result no error Anand Jain
2018-11-07 12:15   ` Nikolay Borisov
2018-11-08  7:26     ` Anand Jain
2018-11-11 14:22 [PATCH 0/9 v2] fix replace-start and replace-cancel racing Anand Jain
2018-11-11 14:22 ` [PATCH 1/9] btrfs: mark btrfs_dev_replace_start() as static Anand Jain

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.