| Message ID | 497be2d1fd745d88d6cbeda5d77168781b5522df.1606938211.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup error handling in relocation | expand |
On 2020/12/3 上午3:51, Josef Bacik wrote: > We already handle some errors in this function, and the callers do the > correct error handling, so clean up the rest of the function to do the > appropriate error handling. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/relocation.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 8f4f1e21c770..bcced4e436af 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -3634,10 +3634,15 @@ struct inode *create_reloc_inode(struct btrfs_fs_info *fs_info, > goto out; > > err = __insert_orphan_inode(trans, root, objectid); > - BUG_ON(err); > + if (err) > + goto out; > > inode = btrfs_iget(fs_info->sb, objectid, root); > - BUG_ON(IS_ERR(inode)); > + if (IS_ERR(inode)) { When error happens here, we have already inserted an inode item into the data reloc root, without the orphan item to clean it up. It won't cause any problem, since we have u64 to store almost endless inodes in a mostly empty tree. But I guess we'd still better try to delete the inserted inode item, or data reloc tree may one day become a landfill with all those inode items. Thanks, Qu > + err = PTR_ERR(inode); > + inode = NULL; > + goto out; > + } > BTRFS_I(inode)->index_cnt = group->start; > > err = btrfs_orphan_add(trans, BTRFS_I(inode)); >
On 12/3/20 12:25 AM, Qu Wenruo wrote: > > > On 2020/12/3 上午3:51, Josef Bacik wrote: >> We already handle some errors in this function, and the callers do the >> correct error handling, so clean up the rest of the function to do the >> appropriate error handling. >> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com> >> --- >> fs/btrfs/relocation.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c >> index 8f4f1e21c770..bcced4e436af 100644 >> --- a/fs/btrfs/relocation.c >> +++ b/fs/btrfs/relocation.c >> @@ -3634,10 +3634,15 @@ struct inode *create_reloc_inode(struct btrfs_fs_info *fs_info, >> goto out; >> >> err = __insert_orphan_inode(trans, root, objectid); >> - BUG_ON(err); >> + if (err) >> + goto out; >> >> inode = btrfs_iget(fs_info->sb, objectid, root); >> - BUG_ON(IS_ERR(inode)); >> + if (IS_ERR(inode)) { > > When error happens here, we have already inserted an inode item into the > data reloc root, without the orphan item to clean it up. > > It won't cause any problem, since we have u64 to store almost endless > inodes in a mostly empty tree. > > But I guess we'd still better try to delete the inserted inode item, or > data reloc tree may one day become a landfill with all those inode items. > Yeah we shouldn't be in the business of leaving random artifacts around, I'll fix this up. Thanks, Josef
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 8f4f1e21c770..bcced4e436af 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -3634,10 +3634,15 @@ struct inode *create_reloc_inode(struct btrfs_fs_info *fs_info, goto out; err = __insert_orphan_inode(trans, root, objectid); - BUG_ON(err); + if (err) + goto out; inode = btrfs_iget(fs_info->sb, objectid, root); - BUG_ON(IS_ERR(inode)); + if (IS_ERR(inode)) { + err = PTR_ERR(inode); + inode = NULL; + goto out; + } BTRFS_I(inode)->index_cnt = group->start; err = btrfs_orphan_add(trans, BTRFS_I(inode));
We already handle some errors in this function, and the callers do the correct error handling, so clean up the rest of the function to do the appropriate error handling. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/relocation.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)