| Message ID | 1541946144-8174-5-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix replace-start and replace-cancel racing | expand |
On Sun, Nov 11, 2018 at 10:22:19PM +0800, Anand Jain wrote: > 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 35ce10f18607..d32d696d931c 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; There's a compiler warning: fs/btrfs/dev-replace.c: In function ‘btrfs_dev_replace_cancel’: fs/btrfs/dev-replace.c:865:9: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized] return result; ^~~~~~ I haven't looked closer though it looks valid.
On 11/14/2018 01:24 AM, David Sterba wrote: > On Sun, Nov 11, 2018 at 10:22:19PM +0800, Anand Jain wrote: >> 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 35ce10f18607..d32d696d931c 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; > > There's a compiler warning: > > fs/btrfs/dev-replace.c: In function ‘btrfs_dev_replace_cancel’: > fs/btrfs/dev-replace.c:865:9: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized] > return result; > ^~~~~~ > I haven't looked closer though it looks valid. > int result; is assigned within switch(), so there isn't actual problem. But will initialize the result to -EINVAL to quite the compiler. Sending v3. Thanks, Anand
On Wed, Nov 14, 2018 at 09:28:34AM +0800, Anand Jain wrote: > >> mutex_unlock(&dev_replace->lock_finishing_cancel_unmount); > >> return result; > > > > There's a compiler warning: > > > > fs/btrfs/dev-replace.c: In function ‘btrfs_dev_replace_cancel’: > > fs/btrfs/dev-replace.c:865:9: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized] > > return result; > > ^~~~~~ > > > I haven't looked closer though it looks valid. > > int result; is assigned within switch(), so there isn't actual problem. The warning is there because switch (dev_replace->replace_state) does not have a default: case that would catch the values outside of what's defined by the enum. So in that case result would have undefined value. > But will initialize the result to -EINVAL to quite the compiler. > Sending v3. I don't see any change in the followup version. https://patchwork.kernel.org/patch/10681939/
On Thu, Nov 15, 2018 at 03:00:21PM +0100, David Sterba wrote: > On Wed, Nov 14, 2018 at 09:28:34AM +0800, Anand Jain wrote: > > >> mutex_unlock(&dev_replace->lock_finishing_cancel_unmount); > > >> return result; > > > > > > There's a compiler warning: > > > > > > fs/btrfs/dev-replace.c: In function ‘btrfs_dev_replace_cancel’: > > > fs/btrfs/dev-replace.c:865:9: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized] > > > return result; > > > ^~~~~~ > > > > > I haven't looked closer though it looks valid. > > > > int result; is assigned within switch(), so there isn't actual problem. > > The warning is there because switch (dev_replace->replace_state) does > not have a default: case that would catch the values outside of what's > defined by the enum. So in that case result would have undefined value. > > > But will initialize the result to -EINVAL to quite the compiler. > > Sending v3. > > I don't see any change in the followup version. > https://patchwork.kernel.org/patch/10681939/ I've added default: result = -EINVAL; to the end of the switch.
On 11/15/2018 11:25 PM, David Sterba wrote: > On Thu, Nov 15, 2018 at 03:00:21PM +0100, David Sterba wrote: >> On Wed, Nov 14, 2018 at 09:28:34AM +0800, Anand Jain wrote: >>>>> mutex_unlock(&dev_replace->lock_finishing_cancel_unmount); >>>>> return result; >>>> >>>> There's a compiler warning: >>>> >>>> fs/btrfs/dev-replace.c: In function ‘btrfs_dev_replace_cancel’: >>>> fs/btrfs/dev-replace.c:865:9: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized] >>>> return result; >>>> ^~~~~~ >>> >>>> I haven't looked closer though it looks valid. >>> >>> int result; is assigned within switch(), so there isn't actual problem. >> >> The warning is there because switch (dev_replace->replace_state) does >> not have a default: case that would catch the values outside of what's >> defined by the enum. So in that case result would have undefined value. >> >>> But will initialize the result to -EINVAL to quite the compiler. >>> Sending v3. >> >> I don't see any change in the followup version. >> https://patchwork.kernel.org/patch/10681939/ Looks like I didn't run git commit --amend, now pulled your changes from misc-next. > I've added > > default: > result = -EINVAL; > > to the end of the switch. Ok. misc-next looks good to me. Thanks, Anand
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c index 35ce10f18607..d32d696d931c 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; }
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(-)