Re: [PATCH v4 15/17] xfs: Check for -ENOATTR or -EEXIST

[Brian Foster <bfoster@redhat.com>]

On Wed, Nov 06, 2019 at 06:27:59PM -0700, Allison Collins wrote:
> Delayed operations cannot return error codes.  So we must check for
> these conditions first before starting set or remove operations
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 5dcb19f..626d4a98 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -458,6 +458,27 @@ xfs_attr_set(
>  		goto out_trans_cancel;
>  
>  	xfs_trans_ijoin(args.trans, dp, 0);
> +
> +	error = xfs_has_attr(&args);
> +	if (error == -EEXIST) {
> +		if (name->type & ATTR_CREATE)
> +			goto out_trans_cancel;
> +		else
> +			name->type |= ATTR_REPLACE;
> +	}
> +
> +	if (error == -ENOATTR && (name->type & ATTR_REPLACE))
> +		goto out_trans_cancel;
> +
> +	if (name->type & ATTR_REPLACE) {
> +		name->type &= ~ATTR_REPLACE;
> +		error = xfs_attr_remove_args(&args);
> +		if (error)
> +			goto out_trans_cancel;
> +
> +		name->type |= ATTR_CREATE;
> +	}
> +

I see Darrick already commented on this.. I think the behavior of the
existing rename code is to essentially create the new xattr with the
INCOMPLETE flag set so we can roll transactions, etc. without any
observable behavior to userspace. Once the new xattr is fully in place,
the rename is performed atomically from the userspace perspective by
flipping the INCOMPLETE flag from the newly constructed xattr to the old
one and we can then remove the old xattr from there.

>  	error = xfs_attr_set_args(&args);
>  	if (error)
>  		goto out_trans_cancel;
> @@ -543,6 +564,10 @@ xfs_attr_remove(
>  	 */
>  	xfs_trans_ijoin(args.trans, dp, 0);
>  
> +	error = xfs_has_attr(&args);
> +	if (error == -ENOATTR)
> +		goto out;
> +

Wouldn't we want to return any error that might occur here (except
-EEXIST), not just -ENOATTR if there's actually no xattr?

Brian

>  	error = xfs_attr_remove_args(&args);
>  	if (error)
>  		goto out;
> -- 
> 2.7.4
>