Re: [PATCH 19/29] btrfs: mark_garbage_root rename err to ret2

David Sterba
2024-03-22T02:36:39Z

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.