On Wed, Oct 18, 2017 at 03:55:17PM -0700, Allison Henderson wrote:
> These sub-routines set or remove 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 | 322 +++++++++++++++++++++++++++--------------------
> fs/xfs/xfs_attr.h | 2 +
> 2 files changed, 189 insertions(+), 135 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 6249c92..b00ec1f 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -203,6 +203,185 @@ xfs_attr_calc_size(
> return nblks;
> }
>
> +/*
> + * 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.
Hmm... shouldn't the deferred attribute set code take care of all the
conversions/attr fork expansions/whatever is necessary to cram in the
parent pointer? So in theory all the parent pointer updates should look
like this:
xfs_defer_init(&dfops...);
xfs_some_name_creating_operation(tp, ...);
if (hasparent) {
xfs_parent_set(tp, ip);
}
xfs_defer_finish(&tp, &dfops);
xfs_trans_commit(tp);
xfs_parent_set() would then be:
if (fits in inode) {
xfs_attr_set_sf(ip, key, value);
xfs_log_attr_area();
return;
}
xfs_attr_do_conversions_if_needed(dfops);
xfs_attr_set(dfops);
...and if there's a really good reason to try to cram things in, we can
add those later? As I scanned the series, that was what kept coming up
in my head -- just tell the xfs_parent.c code to set a parent pointer;
it can figure out if there's sufficient space to put it directly into
the inode and log that, or we need something else that actually requires
the deferred ops mechanism then do that.
(Maybe I'm just fixating on xfs_parent_create...)
Higher level questions about robustness: if we try to set a parent
ptr and the attr name already exists, do we error out? If we try to
remove a parent ptr and there's no attr name, do we error out? I think
you've enough here to start thinking what changes need to be made to
xfs_repair to validate & fix the parent pointers, and what test cases
(and possibly ioctls) will need to be constructed to verify that we
actually get the parent pointers we want.
--D
> + */
> +int
> +xfs_attr_set_args(
> + struct xfs_da_args *args,
> + int flags,
> + bool roll_trans)
> +{
> + struct xfs_inode *dp = args->dp;
> + struct xfs_mount *mp = dp->i_mount;
> + struct xfs_trans_res tres;
> + int rsvd = 0;
> + int error = 0;
> +
> + tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> + M_RES(mp)->tr_attrsetrt.tr_logres * args->total;
> + tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> + tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> +
> + /*
> + * Root fork attributes can use reserved data blocks for this
> + * operation if necessary
> + */
> + error = xfs_trans_alloc(mp, &tres, args->total, 0,
> + rsvd ? XFS_TRANS_RESERVE : 0, &args->trans);
> + if (error)
> + goto out;
> +
> + error = xfs_trans_reserve_quota_nblks(args->trans, dp, args->total, 0,
> + rsvd ? XFS_QMOPT_RES_REGBLKS |
> + XFS_QMOPT_FORCE_RES :
> + XFS_QMOPT_RES_REGBLKS);
> + if (error)
> + goto out;
> +
> + 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);
> + if (error != -ENOSPC) {
> + ASSERT(args->trans);
> + if (!error && (flags & ATTR_KERNOTIME) == 0)
> + xfs_trans_ichgtime(args->trans, dp,
> + XFS_ICHGTIME_CHG);
> + 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);
> + if (error)
> + goto out;
> + xfs_defer_ijoin(args->dfops, dp);
> + if (roll_trans) {
> + 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;
> + }
> + }
> +
> + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> + error = xfs_attr_leaf_addname(args);
> + else
> + error = xfs_attr_node_addname(args);
> + if (error)
> + goto out;
> +
> + if ((flags & ATTR_KERNOTIME) == 0)
> + xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
> +
> + xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE);
> +out:
> + return error;
> +}
> +
> +/*
> + * Remove the attribute specified in @args.
> + */
> +int
> +xfs_attr_remove_args(
> + struct xfs_da_args *args,
> + int flags)
> +{
> + struct xfs_inode *dp = args->dp;
> + struct xfs_mount *mp = dp->i_mount;
> + int error;
> + int rsvd = 0;
> +
> + error = xfs_qm_dqattach_locked(dp, 0);
> + if (error)
> + return error;
> +
> + /*
> + * Root fork attributes can use reserved data blocks for this
> + * operation if necessary
> + */
> + if (flags & ATTR_ROOT)
> + rsvd = XFS_TRANS_RESERVE;
> + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrrm,
> + XFS_ATTRRM_SPACE_RES(mp), 0, rsvd, &args->trans);
> +
> + if (error)
> + goto out;
> +
> + /*
> + * No need to make quota reservations here. We expect to release some
> + * blocks not allocate in the common case.
> + */
> + xfs_trans_ijoin(args->trans, dp, 0);
> +
> + if (!xfs_inode_hasattr(dp)) {
> + error = -ENOATTR;
> + } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> + ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
> + error = xfs_attr_shortform_remove(args);
> + } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> + error = xfs_attr_leaf_removename(args);
> + } else {
> + error = xfs_attr_node_removename(args);
> + }
> +
> + if (error)
> + goto out;
> +
> + /*
> + * 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 ((flags & ATTR_KERNOTIME) == 0)
> + xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
> +
> + xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE);
> +
> + return error;
> +
> +out:
> + if (args->trans)
> + xfs_trans_cancel(args->trans);
> +
> + return error;
> +}
> +
> int
> xfs_attr_set(
> struct xfs_inode *dp,
> @@ -214,10 +393,9 @@ xfs_attr_set(
> struct xfs_mount *mp = dp->i_mount;
> struct xfs_da_args args;
> struct xfs_defer_ops dfops;
> - 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);
>
> @@ -252,106 +430,11 @@ xfs_attr_set(
> return error;
> }
>
> - tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> - M_RES(mp)->tr_attrsetrt.tr_logres * args.total;
> - tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> - tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> -
> - /*
> - * Root fork attributes can use reserved data blocks for this
> - * operation if necessary
> - */
> - error = xfs_trans_alloc(mp, &tres, args.total, 0,
> - rsvd ? XFS_TRANS_RESERVE : 0, &args.trans);
> - if (error)
> - return error;
> -
> xfs_ilock(dp, XFS_ILOCK_EXCL);
> - error = xfs_trans_reserve_quota_nblks(args.trans, dp, args.total, 0,
> - rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
> - XFS_QMOPT_RES_REGBLKS);
> - if (error) {
> - xfs_iunlock(dp, XFS_ILOCK_EXCL);
> - xfs_trans_cancel(args.trans);
> - return error;
> - }
> -
> - 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);
> - 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);
> - if (error)
> - goto out_defer_cancel;
> - 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.
> - */
> -
> - error = xfs_trans_roll_inode(&args.trans, dp);
> - if (error)
> - goto out;
> -
> - }
> -
> - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> - error = xfs_attr_leaf_addname(&args);
> - else
> - error = xfs_attr_node_addname(&args);
> + xfs_defer_init(args.dfops, args.firstblock);
> + error = xfs_attr_set_args(&args, flags, true);
> if (error)
> - goto out;
> + goto out_defer_cancel;
>
> /*
> * If this is a synchronous mount, make sure that the
> @@ -360,9 +443,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.
> */
> @@ -374,10 +454,6 @@ xfs_attr_set(
>
> out_defer_cancel:
> xfs_defer_cancel(&dfops);
> - args.trans = NULL;
> -out:
> - if (args.trans)
> - xfs_trans_cancel(args.trans);
> xfs_iunlock(dp, XFS_ILOCK_EXCL);
> return error;
> }
> @@ -417,38 +493,15 @@ xfs_attr_remove(
> */
> args.op_flags = XFS_DA_OP_OKNOENT;
>
> - error = xfs_qm_dqattach(dp, 0);
> - if (error)
> - return error;
> -
> - /*
> - * Root fork attributes can use reserved data blocks for this
> - * operation if necessary
> - */
> - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrrm,
> - XFS_ATTRRM_SPACE_RES(mp), 0,
> - (flags & ATTR_ROOT) ? XFS_TRANS_RESERVE : 0,
> - &args.trans);
> - if (error)
> - return error;
> -
> xfs_ilock(dp, XFS_ILOCK_EXCL);
> /*
> * No need to make quota reservations here. We expect to release some
> * blocks not allocate in the common case.
> */
> xfs_trans_ijoin(args.trans, dp, 0);
> + xfs_defer_init(args.dfops, args.firstblock);
>
> - if (!xfs_inode_hasattr(dp)) {
> - error = -ENOATTR;
> - } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> - ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
> - error = xfs_attr_shortform_remove(&args);
> - } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> - error = xfs_attr_leaf_removename(&args);
> - } else {
> - error = xfs_attr_node_removename(&args);
> - }
> + error = xfs_attr_remove_args(&args, flags);
>
> if (error)
> goto out;
> @@ -460,9 +513,6 @@ xfs_attr_remove(
> 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.
> */
> @@ -473,6 +523,8 @@ xfs_attr_remove(
> return error;
>
> out:
> + xfs_defer_cancel(&dfops);
> +
> if (args.trans)
> xfs_trans_cancel(args.trans);
> xfs_iunlock(dp, XFS_ILOCK_EXCL);
> diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
> index 5d5a5e2..8542606 100644
> --- a/fs/xfs/xfs_attr.h
> +++ b/fs/xfs/xfs_attr.h
> @@ -149,7 +149,9 @@ 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, bool roll_trans);
> int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags);
> +int xfs_attr_remove_args(struct xfs_da_args *args, int flags);
> int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
> int flags, struct attrlist_cursor_kern *cursor);
>
> --
> 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
