Re: [PATCH 3/3] xfs: allocate xattr buffer on demand

From: Christoph Hellwig <hch@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 3/3] xfs: allocate xattr buffer on demand
Date: Thu, 29 Aug 2019 00:55:59 -0700	[thread overview]
Message-ID: <20190829075559.GC18966@infradead.org> (raw)
In-Reply-To: <20190828042350.6062-4-david@fromorbit.com>

On Wed, Aug 28, 2019 at 02:23:50PM +1000, Dave Chinner wrote:
> + * If ATTR_ALLOC is set in @flags, allocate the buffer for the value after
> + * existence of the attribute has been determined. On success, return that
> + * buffer to the caller and leave them to free it. On failure, free any
> + * allocated buffer and ensure the buffer pointer returned to the caller is
> + * null.

Given that all three callers pass ATTR_ALLOC, do we even need a flag
and keep the old behavior around, at least at the xfs_attr_get level?
For the lower level we still have scrub, but that fills out the args
structure directly.

> +static int
> +xfs_attr_copy_value(
> +	struct xfs_da_args	*args,
> +	unsigned char		*value,
> +	int			valuelen)
> +{
> +	/*
> +	 * No copy if all we have to do is get the length
> +	 */
> +	if (args->flags & ATTR_KERNOVAL) {
> +		args->valuelen = valuelen;
> +		return 0;
> +	}
> +
> +	/*
> +	 * No copy if the length of the existing buffer is too small
> +	 */
> +	if (args->valuelen < valuelen) {
> +		args->valuelen = valuelen;
> +		return -ERANGE;
> +	}
> +
> +	if (args->op_flags & XFS_DA_OP_ALLOCVAL) {
> +		args->value = kmem_alloc_large(valuelen, KM_SLEEP);
> +		if (!args->value)
> +			return -ENOMEM;
> +	}
> +	args->valuelen = valuelen;

Can't we just move the setting of ->valuelen up to common code shared
between all the branches?  That means it would also be set on an
allocation error, but that should be harmless.

> +	/* remote block xattr requires IO for copy-in */
> +	if (args->rmtblkno)
> +		return xfs_attr_rmtval_get(args);
> +
> +	/*
> +	 * This is to prevent a GCC warning because the remote xattr case
> +	 * doesn't have a value to pass in. In that case, we never reach here,
> +	 * but GCC can't work that out and so throws a "passing NULL to
> +	 * memcpy" warning.
> +	 */
> +	if (!value)
> +		return -EINVAL;
> +	memcpy(args->value, value, valuelen);
> +	return 0;
> +}

Can you split creating this helper into a separate prep patch?  While
not strictly required it would make reviewing what is consolidation
vs what is new code for the on-demand buffer allocation a little easier.

> +	error = xfs_attr_get(ip, ea_name, (unsigned char **)&xfs_acl, &len,
> +				ATTR_ALLOC|ATTR_ROOT);

Please keep space between the symbols and | on each side.