Re: [PATCH v11 01/25] xfs: Add xfs_has_attr and subroutines

Allison Collins
2020-07-25T02:49:51Z

On 7/23/20 7:24 PM, Dave Chinner wrote:
> On Tue, Jul 21, 2020 at 04:26:13PM -0700, Darrick J. Wong wrote:
>> On Mon, Jul 20, 2020 at 05:15:42PM -0700, Allison Collins wrote:
>>> This patch adds a new functions to check for the existence of an
>>> attribute. Subroutines are also added to handle the cases of leaf
>>> blocks, nodes or shortform. Common code that appears in existing attr
>>> add and remove functions have been factored out to help reduce the
>>> appearance of duplicated code. We will need these routines later for
>>> delayed attributes since delayed operations cannot return error codes.
>>>
>>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>>> Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
>>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>>
>> Looks good enough for now... I still dislike generating ENOATTR/EEXIST
>> deep in the folds of the attr code but that's probably a bigger thing to
>> be wrangled with later. (And tbh I've thought about this & haven't come
>> up with a better idea anyway :P)
>
> Yes, I agree it is hard to read, but I do think there's a cleaner
> way of doing this. Take, for example, xfs_attr_leaf_try_add(). It
> looks like this:
>
> /*
> * Look up the given attribute in the leaf block. Figure out if
> * the given flags produce an error or call for an atomic rename.
> */
> retval = xfs_attr_leaf_hasname(args, &bp);
> if (retval != -ENOATTR && retval != -EEXIST)
> return retval;
> if (retval == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
> goto out_brelse;
> if (retval == -EEXIST) {
> if (args->attr_flags & XATTR_CREATE)
> goto out_brelse;
>
> trace_xfs_attr_leaf_replace(args);
>
> /* save the attribute state for later removal*/
> args->op_flags |= XFS_DA_OP_RENAME; /* an atomic rename */
> xfs_attr_save_rmt_blk(args);
>
> /*
> * clear the remote attr state now that it is saved so that the
> * values reflect the state of the attribute we are about to
> * add, not the attribute we just found and will remove later.
> */
> args->rmtblkno = 0;
> args->rmtblkcnt = 0;
> args->rmtvaluelen = 0;
> }
>
> /*
> * Add the attribute to the leaf block
> */
> return xfs_attr3_leaf_add(bp, args);
>
> out_brelse:
> xfs_trans_brelse(args->trans, bp);
> return retval;
> }
>
>
> I agree, the error handling is messy and really hard to follow.
> But if we write it like this:
>
> /*
> * Look up the given attribute in the leaf block. Figure out if
> * the given flags produce an error or call for an atomic rename.
> */
> retval = xfs_attr_leaf_hasname(args, &bp);
> switch (retval) {
> case -ENOATTR:
> if (args->attr_flags & XATTR_REPLACE)
> goto out_brelse;
> break;
> case -EEXIST:
> if (args->attr_flags & XATTR_CREATE)
> goto out_brelse;
>
> trace_xfs_attr_leaf_replace(args);
>
> /* save the attribute state for later removal*/
> args->op_flags |= XFS_DA_OP_RENAME; /* an atomic rename */
> xfs_attr_save_rmt_blk(args);
>
> /*
> * clear the remote attr state now that it is saved so that the
> * values reflect the state of the attribute we are about to
> * add, not the attribute we just found and will remove later.
> */
> args->rmtblkno = 0;
> args->rmtblkcnt = 0;
> args->rmtvaluelen = 0;
> break;
> case 0:
> break;
> default:
> return retval;
> }
>
> /*
> * Add the attribute to the leaf block
> */
> return xfs_attr3_leaf_add(bp, args);
>
> out_brelse:
> xfs_trans_brelse(args->trans, bp);
> return retval;
> }
>
> The logic is *much* cleaner and it is not overly verbose, either.
> This sort of change could be done at the end of the series, too,
> rather than requiring a rebase of everything....

Sure, I know this one had quite a bit of ping pong before it landed
where it did. I am not opposed to later rearranging it as long as the
underlying mechanics are the same. :-)

Allison
>
> Cheers,
>
> Dave.
>