| Message ID | 1541591010-29789-10-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: > 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; > } >
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; >> } >>
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; }
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(-)