| Message ID | ef45ecc23ffac44258794dfebaedd30e2db27a45.1686672418.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: do not BUG_ON after failure to migrate space during truncation | expand |
On 2023/6/14 00:07, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > During truncation we reserve 2 metadata units when starting a transaction > (reserved space goes to fs_info->trans_block_rsv) and then attempt to > migrate 1 unit (min_size bytes) from fs_info->trans_block_rsv into the > local block reserve. If we ever fail we trigger a BUG_ON(), which should > never happen, because we reserved 2 units. However if we happen to fail > for some reason, we don't need to be so dire and hit a BUG_ON(), we can > just error out the truncation and, since this is highly unexpected, > surround the error check with WARN_ON(), to get an informative stack > trace and tag the branh as 'unlikely'. Also make the 'min_size' variable > const, since it's not supposed to ever change and any accidental change > could possibly make the space migration not so unlikely to fail. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/inode.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 094664d9262b..c57623cb1a78 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -8335,7 +8335,7 @@ static int btrfs_truncate(struct btrfs_inode *inode, bool skip_writeback) > int ret; > struct btrfs_trans_handle *trans; > u64 mask = fs_info->sectorsize - 1; > - u64 min_size = btrfs_calc_metadata_size(fs_info, 1); > + const u64 min_size = btrfs_calc_metadata_size(fs_info, 1); > > if (!skip_writeback) { > ret = btrfs_wait_ordered_range(&inode->vfs_inode, > @@ -8392,7 +8392,15 @@ static int btrfs_truncate(struct btrfs_inode *inode, bool skip_writeback) > /* Migrate the slack space for the truncate to our reserve */ > ret = btrfs_block_rsv_migrate(&fs_info->trans_block_rsv, rsv, > min_size, false); > - BUG_ON(ret); > + /* > + * We have reserved 2 metadata units when we started the transaction and > + * min_size matches 1 unit, so this should never fail, but if it does, > + * it's not critical we just fail truncation. > + */ > + if (WARN_ON(ret)) { > + btrfs_end_transaction(trans); > + goto out; > + } > > trans->block_rsv = rsv; > > @@ -8440,7 +8448,14 @@ static int btrfs_truncate(struct btrfs_inode *inode, bool skip_writeback) > btrfs_block_rsv_release(fs_info, rsv, -1, NULL); > ret = btrfs_block_rsv_migrate(&fs_info->trans_block_rsv, > rsv, min_size, false); > - BUG_ON(ret); /* shouldn't happen */ > + /* > + * We have reserved 2 metadata units when we started the > + * transaction and min_size matches 1 unit, so this should never > + * fail, but if it does, it's not critical we just fail truncation. > + */ > + if (WARN_ON(ret)) > + break; > + > trans->block_rsv = rsv; > } >
On Tue, Jun 13, 2023 at 05:07:54PM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > During truncation we reserve 2 metadata units when starting a transaction > (reserved space goes to fs_info->trans_block_rsv) and then attempt to > migrate 1 unit (min_size bytes) from fs_info->trans_block_rsv into the > local block reserve. If we ever fail we trigger a BUG_ON(), which should > never happen, because we reserved 2 units. However if we happen to fail > for some reason, we don't need to be so dire and hit a BUG_ON(), we can > just error out the truncation and, since this is highly unexpected, > surround the error check with WARN_ON(), to get an informative stack > trace and tag the branh as 'unlikely'. Also make the 'min_size' variable > const, since it's not supposed to ever change and any accidental change > could possibly make the space migration not so unlikely to fail. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Added to misc-next, thanks.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 094664d9262b..c57623cb1a78 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -8335,7 +8335,7 @@ static int btrfs_truncate(struct btrfs_inode *inode, bool skip_writeback) int ret; struct btrfs_trans_handle *trans; u64 mask = fs_info->sectorsize - 1; - u64 min_size = btrfs_calc_metadata_size(fs_info, 1); + const u64 min_size = btrfs_calc_metadata_size(fs_info, 1); if (!skip_writeback) { ret = btrfs_wait_ordered_range(&inode->vfs_inode, @@ -8392,7 +8392,15 @@ static int btrfs_truncate(struct btrfs_inode *inode, bool skip_writeback) /* Migrate the slack space for the truncate to our reserve */ ret = btrfs_block_rsv_migrate(&fs_info->trans_block_rsv, rsv, min_size, false); - BUG_ON(ret); + /* + * We have reserved 2 metadata units when we started the transaction and + * min_size matches 1 unit, so this should never fail, but if it does, + * it's not critical we just fail truncation. + */ + if (WARN_ON(ret)) { + btrfs_end_transaction(trans); + goto out; + } trans->block_rsv = rsv; @@ -8440,7 +8448,14 @@ static int btrfs_truncate(struct btrfs_inode *inode, bool skip_writeback) btrfs_block_rsv_release(fs_info, rsv, -1, NULL); ret = btrfs_block_rsv_migrate(&fs_info->trans_block_rsv, rsv, min_size, false); - BUG_ON(ret); /* shouldn't happen */ + /* + * We have reserved 2 metadata units when we started the + * transaction and min_size matches 1 unit, so this should never + * fail, but if it does, it's not critical we just fail truncation. + */ + if (WARN_ON(ret)) + break; + trans->block_rsv = rsv; }