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

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 29, 2019 at 12:55:59AM -0700, Christoph Hellwig wrote:
> 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

Only one caller passes ATTR_ALLOC - the ACL code. The other two
have their own buffers that are supplied....

> 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.

Can't overwrite args->valuelen until we've done the ERANGE check.
Sure, I could put it in a local variable, but that doesn't reduce
the amount of code, or make it obvious that we intentionally return
the attribute size when the supplied buffer it too small...

> > +	/* 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.

ok.

> > +	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.

Fixed.

-- 
Dave Chinner
david@fromorbit.com