| Message ID | 1472687013-463-1-git-send-email-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
At 09/01/2016 07:43 AM, Liu Bo wrote: > Qgroup function may overwrite the saved error 'err' with 0 > in case quota is not enabled, and this ends up with a > endless loop in balance because we keep going back to balance > the same block group. In which case? If join_trans() fails, we won't go through qgroup fix. And before join_trans(), they all go to out_free/out tag. Nothing to do with qgroup fix. And if we hit qgroup_fix_relocated_data_extents(), then 'err' is alreayd 0, nothing we need to save. And even for quota disabled case, qgroup_fix_relocated_data_extents() will just return 0. Nothing wrong at all. Or did I miss something? Thanks, Qu > > It really should use 'ret' instead. > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > fs/btrfs/relocation.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 8a2c2a0..c0c13dc 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -4200,9 +4200,11 @@ restart: > err = PTR_ERR(trans); > goto out_free; > } > - err = qgroup_fix_relocated_data_extents(trans, rc); > - if (err < 0) { > - btrfs_abort_transaction(trans, err); > + ret = qgroup_fix_relocated_data_extents(trans, rc); > + if (ret < 0) { > + btrfs_abort_transaction(trans, ret); > + if (!err) > + err = ret; > goto out_free; > } > btrfs_commit_transaction(trans, rc->extent_root); > -- 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 Thu, Sep 01, 2016 at 08:32:00AM +0800, Qu Wenruo wrote: > > > At 09/01/2016 07:43 AM, Liu Bo wrote: > > Qgroup function may overwrite the saved error 'err' with 0 > > in case quota is not enabled, and this ends up with a > > endless loop in balance because we keep going back to balance > > the same block group. > > In which case? > > If join_trans() fails, we won't go through qgroup fix. > And before join_trans(), they all go to out_free/out tag. > Nothing to do with qgroup fix. I don't think so. It doesn't always go to out_free, in the while() loop, it'd break the loop on any error and record it in @err, then go through prepare_to_merge() + merge_reloc_roots() and other stuff. Here's an example for keeping err after the above while loop(), ... err = prepare_to_merge(rc, err); ... Thanks, -liubo > > And if we hit qgroup_fix_relocated_data_extents(), then 'err' is alreayd 0, > nothing we need to save. > > And even for quota disabled case, qgroup_fix_relocated_data_extents() will > just return 0. Nothing wrong at all. > > Or did I miss something? > > Thanks, > Qu > > > > It really should use 'ret' instead. > > > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > > --- > > fs/btrfs/relocation.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > > index 8a2c2a0..c0c13dc 100644 > > --- a/fs/btrfs/relocation.c > > +++ b/fs/btrfs/relocation.c > > @@ -4200,9 +4200,11 @@ restart: > > err = PTR_ERR(trans); > > goto out_free; > > } > > - err = qgroup_fix_relocated_data_extents(trans, rc); > > - if (err < 0) { > > - btrfs_abort_transaction(trans, err); > > + ret = qgroup_fix_relocated_data_extents(trans, rc); > > + if (ret < 0) { > > + btrfs_abort_transaction(trans, ret); > > + if (!err) > > + err = ret; > > goto out_free; > > } > > btrfs_commit_transaction(trans, rc->extent_root); > > > > -- 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
At 09/01/2016 08:54 AM, Liu Bo wrote: > On Thu, Sep 01, 2016 at 08:32:00AM +0800, Qu Wenruo wrote: >> >> >> At 09/01/2016 07:43 AM, Liu Bo wrote: >>> Qgroup function may overwrite the saved error 'err' with 0 >>> in case quota is not enabled, and this ends up with a >>> endless loop in balance because we keep going back to balance >>> the same block group. >> >> In which case? >> >> If join_trans() fails, we won't go through qgroup fix. >> And before join_trans(), they all go to out_free/out tag. >> Nothing to do with qgroup fix. > > I don't think so. > > It doesn't always go to out_free, in the while() loop, it'd break the > loop on any error and record it in @err, then go through > prepare_to_merge() + merge_reloc_roots() and other stuff. > > Here's an example for keeping err after the above while loop(), > ... > err = prepare_to_merge(rc, err); > ... OK, that's right. Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com> While IMHO it's a larger problem that we didn't handle the error immediately after prepare_to_merge() or inside the while(1) loop, but postpone it to later parts. Thanks, Qu > > Thanks, > > -liubo > >> >> And if we hit qgroup_fix_relocated_data_extents(), then 'err' is alreayd 0, >> nothing we need to save. >> >> And even for quota disabled case, qgroup_fix_relocated_data_extents() will >> just return 0. Nothing wrong at all. >> >> Or did I miss something? >> >> Thanks, >> Qu >>> >>> It really should use 'ret' instead. >>> >>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com> >>> --- >>> fs/btrfs/relocation.c | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c >>> index 8a2c2a0..c0c13dc 100644 >>> --- a/fs/btrfs/relocation.c >>> +++ b/fs/btrfs/relocation.c >>> @@ -4200,9 +4200,11 @@ restart: >>> err = PTR_ERR(trans); >>> goto out_free; >>> } >>> - err = qgroup_fix_relocated_data_extents(trans, rc); >>> - if (err < 0) { >>> - btrfs_abort_transaction(trans, err); >>> + ret = qgroup_fix_relocated_data_extents(trans, rc); >>> + if (ret < 0) { >>> + btrfs_abort_transaction(trans, ret); >>> + if (!err) >>> + err = ret; >>> goto out_free; >>> } >>> btrfs_commit_transaction(trans, rc->extent_root); >>> >> >> > > -- 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
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 8a2c2a0..c0c13dc 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -4200,9 +4200,11 @@ restart: err = PTR_ERR(trans); goto out_free; } - err = qgroup_fix_relocated_data_extents(trans, rc); - if (err < 0) { - btrfs_abort_transaction(trans, err); + ret = qgroup_fix_relocated_data_extents(trans, rc); + if (ret < 0) { + btrfs_abort_transaction(trans, ret); + if (!err) + err = ret; goto out_free; } btrfs_commit_transaction(trans, rc->extent_root);
Qgroup function may overwrite the saved error 'err' with 0 in case quota is not enabled, and this ends up with a endless loop in balance because we keep going back to balance the same block group. It really should use 'ret' instead. Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/relocation.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)