Re: [PATCH v24 06/11] xfs: Add xfs_attr_set_deferred and xfs_attr_remove_deferred

[Allison Henderson <allison.henderson@oracle.com>]

On 8/31/21 8:47 PM, Dave Chinner wrote:
> On Tue, Aug 24, 2021 at 03:44:29PM -0700, Allison Henderson wrote:
>> From: Allison Collins <allison.henderson@oracle.com>
>>
>> These routines set up and queue a new deferred attribute operations.
>> These functions are meant to be called by any routine needing to
>> initiate a deferred attribute operation as opposed to the existing
>> inline operations. New helper function xfs_attr_item_init also added.
>>
>> Finally enable delayed attributes in xfs_attr_set and xfs_attr_remove.
>>
>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> .....
>>   
>> +STATIC int
>> +xfs_attr_item_init(
>> +	struct xfs_da_args	*args,
>> +	unsigned int		op_flags,	/* op flag (set or remove) */
>> +	struct xfs_attr_item	**attr)		/* new xfs_attr_item */
>> +{
>> +
>> +	struct xfs_attr_item	*new;
>> +
>> +	new = kmem_zalloc(sizeof(struct xfs_attr_item), KM_NOFS);
> 
> In transaction context here so we don't need KM_NOFS.
ok, will remove

> 
>> +	new->xattri_op_flags = op_flags;
>> +	new->xattri_dac.da_args = args;
>> +
>> +	*attr = new;
>> +	return 0;
>> +}
> 
> Why doesn't this just return the object or NULL on allocation
> failure? What other error could it ever return?
I had adopted this function signature just to be consistent with other 
*_item_init routines at the time.  Mostly just trying to be uniform, 
though they may have changed since.  This patch spent some time on the 
back burner while we were more focused on the state machine refactoring.

> 
>> +
>> +/* Sets an attribute for an inode as a deferred operation */
>> +int
>> +xfs_attr_set_deferred(
>> +	struct xfs_da_args	*args)
>> +{
>> +	struct xfs_attr_item	*new;
>> +	int			error = 0;
>> +
>> +	error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new);
>> +	if (error)
>> +		return error;
> 
> i.e.
> 	attri = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET);
> 	if (!attri)
> 		return -ENOMEM;
> 
>> +
>> +	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
>> +
>> +	return 0;
>> +}
>> +
>> +/* Removes an attribute for an inode as a deferred operation */
>> +int
>> +xfs_attr_remove_deferred(
>> +	struct xfs_da_args	*args)
>> +{
>> +
>> +	struct xfs_attr_item	*new;
>> +	int			error;
>> +
>> +	error  = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new);
>> +	if (error)
>> +		return error;
>> +
>> +	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
>> +
>> +	return 0;
>> +}
> 
> We really should not use "new" as a variable name. As a general
> rule, the common pattern set by this file is that xfs_attri_item
> objects in a function are named "attri". Just because it's newly
> allocated doesn't mean we should use a different convention for
> naming xfs_attri_item objects...
> 
Ok, I had seen the pattern around and reused it.  Will change to attri

Allison

> Cheers,
> 
> Dave.
>