| Message ID | 20230524150317.1767981-10-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/14] btrfs: optimize out btrfs_is_zoned for !CONFIG_BLK_DEV_ZONED | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On Wed, May 24, 2023 at 05:03:12PM +0200, Christoph Hellwig wrote: > The callers don't check the btrfs_finish_ordered_io return value, so > drop it. Same general comments like in https://lore.kernel.org/linux-btrfs/20230530150359.GS575@twin.jikos.cz/ "Function can return void if none of its callees return an error, directly or indirectly, there are no BUG_ONs left to be turned to proper error handling or there's no missing error handling" btrfs_finish_ordered_io mixes a few error handling styles, there's direct return -ERROR, transaction abort or mapping_set_error. Some called functions are not error handling everything propely and at least btrfs_free_reserved_extent() returns an error but is not handled. I'm not counting the state bit handlers (clear_extent_bit) as we know they "should not fail". unpin_extent_cache() does not look clean either. If 'callers don't check error values' the question is 'Should they?'.
On Tue, May 30, 2023 at 05:44:15PM +0200, David Sterba wrote: > On Wed, May 24, 2023 at 05:03:12PM +0200, Christoph Hellwig wrote: > > The callers don't check the btrfs_finish_ordered_io return value, so > > drop it. > > Same general comments like in > https://lore.kernel.org/linux-btrfs/20230530150359.GS575@twin.jikos.cz/ > > "Function can return void if none of its callees return an error, > directly or indirectly, there are no BUG_ONs left to be turned to > proper error handling or there's no missing error handling" > > btrfs_finish_ordered_io mixes a few error handling styles, there's > direct return -ERROR, transaction abort or mapping_set_error. Some > called functions are not error handling everything propely and at least > btrfs_free_reserved_extent() returns an error but is not handled. > > I'm not counting the state bit handlers (clear_extent_bit) as we know > they "should not fail". unpin_extent_cache() does not look clean either. > > If 'callers don't check error values' the question is 'Should they?'. The clear answer is no, as we're in an I/O completion handler where there is no one we could return them to. The errors are propagate through the mapping state.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 31124954ec6ecf..cee71eaec7cff9 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -3180,7 +3180,7 @@ static int insert_ordered_extent_file_extent(struct btrfs_trans_handle *trans, * an ordered extent if the range of bytes in the file it covers are * fully written. */ -int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent) +void btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent) { struct btrfs_inode *inode = BTRFS_I(ordered_extent->inode); struct btrfs_root *root = inode->root; @@ -3383,8 +3383,6 @@ int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent) btrfs_put_ordered_extent(ordered_extent); /* once for the tree */ btrfs_put_ordered_extent(ordered_extent); - - return ret; } void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode, diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h index 2a20017d9ec6f5..2c6efebd043c04 100644 --- a/fs/btrfs/ordered-data.h +++ b/fs/btrfs/ordered-data.h @@ -161,7 +161,7 @@ btrfs_ordered_inode_tree_init(struct btrfs_ordered_inode_tree *t) t->last = NULL; } -int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent); +void btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent); void btrfs_put_ordered_extent(struct btrfs_ordered_extent *entry); void btrfs_remove_ordered_extent(struct btrfs_inode *btrfs_inode,
The callers don't check the btrfs_finish_ordered_io return value, so drop it. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/inode.c | 4 +--- fs/btrfs/ordered-data.h | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-)