Re: [PATCH 8/9] btrfs: user requsted replace cancel is not an error
On 11/16/2018 06:29 PM, Anand Jain wrote:
On 11/15/2018 11:31 PM, David Sterba wrote:
On Sun, Nov 11, 2018 at 10:22:23PM +0800, Anand Jain wrote:
As of now only user requested replace cancel can cancel the
replace-scrub
so no need to log error for it.
This has probably some user visible effect or threre are steps to
reproduce the message even if it's not expected (ie. the case that this
patch fixes). Please update the changelog, thanks.
before the patch [1]
[1]
btrfs: fix use-after-free due to race between replace start and cancel
We used to set the replace_state to CANCELED [2] and then call
btrfs_scrub_cancel(), but the problem with this approach is
if the scrub isn't running yet, then things get messier.
[2]
- dev_replace->replace_state =
BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED;
So to fix, [1] shall set replace_state to CANCELED only after the
replace cancel thread has successfully canceled the replace using
btrfs_scrub_cancel().
And about the cleanup process for the replace target..
we call
btrfs_dev_replace_finishing(.. ret)
after
ret= btrfs_scrub_dev()
now ret is -ECANCELED due to replace cancel request by the user.
(its not set to -ECANCELED for any other reason).
btrfs_dev_replace_finishing() has the following code [3] which it
earlier blocked processing of the cleanup process after the cancel,
because the replace_state was already updated to
BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED.
[3]
--
static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
int scrub_ret)
{
::
/* was the operation canceled, or is it finished? */
if (dev_replace->replace_state !=
BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED) {
btrfs_dev_replace_read_unlock(dev_replace);
mutex_unlock(_replace->lock_finishing_cancel_unmount);
return 0;
}
---
In fact before this patch [1] the code wasn't sync-ing the IO
when replace was canceled. Which this patch also fixes by using
the btrfs_dev_replace_finishing()
---
btrfs_dev_replace_finishing
::
/*
* flush all outstanding I/O and inode extent mappings before the
* copy operation is declared as being finished
*/
ret = btrfs_start_delalloc_roots(fs_info, -1);
if (ret) {
mutex_unlock(_replace->lock_finishing_cancel_unmount);
return ret;
}
btrfs_wait_ordered_roots(fs_info, U64_MAX, 0, (u64)-1);
---
So to answer above question... these warn and error log wasn't
reported before when replace was canceled and now since I am
using the btrfs_dev_replace_finishing() to finish the job
of cancel.. it shall be reported which is ok to quite.
Do you think we still need to update the change log?
OR I think more appropriately this patch should be merged with
[PATCH 4/9] btrfs: fix UAF due to race between replace start and cancel
-Anand
HTH.
Thanks, Anand
Re: [PATCH 8/9] btrfs: user requsted replace cancel is not an error
On 11/15/2018 11:31 PM, David Sterba wrote:
On Sun, Nov 11, 2018 at 10:22:23PM +0800, Anand Jain wrote:
As of now only user requested replace cancel can cancel the replace-scrub
so no need to log error for it.
This has probably some user visible effect or threre are steps to
reproduce the message even if it's not expected (ie. the case that this
patch fixes). Please update the changelog, thanks.
before the patch [1]
[1]
btrfs: fix use-after-free due to race between replace start and cancel
We used to set the replace_state to CANCELED [2] and then call
btrfs_scrub_cancel(), but the problem with this approach is
if the scrub isn't running yet, then things get messier.
[2]
- dev_replace->replace_state = BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED;
So to fix, [1] shall set replace_state to CANCELED only after the
replace cancel thread has successfully canceled the replace using
btrfs_scrub_cancel().
And about the cleanup process for the replace target..
we call
btrfs_dev_replace_finishing(.. ret)
after
ret= btrfs_scrub_dev()
now ret is -ECANCELED due to replace cancel request by the user.
(its not set to -ECANCELED for any other reason).
btrfs_dev_replace_finishing() has the following code [3] which it
earlier blocked processing of the cleanup process after the cancel,
because the replace_state was already updated to
BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED.
[3]
--
static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
int scrub_ret)
{
::
/* was the operation canceled, or is it finished? */
if (dev_replace->replace_state !=
BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED) {
btrfs_dev_replace_read_unlock(dev_replace);
mutex_unlock(_replace->lock_finishing_cancel_unmount);
return 0;
}
---
In fact before this patch [1] the code wasn't sync-ing the IO
when replace was canceled. Which this patch also fixes by using
the btrfs_dev_replace_finishing()
---
btrfs_dev_replace_finishing
::
/*
* flush all outstanding I/O and inode extent mappings before the
* copy operation is declared as being finished
*/
ret = btrfs_start_delalloc_roots(fs_info, -1);
if (ret) {
mutex_unlock(_replace->lock_finishing_cancel_unmount);
return ret;
}
btrfs_wait_ordered_roots(fs_info, U64_MAX, 0, (u64)-1);
---
So to answer above question... these warn and error log wasn't
reported before when replace was canceled and now since I am
using the btrfs_dev_replace_finishing() to finish the job
of cancel.. it shall be reported which is ok to quite.
Do you think we still need to update the change log?
HTH.
Thanks, Anand
Re: [PATCH 8/9] btrfs: user requsted replace cancel is not an error
On Sun, Nov 11, 2018 at 10:22:23PM +0800, Anand Jain wrote: > As of now only user requested replace cancel can cancel the replace-scrub > so no need to log error for it. This has probably some user visible effect or threre are steps to reproduce the message even if it's not expected (ie. the case that this patch fixes). Please update the changelog, thanks.
