| Message ID | e59e9745f1c052ae0df24378455ad3e74eed7f3a.1526679483.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- 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 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; > } > > /* > -- 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 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. -- 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 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 > -- 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 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. -- 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/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; } /*