| Message ID | 1541591010-29789-6-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 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: > /* >
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: >> /* >>
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: /*
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(-)