* Re: [PATCH] Btrfs: fix error handling in btrfs_truncate()
2018-05-18 21:43 [PATCH] Btrfs: fix error handling in btrfs_truncate() Omar Sandoval
@ 2018-05-18 21:54 ` Omar Sandoval
2018-05-21 8:47 ` Nikolay Borisov
2018-05-22 11:53 ` David Sterba
2 siblings, 0 replies; 6+ messages in thread
From: Omar Sandoval @ 2018-05-18 21:54 UTC (permalink / raw)
To: linux-btrfs; +Cc: kernel-team, Chris Mason, Josef Bacik, David Sterba, Jun Wu
On Fri, May 18, 2018 at 02:43:02PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> Jun Wu at Facebook reported that an internal service was seeing a return
> value of 1 from ftruncate() on Btrfs when compression is enabled. This
> is coming from the NEED_TRUNCATE_BLOCK return value from
> btrfs_truncate_inode_items().
>
> btrfs_truncate() uses two variables for error handling, ret and err (if
> this sounds familiar, it's because btrfs_truncate_inode_items() does
> something similar). When btrfs_truncate_inode_items() returns non-zero,
> we set err to the return value, but we don't reset it to zero in the
> successful NEED_TRUNCATE_BLOCK case. We only have err because we don't
> want to mask an error if we call btrfs_update_inode() and
> btrfs_end_transaction(), so let's make that its own scoped return
> variable and use ret everywhere else.
To expand on this, this is bad because userspace that checks for a
non-zero return value will think the truncate failed even though it
succeeded, and we also end up not creating an inotify event for the
truncate.
> Fixes: ddfae63cc8e0 ("btrfs: move btrfs_truncate_block out of trans handle")
> Reported-by: Jun Wu <quark@fb.com>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> This is based on Linus' master rather than my orphan ENOSPC fixes
> because I think we want to get this in for v4.17 and stable, and rebase
> my fixes on top of this.
>
> fs/btrfs/inode.c | 34 ++++++++++++++--------------------
> 1 file changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d241285a0d2a..d4a47ae36ed8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -9031,8 +9031,7 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
> struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> struct btrfs_root *root = BTRFS_I(inode)->root;
> struct btrfs_block_rsv *rsv;
> - int ret = 0;
> - int err = 0;
> + int ret;
> struct btrfs_trans_handle *trans;
> u64 mask = fs_info->sectorsize - 1;
> u64 min_size = btrfs_calc_trunc_metadata_size(fs_info, 1);
> @@ -9092,7 +9091,7 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
> */
> trans = btrfs_start_transaction(root, 2);
> if (IS_ERR(trans)) {
> - err = PTR_ERR(trans);
> + ret = PTR_ERR(trans);
> goto out;
> }
>
> @@ -9116,23 +9115,19 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
> inode->i_size,
> BTRFS_EXTENT_DATA_KEY);
> trans->block_rsv = &fs_info->trans_block_rsv;
> - if (ret != -ENOSPC && ret != -EAGAIN) {
> - err = ret;
> + if (ret != -ENOSPC && ret != -EAGAIN)
> break;
> - }
>
> ret = btrfs_update_inode(trans, root, inode);
> - if (ret) {
> - err = ret;
> + if (ret)
> break;
> - }
>
> btrfs_end_transaction(trans);
> btrfs_btree_balance_dirty(fs_info);
>
> trans = btrfs_start_transaction(root, 2);
> if (IS_ERR(trans)) {
> - ret = err = PTR_ERR(trans);
> + ret = PTR_ERR(trans);
> trans = NULL;
> break;
> }
> @@ -9168,26 +9163,25 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
> if (ret == 0 && inode->i_nlink > 0) {
> trans->block_rsv = root->orphan_block_rsv;
> ret = btrfs_orphan_del(trans, BTRFS_I(inode));
> - if (ret)
> - err = ret;
> }
>
> if (trans) {
> + int ret2;
> +
> trans->block_rsv = &fs_info->trans_block_rsv;
> - ret = btrfs_update_inode(trans, root, inode);
> - if (ret && !err)
> - err = ret;
> + ret2 = btrfs_update_inode(trans, root, inode);
> + if (ret2 && !ret)
> + ret = ret2;
>
> - ret = btrfs_end_transaction(trans);
> + ret2 = btrfs_end_transaction(trans);
> + if (ret2 && !ret)
> + ret = ret2;
> btrfs_btree_balance_dirty(fs_info);
> }
> out:
> btrfs_free_block_rsv(fs_info, rsv);
>
> - if (ret && !err)
> - err = ret;
> -
> - return err;
> + return ret;
> }
>
> /*
> --
> 2.17.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs: fix error handling in btrfs_truncate()
2018-05-18 21:43 [PATCH] Btrfs: fix error handling in btrfs_truncate() Omar Sandoval
2018-05-18 21:54 ` Omar Sandoval
@ 2018-05-21 8:47 ` Nikolay Borisov
2018-05-22 11:53 ` David Sterba
2 siblings, 0 replies; 6+ messages in thread
From: Nikolay Borisov @ 2018-05-21 8:47 UTC (permalink / raw)
To: Omar Sandoval, linux-btrfs
Cc: kernel-team, Chris Mason, Josef Bacik, David Sterba
On 19.05.2018 00:43, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> Jun Wu at Facebook reported that an internal service was seeing a return
> value of 1 from ftruncate() on Btrfs when compression is enabled. This
> is coming from the NEED_TRUNCATE_BLOCK return value from
> btrfs_truncate_inode_items().
>
> btrfs_truncate() uses two variables for error handling, ret and err (if
> this sounds familiar, it's because btrfs_truncate_inode_items() does
> something similar). When btrfs_truncate_inode_items() returns non-zero,
> we set err to the return value, but we don't reset it to zero in the
> successful NEED_TRUNCATE_BLOCK case. We only have err because we don't
> want to mask an error if we call btrfs_update_inode() and
> btrfs_end_transaction(), so let's make that its own scoped return
> variable and use ret everywhere else.
>
> Fixes: ddfae63cc8e0 ("btrfs: move btrfs_truncate_block out of trans handle")
> Reported-by: Jun Wu <quark@fb.com>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> This is based on Linus' master rather than my orphan ENOSPC fixes
> because I think we want to get this in for v4.17 and stable, and rebase
> my fixes on top of this.
>
> fs/btrfs/inode.c | 34 ++++++++++++++--------------------
> 1 file changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d241285a0d2a..d4a47ae36ed8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -9031,8 +9031,7 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
> struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> struct btrfs_root *root = BTRFS_I(inode)->root;
> struct btrfs_block_rsv *rsv;
> - int ret = 0;
> - int err = 0;
> + int ret;
> struct btrfs_trans_handle *trans;
> u64 mask = fs_info->sectorsize - 1;
> u64 min_size = btrfs_calc_trunc_metadata_size(fs_info, 1);
> @@ -9092,7 +9091,7 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
> */
> trans = btrfs_start_transaction(root, 2);
> if (IS_ERR(trans)) {
> - err = PTR_ERR(trans);
> + ret = PTR_ERR(trans);
> goto out;
> }
>
> @@ -9116,23 +9115,19 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
> inode->i_size,
> BTRFS_EXTENT_DATA_KEY);
> trans->block_rsv = &fs_info->trans_block_rsv;
> - if (ret != -ENOSPC && ret != -EAGAIN) {
> - err = ret;
> + if (ret != -ENOSPC && ret != -EAGAIN)
> break;
> - }
>
> ret = btrfs_update_inode(trans, root, inode);
> - if (ret) {
> - err = ret;
> + if (ret)
> break;
> - }
>
> btrfs_end_transaction(trans);
> btrfs_btree_balance_dirty(fs_info);
>
> trans = btrfs_start_transaction(root, 2);
> if (IS_ERR(trans)) {
> - ret = err = PTR_ERR(trans);
> + ret = PTR_ERR(trans);
> trans = NULL;
> break;
> }
> @@ -9168,26 +9163,25 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
> if (ret == 0 && inode->i_nlink > 0) {
> trans->block_rsv = root->orphan_block_rsv;
> ret = btrfs_orphan_del(trans, BTRFS_I(inode));
> - if (ret)
> - err = ret;
> }
>
> if (trans) {
> + int ret2;
> +
> trans->block_rsv = &fs_info->trans_block_rsv;
> - ret = btrfs_update_inode(trans, root, inode);
> - if (ret && !err)
> - err = ret;
> + ret2 = btrfs_update_inode(trans, root, inode);
> + if (ret2 && !ret)
> + ret = ret2;
>
> - ret = btrfs_end_transaction(trans);
> + ret2 = btrfs_end_transaction(trans);
> + if (ret2 && !ret)
> + ret = ret2;
> btrfs_btree_balance_dirty(fs_info);
> }
> out:
> btrfs_free_block_rsv(fs_info, rsv);
>
> - if (ret && !err)
> - err = ret;
> -
> - return err;
> + return ret;
> }
>
> /*
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs: fix error handling in btrfs_truncate()
2018-05-18 21:43 [PATCH] Btrfs: fix error handling in btrfs_truncate() Omar Sandoval
2018-05-18 21:54 ` Omar Sandoval
2018-05-21 8:47 ` Nikolay Borisov
@ 2018-05-22 11:53 ` David Sterba
2018-05-22 12:11 ` Nikolay Borisov
2 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2018-05-22 11:53 UTC (permalink / raw)
To: Omar Sandoval
Cc: linux-btrfs, kernel-team, Chris Mason, Josef Bacik, David Sterba
On Fri, May 18, 2018 at 02:43:02PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> Jun Wu at Facebook reported that an internal service was seeing a return
> value of 1 from ftruncate() on Btrfs when compression is enabled. This
> is coming from the NEED_TRUNCATE_BLOCK return value from
> btrfs_truncate_inode_items().
>
> btrfs_truncate() uses two variables for error handling, ret and err (if
> this sounds familiar, it's because btrfs_truncate_inode_items() does
> something similar).
This err/ret pattern is widely used in btrfs code and IIRC mostly
consistently, where err is the global return and ret is local.
> When btrfs_truncate_inode_items() returns non-zero,
> we set err to the return value, but we don't reset it to zero in the
> successful NEED_TRUNCATE_BLOCK case. We only have err because we don't
> want to mask an error if we call btrfs_update_inode() and
> btrfs_end_transaction(), so let's make that its own scoped return
> variable and use ret everywhere else.
That's an alternative pattern when 'ret' is the global return and ret2
are locals. Either way, it would be good to unify that.
> Fixes: ddfae63cc8e0 ("btrfs: move btrfs_truncate_block out of trans handle")
> Reported-by: Jun Wu <quark@fb.com>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> This is based on Linus' master rather than my orphan ENOSPC fixes
> because I think we want to get this in for v4.17 and stable, and rebase
> my fixes on top of this.
For a 4.17-rc the patch would need to be smaller. You describe the
actual bug as a missing reset of the err, so if it's just that, we can
consider that for 4.17, but the patch as it is changes the error value
propagatin in not so trivial way.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs: fix error handling in btrfs_truncate()
2018-05-22 11:53 ` David Sterba
@ 2018-05-22 12:11 ` Nikolay Borisov
2018-05-22 12:54 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Borisov @ 2018-05-22 12:11 UTC (permalink / raw)
To: dsterba, Omar Sandoval, linux-btrfs, kernel-team, Chris Mason,
Josef Bacik, David Sterba
On 22.05.2018 14:53, David Sterba wrote:
> On Fri, May 18, 2018 at 02:43:02PM -0700, Omar Sandoval wrote:
>> From: Omar Sandoval <osandov@fb.com>
>>
>> Jun Wu at Facebook reported that an internal service was seeing a return
>> value of 1 from ftruncate() on Btrfs when compression is enabled. This
>> is coming from the NEED_TRUNCATE_BLOCK return value from
>> btrfs_truncate_inode_items().
>>
>> btrfs_truncate() uses two variables for error handling, ret and err (if
>> this sounds familiar, it's because btrfs_truncate_inode_items() does
>> something similar).
>
> This err/ret pattern is widely used in btrfs code and IIRC mostly
> consistently, where err is the global return and ret is local.
And as a matter of fact it's a bad pattern, precisely because of the
type of errors this and one of the patches in the orphan items fix.
Everytime I hear "global state" I cringe, since it just opens avenues
for bugs.
>
>> When btrfs_truncate_inode_items() returns non-zero,
>> we set err to the return value, but we don't reset it to zero in the
>> successful NEED_TRUNCATE_BLOCK case. We only have err because we don't
>> want to mask an error if we call btrfs_update_inode() and
>> btrfs_end_transaction(), so let's make that its own scoped return
>> variable and use ret everywhere else.
>
> That's an alternative pattern when 'ret' is the global return and ret2
> are locals. Either way, it would be good to unify that.
FWIW I prefer the style put forth in this patch, since it's easier to
reason about ret2 because it's defined in a narrower context (the single
if (trans)) which helps spot problems more easily.
>
>> Fixes: ddfae63cc8e0 ("btrfs: move btrfs_truncate_block out of trans handle")
>> Reported-by: Jun Wu <quark@fb.com>
>> Signed-off-by: Omar Sandoval <osandov@fb.com>
>> ---
>> This is based on Linus' master rather than my orphan ENOSPC fixes
>> because I think we want to get this in for v4.17 and stable, and rebase
>> my fixes on top of this.
>
> For a 4.17-rc the patch would need to be smaller. You describe the
> actual bug as a missing reset of the err, so if it's just that, we can
> consider that for 4.17, but the patch as it is changes the error value
> propagatin in not so trivial way.
> --
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs: fix error handling in btrfs_truncate()
2018-05-22 12:11 ` Nikolay Borisov
@ 2018-05-22 12:54 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2018-05-22 12:54 UTC (permalink / raw)
To: Nikolay Borisov
Cc: dsterba, Omar Sandoval, linux-btrfs, kernel-team, Chris Mason,
Josef Bacik, David Sterba
On Tue, May 22, 2018 at 03:11:04PM +0300, Nikolay Borisov wrote:
>
>
> On 22.05.2018 14:53, David Sterba wrote:
> > On Fri, May 18, 2018 at 02:43:02PM -0700, Omar Sandoval wrote:
> >> From: Omar Sandoval <osandov@fb.com>
> >>
> >> Jun Wu at Facebook reported that an internal service was seeing a return
> >> value of 1 from ftruncate() on Btrfs when compression is enabled. This
> >> is coming from the NEED_TRUNCATE_BLOCK return value from
> >> btrfs_truncate_inode_items().
> >>
> >> btrfs_truncate() uses two variables for error handling, ret and err (if
> >> this sounds familiar, it's because btrfs_truncate_inode_items() does
> >> something similar).
> >
> > This err/ret pattern is widely used in btrfs code and IIRC mostly
> > consistently, where err is the global return and ret is local.
>
> And as a matter of fact it's a bad pattern, precisely because of the
> type of errors this and one of the patches in the orphan items fix.
> Everytime I hear "global state" I cringe, since it just opens avenues
> for bugs.
If we do the single point of return in the function, then the single
global (function-scope) ret value makes sense.
Arguably one can do the same sort of mistakes with the ret/ret2 pattern,
we still need to correctly propagate the local to the function-scope, so
switching is not strictly neccessary and not an all-cure.
> >> When btrfs_truncate_inode_items() returns non-zero,
> >> we set err to the return value, but we don't reset it to zero in the
> >> successful NEED_TRUNCATE_BLOCK case. We only have err because we don't
> >> want to mask an error if we call btrfs_update_inode() and
> >> btrfs_end_transaction(), so let's make that its own scoped return
> >> variable and use ret everywhere else.
> >
> > That's an alternative pattern when 'ret' is the global return and ret2
> > are locals. Either way, it would be good to unify that.
>
> FWIW I prefer the style put forth in this patch, since it's easier to
> reason about ret2 because it's defined in a narrower context (the single
> if (trans)) which helps spot problems more easily.
Yeah, consistency POV, keeping the meaning of 'ret' same everywhere
would be better, and we can add any number of ret2, ret3 if we have more
nested contexts.
I did a quick and dirty grep where 'err' is used as the function-scope
and it's in low-tens, so I think it's doable.
^ permalink raw reply [flat|nested] 6+ messages in thread