| Message ID | 20180626234315.15643-1-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27.06.2018 02:43, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > If a power failure happens while the qgroup rescan kthread is running, > the next mount operation will always fail. This is because of a recent > regression that makes qgroup_rescan_init() incorrectly return -EINVAL > when we are mounting the filesystem (through btrfs_read_qgroup_config()). > This causes the -EINVAL error to be returned regardless of any qgroup > flags being set instead of returning the error only when neither of > the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON > are set. > > A test case for fstests follows up soon. > > Fixes: 9593bf49675e ("btrfs: qgroup: show more meaningful qgroup_rescan_init error message") > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > fs/btrfs/qgroup.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index 1874a6d2e6f5..d4171de93087 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -2784,13 +2784,20 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid, > > if (!init_flags) { > /* we're resuming qgroup rescan at mount time */ > - if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN)) > + if (!(fs_info->qgroup_flags & > + BTRFS_QGROUP_STATUS_FLAG_RESCAN)) { > btrfs_warn(fs_info, > "qgroup rescan init failed, qgroup is not enabled"); > - else if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_ON)) > + ret = -EINVAL; > + } else if (!(fs_info->qgroup_flags & > + BTRFS_QGROUP_STATUS_FLAG_ON)) { > btrfs_warn(fs_info, > "qgroup rescan init failed, qgroup rescan is not queued"); > - return -EINVAL; > + ret = -EINVAL; > + } > + > + if (ret) > + return ret; How is this patch functionally different than the old code. In both cases if either of those 2 is not set a warn is printed and -EINVAL is returned? > } > > mutex_lock(&fs_info->qgroup_rescan_lock); > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 27, 2018 at 4:44 PM, Nikolay Borisov <nborisov@suse.com> wrote: > > > On 27.06.2018 02:43, fdmanana@kernel.org wrote: >> From: Filipe Manana <fdmanana@suse.com> >> >> If a power failure happens while the qgroup rescan kthread is running, >> the next mount operation will always fail. This is because of a recent >> regression that makes qgroup_rescan_init() incorrectly return -EINVAL >> when we are mounting the filesystem (through btrfs_read_qgroup_config()). >> This causes the -EINVAL error to be returned regardless of any qgroup >> flags being set instead of returning the error only when neither of >> the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON >> are set. >> >> A test case for fstests follows up soon. >> >> Fixes: 9593bf49675e ("btrfs: qgroup: show more meaningful qgroup_rescan_init error message") >> Signed-off-by: Filipe Manana <fdmanana@suse.com> >> --- >> fs/btrfs/qgroup.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c >> index 1874a6d2e6f5..d4171de93087 100644 >> --- a/fs/btrfs/qgroup.c >> +++ b/fs/btrfs/qgroup.c >> @@ -2784,13 +2784,20 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid, >> >> if (!init_flags) { >> /* we're resuming qgroup rescan at mount time */ >> - if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN)) >> + if (!(fs_info->qgroup_flags & >> + BTRFS_QGROUP_STATUS_FLAG_RESCAN)) { >> btrfs_warn(fs_info, >> "qgroup rescan init failed, qgroup is not enabled"); >> - else if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_ON)) >> + ret = -EINVAL; >> + } else if (!(fs_info->qgroup_flags & >> + BTRFS_QGROUP_STATUS_FLAG_ON)) { >> btrfs_warn(fs_info, >> "qgroup rescan init failed, qgroup rescan is not queued"); >> - return -EINVAL; >> + ret = -EINVAL; >> + } >> + >> + if (ret) >> + return ret; > > > How is this patch functionally different than the old code. In both > cases if either of those 2 is not set a warn is printed and -EINVAL is > returned? It is explained in the changelog: "This is because of a recent regression that makes qgroup_rescan_init() incorrectly return -EINVAL when we are mounting the filesystem (through btrfs_read_qgroup_config()). This causes the -EINVAL error to be returned regardless of any qgroup flags being set instead of returning the error only when neither of the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON are set." If you can't understand it, try the test case... > >> } >> >> mutex_lock(&fs_info->qgroup_rescan_lock); >> -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27.06.2018 18:45, Filipe Manana wrote: > On Wed, Jun 27, 2018 at 4:44 PM, Nikolay Borisov <nborisov@suse.com> wrote: >> >> >> On 27.06.2018 02:43, fdmanana@kernel.org wrote: >>> From: Filipe Manana <fdmanana@suse.com> >>> >>> If a power failure happens while the qgroup rescan kthread is running, >>> the next mount operation will always fail. This is because of a recent >>> regression that makes qgroup_rescan_init() incorrectly return -EINVAL >>> when we are mounting the filesystem (through btrfs_read_qgroup_config()). >>> This causes the -EINVAL error to be returned regardless of any qgroup >>> flags being set instead of returning the error only when neither of >>> the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON >>> are set. >>> >>> A test case for fstests follows up soon. >>> >>> Fixes: 9593bf49675e ("btrfs: qgroup: show more meaningful qgroup_rescan_init error message") >>> Signed-off-by: Filipe Manana <fdmanana@suse.com> >>> --- >>> fs/btrfs/qgroup.c | 13 ++++++++++--- >>> 1 file changed, 10 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c >>> index 1874a6d2e6f5..d4171de93087 100644 >>> --- a/fs/btrfs/qgroup.c >>> +++ b/fs/btrfs/qgroup.c >>> @@ -2784,13 +2784,20 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid, >>> >>> if (!init_flags) { >>> /* we're resuming qgroup rescan at mount time */ >>> - if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN)) >>> + if (!(fs_info->qgroup_flags & >>> + BTRFS_QGROUP_STATUS_FLAG_RESCAN)) { >>> btrfs_warn(fs_info, >>> "qgroup rescan init failed, qgroup is not enabled"); >>> - else if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_ON)) >>> + ret = -EINVAL; >>> + } else if (!(fs_info->qgroup_flags & >>> + BTRFS_QGROUP_STATUS_FLAG_ON)) { >>> btrfs_warn(fs_info, >>> "qgroup rescan init failed, qgroup rescan is not queued"); >>> - return -EINVAL; >>> + ret = -EINVAL; >>> + } >>> + >>> + if (ret) >>> + return ret; >> >> >> How is this patch functionally different than the old code. In both >> cases if either of those 2 is not set a warn is printed and -EINVAL is >> returned? > > It is explained in the changelog: No need to be snide > > "This is because of a recent > regression that makes qgroup_rescan_init() incorrectly return -EINVAL > when we are mounting the filesystem (through btrfs_read_qgroup_config()). > This causes the -EINVAL error to be returned regardless of any qgroup > flags being set instead of returning the error only when neither of > the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON > are set." > > If you can't understand it, try the test case... Ok I see it now, however your description contradicts the code. Currently -EINVAL will be returned when either of the 2 flags is unset i.e !BTRFS_QGROUP_STATUS_FLAG_RESCAN && BTRFS_QGROUP_STATUS_FLAG_ON !BTRFS_QGROUP_STATUS_FLAG_ON && BTRFS_QGROUP_STATUS_FLAG_RESCAN and in your description you refer to neither that is the 2 flags being unset. Perhaps those combinations are invalid due to some other reasons which are not visible in the code but in this case the changelog should be expanded to cover why those 2 combinations are impossible (because if they are -EINVAL is still likely ) ? > > >> >>> } >>> >>> mutex_lock(&fs_info->qgroup_rescan_lock); >>> > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 27, 2018 at 4:55 PM, Nikolay Borisov <nborisov@suse.com> wrote: > > > On 27.06.2018 18:45, Filipe Manana wrote: >> On Wed, Jun 27, 2018 at 4:44 PM, Nikolay Borisov <nborisov@suse.com> wrote: >>> >>> >>> On 27.06.2018 02:43, fdmanana@kernel.org wrote: >>>> From: Filipe Manana <fdmanana@suse.com> >>>> >>>> If a power failure happens while the qgroup rescan kthread is running, >>>> the next mount operation will always fail. This is because of a recent >>>> regression that makes qgroup_rescan_init() incorrectly return -EINVAL >>>> when we are mounting the filesystem (through btrfs_read_qgroup_config()). >>>> This causes the -EINVAL error to be returned regardless of any qgroup >>>> flags being set instead of returning the error only when neither of >>>> the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON >>>> are set. >>>> >>>> A test case for fstests follows up soon. >>>> >>>> Fixes: 9593bf49675e ("btrfs: qgroup: show more meaningful qgroup_rescan_init error message") >>>> Signed-off-by: Filipe Manana <fdmanana@suse.com> >>>> --- >>>> fs/btrfs/qgroup.c | 13 ++++++++++--- >>>> 1 file changed, 10 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c >>>> index 1874a6d2e6f5..d4171de93087 100644 >>>> --- a/fs/btrfs/qgroup.c >>>> +++ b/fs/btrfs/qgroup.c >>>> @@ -2784,13 +2784,20 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid, >>>> >>>> if (!init_flags) { >>>> /* we're resuming qgroup rescan at mount time */ >>>> - if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN)) >>>> + if (!(fs_info->qgroup_flags & >>>> + BTRFS_QGROUP_STATUS_FLAG_RESCAN)) { >>>> btrfs_warn(fs_info, >>>> "qgroup rescan init failed, qgroup is not enabled"); >>>> - else if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_ON)) >>>> + ret = -EINVAL; >>>> + } else if (!(fs_info->qgroup_flags & >>>> + BTRFS_QGROUP_STATUS_FLAG_ON)) { >>>> btrfs_warn(fs_info, >>>> "qgroup rescan init failed, qgroup rescan is not queued"); >>>> - return -EINVAL; >>>> + ret = -EINVAL; >>>> + } >>>> + >>>> + if (ret) >>>> + return ret; >>> >>> >>> How is this patch functionally different than the old code. In both >>> cases if either of those 2 is not set a warn is printed and -EINVAL is >>> returned? >> >> It is explained in the changelog: > > No need to be snide No one's being snide. Simply, I can't see how the changelog doesn't explain (what is already quite easy to notice from the code). > >> >> "This is because of a recent >> regression that makes qgroup_rescan_init() incorrectly return -EINVAL >> when we are mounting the filesystem (through btrfs_read_qgroup_config()). >> This causes the -EINVAL error to be returned regardless of any qgroup >> flags being set instead of returning the error only when neither of >> the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON >> are set." >> >> If you can't understand it, try the test case... > > Ok I see it now, however your description contradicts the code. > Currently -EINVAL will be returned when either of the 2 flags is unset i.e > > !BTRFS_QGROUP_STATUS_FLAG_RESCAN && BTRFS_QGROUP_STATUS_FLAG_ON > !BTRFS_QGROUP_STATUS_FLAG_ON && BTRFS_QGROUP_STATUS_FLAG_RESCAN > > and in your description you refer to neither that is the 2 flags being > unset. Perhaps those combinations are invalid due to some other reasons > which are not visible in the code but in this case the changelog should > be expanded to cover why those 2 combinations are impossible (because if > they are -EINVAL is still likely ) ? I don't think the changelog is contradictory. It says that -EINVAL is always returned, independently of which qgroup flags are set/missing. Further it says that the error should be returned only when one of those 2 qgroup flags is not set (or both are not set). > >> >> >>> >>>> } >>>> >>>> mutex_lock(&fs_info->qgroup_rescan_lock); >>>> >> -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018年06月27日 07:43, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > If a power failure happens while the qgroup rescan kthread is running, > the next mount operation will always fail. This is because of a recent > regression that makes qgroup_rescan_init() incorrectly return -EINVAL > when we are mounting the filesystem (through btrfs_read_qgroup_config()). > This causes the -EINVAL error to be returned regardless of any qgroup > flags being set instead of returning the error only when neither of > the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON > are set. My fault. > > A test case for fstests follows up soon. > > Fixes: 9593bf49675e ("btrfs: qgroup: show more meaningful qgroup_rescan_init error message") > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/qgroup.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index 1874a6d2e6f5..d4171de93087 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -2784,13 +2784,20 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid, > > if (!init_flags) { > /* we're resuming qgroup rescan at mount time */ > - if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN)) > + if (!(fs_info->qgroup_flags & > + BTRFS_QGROUP_STATUS_FLAG_RESCAN)) { > btrfs_warn(fs_info, > "qgroup rescan init failed, qgroup is not enabled"); > - else if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_ON)) > + ret = -EINVAL; > + } else if (!(fs_info->qgroup_flags & > + BTRFS_QGROUP_STATUS_FLAG_ON)) { > btrfs_warn(fs_info, > "qgroup rescan init failed, qgroup rescan is not queued"); > - return -EINVAL; > + ret = -EINVAL; > + } > + > + if (ret) > + return ret; > } > > mutex_lock(&fs_info->qgroup_rescan_lock); >
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index 1874a6d2e6f5..d4171de93087 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -2784,13 +2784,20 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid, if (!init_flags) { /* we're resuming qgroup rescan at mount time */ - if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN)) + if (!(fs_info->qgroup_flags & + BTRFS_QGROUP_STATUS_FLAG_RESCAN)) { btrfs_warn(fs_info, "qgroup rescan init failed, qgroup is not enabled"); - else if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_ON)) + ret = -EINVAL; + } else if (!(fs_info->qgroup_flags & + BTRFS_QGROUP_STATUS_FLAG_ON)) { btrfs_warn(fs_info, "qgroup rescan init failed, qgroup rescan is not queued"); - return -EINVAL; + ret = -EINVAL; + } + + if (ret) + return ret; } mutex_lock(&fs_info->qgroup_rescan_lock);