Re: [PATCH 03/21] xfs: Add attibute set and helper functions

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

On 05/07/2018 04:36 PM, Darrick J. Wong wrote:
> On Sun, May 06, 2018 at 10:24:36AM -0700, Allison Henderson wrote:
> > This patch adds xfs_attr_set_args and xfs_bmap_set_attrforkoff.
> > These sub-routines set the attributes specified in @args.
> > We will use this later for setting parent pointers as a deferred
> > attribute operation.
> >
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > ---
> >   fs/xfs/libxfs/xfs_attr.c | 217 ++++++++++++++++++++++++++++-------------------
> >   fs/xfs/libxfs/xfs_attr.h |   2 +
> >   fs/xfs/libxfs/xfs_bmap.c |  49 ++++++-----
> >   fs/xfs/libxfs/xfs_bmap.h |   1 +
> >   4 files changed, 165 insertions(+), 104 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 0ade22b..99c4a31 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -168,6 +168,134 @@ xfs_attr_get(
> >   }
> >   
> >   /*
> > + * Set the attribute specified in @args. In the case of the parent attribute
> > + * being set, we do not want to roll the transaction on shortform-to-leaf
> > + * conversion, as the attribute must be added in the same transaction as the
> > + * parent directory modifications. Hence @roll_trans needs to be set
> > + * appropriately to control whether the transaction is committed during this
> > + * function.
> > + */
> > +int
> > +xfs_attr_set_args(
> > +	struct xfs_da_args	*args,
> > +	int			flags,
> > +	struct xfs_buf          *leaf_bp,
> > +	bool			roll_trans)
> > +{
> > +	struct xfs_inode	*dp = args->dp;
> > +	struct xfs_mount        *mp = dp->i_mount;
> > +	int			error = 0;
> > +	int			err2 = 0;
> > +	int			sf_size;
> > +
> > +	/*
> > +	 * New inodes setting the parent pointer attr will
> > +	 * not have an attribute fork yet. So set the attribute
> > +	 * fork appropriately
> > +	 */
> > +	if (XFS_IFORK_Q((args->dp)) == 0) {
> > +		sf_size = sizeof(struct xfs_attr_sf_hdr) +
> > +		     XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen);
> > +		xfs_bmap_set_attrforkoff(args->dp, sf_size, NULL);
> > +		args->dp->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP);
> > +		args->dp->i_afp->if_flags = XFS_IFEXTENTS;
> > +	}
> > +
> > +	xfs_trans_ijoin(args->trans, dp, 0);
> > +	/*
> > +	 * If the attribute list is non-existent or a shortform list,
> > +	 * upgrade it to a single-leaf-block attribute list.
> > +	 */
> > +	if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> > +	    (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> > +	     dp->i_d.di_anextents == 0)) {
> > +
> > +		/*
> > +		 * Build initial attribute list (if required).
> > +		 */
> > +		if (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS)
> > +			xfs_attr_shortform_create(args);
> > +
> > +		/*
> > +		 * Try to add the attr to the attribute list in the inode.
> > +		 */
> > +		error = xfs_attr_shortform_addname(args, roll_trans);
> > +		if (error != -ENOSPC) {
> > +			if (roll_trans) {
>
> I dislike this roll_trans parameter.  Most other places in xfs when a
> function is passed in a defer_ops or a transaction it's assumed that we
> don't own the transaction or the defer_ops and so while it's ok to
> attach dirty things to the dfops or the tp, we let the caller decide
> when it's appropriate to start committing things.
>
> This function is getting rather long and indenty, can it be broken up
> into smaller pieces?  That should make it easier to reuse the core
> logic of "try to stuff it in the sfattr, if it doesn't fit then convert
> to attr block and retry the add" without having to add extra parameters
> to control whether or not we commit transactions.
>
> This is more complex than in other parts of xfs because we're (for the
> moment anyway) leaving both the deferred and non-deferred paths, but at
> least the attr logic and the transaction management logic should be
> split into separate functions to handle the unique situations of both
> the deferred and non-deferred xattr setting code.
>
> Also, please don't hoist code into a helper function /and/ change its
> behavior & parameters in the same patch.

Sure, I'll see if I can split it up a little more to make it easier to 
follow.  Sorry, about the hoist and hijack... a lot of this comes from
collecting fixes on top of the set, and then moving them down to an
appropriate patch.  The roll_trans and related probably should have gone
to the set below.  I think there's more discussion concerning roll_trans 
in the next patch, so I'll jump that way....

> --D
>
> > +				/*
> > +				 * Commit the shortform mods, and we're done.
> > +				 * NOTE: this is also the error path (EEXIST, etc).
> > +				 */
> > +				ASSERT(args->trans != NULL);
> > +
> > +				/*
> > +				 * If this is a synchronous mount, make sure that
> > +				 * the transaction goes to disk before returning
> > +				 * to the user.
> > +				 */
> > +				if (mp->m_flags & XFS_MOUNT_WSYNC)
> > +					xfs_trans_set_sync(args->trans);
> > +
> > +				if (!error && (flags & ATTR_KERNOTIME) == 0)
> > +					xfs_trans_ichgtime(args->trans, dp,
> > +							   XFS_ICHGTIME_CHG);
> > +
> > +				err2 = xfs_trans_commit(args->trans);
> > +				error = error ? error : err2;
> > +			}
> > +			goto out;
> > +		}
> > +
> > +		/*
> > +		 * It won't fit in the shortform, transform to a leaf block.
> > +		 * GROT: another possible req'mt for a double-split btree op.
> > +		 */
> > +		error = xfs_attr_shortform_to_leaf(args, &leaf_bp);
> > +		if (error)
> > +			goto out;
> > +
> > +		xfs_defer_bjoin(args->dfops, leaf_bp);
> > +		xfs_defer_ijoin(args->dfops, dp);
> > +		if (roll_trans) {
> > +			/*
> > +			 * Prevent the leaf buffer from being unlocked so that a
> > +			 * concurrent AIL push cannot grab the half-baked leaf
> > +			 * buffer and run into problems with the write verifier.
> > +			 */
> > +			xfs_trans_bhold(args->trans, leaf_bp);
> > +
> > +			error = xfs_defer_finish(&args->trans, args->dfops);
> > +			if (error) {
> > +				args->trans = NULL;
> > +				goto out;
> > +			}
> > +
> > +			/*
> > +			 * Commit the leaf transformation.  We'll need another
> > +			 * (linked) transaction to add the new attribute to the
> > +			 * leaf.
> > +			 */
> > +			error = xfs_trans_roll_inode(&args->trans, dp);
> > +			if (error)
> > +				goto out;
> > +			xfs_defer_ijoin(args->dfops, dp);
> > +			xfs_trans_bjoin(args->trans, leaf_bp);
> > +				leaf_bp = NULL;
> > +		}
> > +	}
> > +
> > +	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > +		error = xfs_attr_leaf_addname(args, roll_trans);
> > +	else
> > +		error = xfs_attr_node_addname(args, roll_trans);
> > +	if (error)
> > +		goto out;
> > +
> > +out:
> > +	return error;
> > +}
> > +
> > +/*
> >    * Calculate how many blocks we need for the new attribute,
> >    */
> >   STATIC int
> > @@ -218,7 +346,7 @@ xfs_attr_set(
> >   	struct xfs_trans_res	tres;
> >   	xfs_fsblock_t		firstblock;
> >   	int			rsvd = (flags & ATTR_ROOT) != 0;
> > -	int			error, err2, local;
> > +	int			error, local;
> >   
> >   	XFS_STATS_INC(mp, xs_attr_set);
> >   
> > @@ -279,88 +407,11 @@ xfs_attr_set(
> >   
> >   	xfs_trans_ijoin(args.trans, dp, 0);
> >   
> > -	/*
> > -	 * If the attribute list is non-existent or a shortform list,
> > -	 * upgrade it to a single-leaf-block attribute list.
> > -	 */
> > -	if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> > -	    (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> > -	     dp->i_d.di_anextents == 0)) {
> > -
> > -		/*
> > -		 * Build initial attribute list (if required).
> > -		 */
> > -		if (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS)
> > -			xfs_attr_shortform_create(&args);
> > -
> > -		/*
> > -		 * Try to add the attr to the attribute list in
> > -		 * the inode.
> > -		 */
> > -		error = xfs_attr_shortform_addname(&args, true);
> > -		if (error != -ENOSPC) {
> > -			/*
> > -			 * Commit the shortform mods, and we're done.
> > -			 * NOTE: this is also the error path (EEXIST, etc).
> > -			 */
> > -			ASSERT(args.trans != NULL);
> > -
> > -			/*
> > -			 * If this is a synchronous mount, make sure that
> > -			 * the transaction goes to disk before returning
> > -			 * to the user.
> > -			 */
> > -			if (mp->m_flags & XFS_MOUNT_WSYNC)
> > -				xfs_trans_set_sync(args.trans);
> > -
> > -			if (!error && (flags & ATTR_KERNOTIME) == 0) {
> > -				xfs_trans_ichgtime(args.trans, dp,
> > -							XFS_ICHGTIME_CHG);
> > -			}
> > -			err2 = xfs_trans_commit(args.trans);
> > -			xfs_iunlock(dp, XFS_ILOCK_EXCL);
> > -
> > -			return error ? error : err2;
> > -		}
> > -
> > -		/*
> > -		 * It won't fit in the shortform, transform to a leaf block.
> > -		 * GROT: another possible req'mt for a double-split btree op.
> > -		 */
> > -		xfs_defer_init(args.dfops, args.firstblock);
> > -		error = xfs_attr_shortform_to_leaf(&args, &leaf_bp);
> > -		if (error)
> > -			goto out_defer_cancel;
> > -		/*
> > -		 * Prevent the leaf buffer from being unlocked so that a
> > -		 * concurrent AIL push cannot grab the half-baked leaf
> > -		 * buffer and run into problems with the write verifier.
> > -		 */
> > -		xfs_trans_bhold(args.trans, leaf_bp);
> > -		xfs_defer_bjoin(args.dfops, leaf_bp);
> > -		xfs_defer_ijoin(args.dfops, dp);
> > -		error = xfs_defer_finish(&args.trans, args.dfops);
> > -		if (error)
> > -			goto out_defer_cancel;
> > -
> > -		/*
> > -		 * Commit the leaf transformation.  We'll need another (linked)
> > -		 * transaction to add the new attribute to the leaf, which
> > -		 * means that we have to hold & join the leaf buffer here too.
> > -		 */
> > -		error = xfs_trans_roll_inode(&args.trans, dp);
> > -		if (error)
> > -			goto out;
> > -		xfs_trans_bjoin(args.trans, leaf_bp);
> > -		leaf_bp = NULL;
> > -	}
> > +	xfs_defer_init(args.dfops, args.firstblock);
> > +	error = xfs_attr_set_args(&args, flags, leaf_bp, true);
> >   
> > -	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > -		error = xfs_attr_leaf_addname(&args, true);
> > -	else
> > -		error = xfs_attr_node_addname(&args, true);
> >   	if (error)
> > -		goto out;
> > +		goto out_defer_cancel;
> >   
> >   	/*
> >   	 * If this is a synchronous mount, make sure that the
> > @@ -369,9 +420,6 @@ xfs_attr_set(
> >   	if (mp->m_flags & XFS_MOUNT_WSYNC)
> >   		xfs_trans_set_sync(args.trans);
> >   
> > -	if ((flags & ATTR_KERNOTIME) == 0)
> > -		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
> > -
> >   	/*
> >   	 * Commit the last in the sequence of transactions.
> >   	 */
> > @@ -383,7 +431,6 @@ xfs_attr_set(
> >   
> >   out_defer_cancel:
> >   	xfs_defer_cancel(&dfops);
> > -out:
> >   	if (leaf_bp)
> >   		xfs_trans_brelse(args.trans, leaf_bp);
> >   	if (args.trans)
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index d07bf27..b5dc02c 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -152,6 +152,8 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name,
> >   		 unsigned char *value, int *valuelenp, int flags);
> >   int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name,
> >   		 unsigned char *value, int valuelen, int flags);
> > +int xfs_attr_set_args(struct xfs_da_args *args, int flags,
> > +			struct xfs_buf *leaf_bp, bool roll_trans);
> >   int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags);
> >   int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
> >   		  int flags, struct attrlist_cursor_kern *cursor);
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 6a7c2f0..4e16a5d 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -1031,6 +1031,34 @@ xfs_bmap_add_attrfork_local(
> >   	return -EFSCORRUPTED;
> >   }
> >   
> > +/* Set an inode attr fork off based on the format */
> > +int
> > +xfs_bmap_set_attrforkoff(
> > +	struct xfs_inode	*ip,
> > +	int			size,
> > +	int			*version)
> > +{
> > +	switch (ip->i_d.di_format) {
> > +	case XFS_DINODE_FMT_DEV:
> > +		ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
> > +		break;
> > +	case XFS_DINODE_FMT_LOCAL:
> > +	case XFS_DINODE_FMT_EXTENTS:
> > +	case XFS_DINODE_FMT_BTREE:
> > +		ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size);
> > +		if (!ip->i_d.di_forkoff)
> > +			ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> > +		else if ((ip->i_mount->m_flags & XFS_MOUNT_ATTR2) && version)
> > +			*version = 2;
> > +		break;
> > +	default:
> > +		ASSERT(0);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   /*
> >    * Convert inode from non-attributed to attributed.
> >    * Must not be in a transaction, ip must not be locked.
> > @@ -1084,26 +1112,9 @@ xfs_bmap_add_attrfork(
> >   
> >   	xfs_trans_ijoin(tp, ip, 0);
> >   	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > -
> > -	switch (ip->i_d.di_format) {
> > -	case XFS_DINODE_FMT_DEV:
> > -		ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
> > -		break;
> > -	case XFS_DINODE_FMT_LOCAL:
> > -	case XFS_DINODE_FMT_EXTENTS:
> > -	case XFS_DINODE_FMT_BTREE:
> > -		ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size);
> > -		if (!ip->i_d.di_forkoff)
> > -			ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> > -		else if (mp->m_flags & XFS_MOUNT_ATTR2)
> > -			version = 2;
> > -		break;
> > -	default:
> > -		ASSERT(0);
> > -		error = -EINVAL;
> > +	error = xfs_bmap_set_attrforkoff(ip, size, &version);
> > +	if (error)
> >   		goto trans_cancel;
> > -	}
> > -
> >   	ASSERT(ip->i_afp == NULL);
> >   	ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP);
> >   	ip->i_afp->if_flags = XFS_IFEXTENTS;
> > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> > index 2b766b3..50e9115 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.h
> > +++ b/fs/xfs/libxfs/xfs_bmap.h
> > @@ -191,6 +191,7 @@ void	xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno,
> >   		xfs_filblks_t len);
> >   void	xfs_trim_extent_eof(struct xfs_bmbt_irec *, struct xfs_inode *);
> >   int	xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd);
> > +int	xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version);
> >   void	xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork);
> >   void	xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
> >   			  xfs_fsblock_t bno, xfs_filblks_t len,
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html