Re: [PATCH 03/17] Add xfs_attr_set_defered and xfs_attr_remove_defered

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Allison Henderson <allison.henderson@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 03/17] Add xfs_attr_set_defered and xfs_attr_remove_defered
Date: Wed, 11 Oct 2017 21:38:09 -0700	[thread overview]
Message-ID: <20171012043809.GE7122@magnolia> (raw)
In-Reply-To: <1507327548-3221-4-git-send-email-allison.henderson@oracle.com>

On Fri, Oct 06, 2017 at 03:05:34PM -0700, Allison Henderson wrote:

Needs changelog... why are we doing this?

> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
> :100644 100644 5325ec2... 5c9b604... M	fs/xfs/libxfs/xfs_attr.c
> :100644 100644 06c4081... 67b0fbf... M	fs/xfs/xfs_attr.h
>  fs/xfs/libxfs/xfs_attr.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_attr.h        |  5 +++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 5325ec2..5c9b604 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -458,6 +458,37 @@ xfs_attr_set(
>  	return error;
>  }
>  
> +int
> +xfs_attr_set_defered(

"deferred"

> +	struct xfs_inode	*dp,
> +	struct xfs_defer_ops    *dfops,
> +	const unsigned char	*name,
> +	int			namelen,
> +	unsigned char		*value,
> +	int			valuelen,

unsigned int valuelen?  I don't see a need for negative lengths. :)

> +	int			flags)
> +{
> +
> +	struct xfs_attr_item     *new;
> +
> +	ASSERT(name != NULL);
> +	ASSERT(value != NULL);

It's probably more valuable to assert on namelen/valuelen because if
these pointers are null we'll crash when get to the memcpy.

> +
> +	new = kmem_alloc(sizeof(struct xfs_attr_item), KM_SLEEP);

Hmm.  At first I saw the KM_SLEEP and wondered why we'd sleep in
transaction context with an inode (presumably) locked, but xfs_defer_add
does that too.  Then again they could both be wrong.

If nothing else we've the inode locked so I think this also needs
KM_NOFS so that we don't recurse into fs reclaim and deadlock trying to
free up memory related to this inode.`

(Same applies to the next function.)

--D

> +	memset(new, 0, sizeof(struct xfs_attr_item));
> +	new->xattri_ino = dp->i_ino;
> +	new->xattri_op_flags = ATTR_OP_FLAGS_SET;
> +	new->xattri_name_len = namelen;
> +	new->xattri_nameval_len = namelen + valuelen;
> +	new->xattri_flags = flags;
> +	memcpy(new->xattri_nameval, name, namelen);
> +	memcpy(&new->xattri_nameval[namelen], value, valuelen);
> +
> +	xfs_defer_add(dfops, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> +
> +	return 0;
> +}
> +
>  /*
>   * Generic handler routine to remove a name from an attribute list.
>   * Transitions attribute list from Btree to shortform as necessary.
> @@ -531,6 +562,33 @@ xfs_attr_remove(
>  	return error;
>  }
>  
> +int
> +xfs_attr_remove_defered(
> +	struct xfs_inode        *dp,
> +	struct xfs_defer_ops    *dfops,
> +	const unsigned char     *name,
> +	int			namelen,
> +	int                     flags)
> +{
> +
> +	struct xfs_attr_item     *new;
> +
> +	ASSERT(name != NULL);
> +
> +	new = kmem_alloc(sizeof(struct xfs_attr_item), KM_SLEEP);
> +	memset(new, 0, sizeof(struct xfs_attr_item));
> +	new->xattri_ino = dp->i_ino;
> +	new->xattri_op_flags = ATTR_OP_FLAGS_REMOVE;
> +	new->xattri_name_len = namelen;
> +	new->xattri_nameval_len = namelen;
> +	new->xattri_flags = flags;
> +	memcpy(new->xattri_nameval, name, namelen);
> +
> +	xfs_defer_add(dfops, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> +
> +	return 0;
> +}
> +
>  /*========================================================================
>   * External routines when attribute list is inside the inode
>   *========================================================================*/
> diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
> index 06c4081..67b0fbf 100644
> --- a/fs/xfs/xfs_attr.h
> +++ b/fs/xfs/xfs_attr.h
> @@ -176,5 +176,10 @@ int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
>  int xfs_attr_args_init(struct xfs_da_args *args, struct xfs_inode *dp,
>  		       const unsigned char *name, int flags);
>  int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
> +int xfs_attr_set_defered(struct xfs_inode *dp, struct xfs_defer_ops *dfops,
> +			 const unsigned char *name, int name_len,
> +			 unsigned char *value, int valuelen, int flags);
> +int xfs_attr_remove_defered(struct xfs_inode *dp, struct xfs_defer_ops *dfops,
> +			    const unsigned char *name, int namelen, int flags);
>  
>  #endif	/* __XFS_ATTR_H__ */
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html