| Message ID | 417f039ffc4db265a98214c8f86e9a36dbfb1c31.1710857863.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | trivial adjustments for return variable coding style | expand |
On Tue, Mar 19, 2024 at 08:25:27PM +0530, Anand Jain wrote: > In this function, the variable err is used as the second return value. If > it is not zero, rename it to ret2. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> This is fine but terrible, a good follow up cleanup would be to make btrfs_end_transaction() a void and remove the random places we check for an error. We don't really need it to tell use we have EROFS, we'll get it in some other operation down the line if we didn't get it somewhere in here. Thanks, Josef
On Tue, Mar 19, 2024 at 02:07:48PM -0400, Josef Bacik wrote: > On Tue, Mar 19, 2024 at 08:25:27PM +0530, Anand Jain wrote: > > In this function, the variable err is used as the second return value. If > > it is not zero, rename it to ret2. > > > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > > This is fine but terrible, a good follow up cleanup would be to make > btrfs_end_transaction() a void and remove the random places we check for an > error. We don't really need it to tell use we have EROFS, we'll get it in some > other operation down the line if we didn't get it somewhere in here. Thanks, I'm not sure we can ignore it everywhere, in many places it's ok, namely after transaction abort but if some control flow depends on the return value and starts doing updates that would hit the abort much later then I'd rather keep it. Examples I found: btrfs_delete_subvolume() line 4611, restore dead root status back btrfs_ioctl_qgroup_assign() if end_transaction fails we don't have any way to communicate errors from the ioctl btrfs_ioctl_qgroup_create() same btrfs_ioctl_qgroup_limit() same ... and there are similar cases It's not just EROFS but also the error code of transaction abort. I'd rather see the failures detected as early as possible, the delayed error would only become confusing to debug. We can have 2 versions so it's semantically clear which one is supposed to be handled and leave one void for the majority of cases.
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 030262943190..6f8747c907d5 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -4223,7 +4223,8 @@ static noinline_for_stack int mark_garbage_root(struct btrfs_root *root) { struct btrfs_fs_info *fs_info = root->fs_info; struct btrfs_trans_handle *trans; - int ret, err; + int ret; + int ret2; trans = btrfs_start_transaction(fs_info->tree_root, 0); if (IS_ERR(trans)) @@ -4236,9 +4237,10 @@ static noinline_for_stack int mark_garbage_root(struct btrfs_root *root) ret = btrfs_update_root(trans, fs_info->tree_root, &root->root_key, &root->root_item); - err = btrfs_end_transaction(trans); - if (err) - return err; + ret2 = btrfs_end_transaction(trans); + if (ret2) + return ret2; + return ret; }
In this function, the variable err is used as the second return value. If it is not zero, rename it to ret2. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/relocation.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)