On Fri, Apr 12, 2019 at 03:50:35PM -0700, Allison Henderson wrote:
> This patch modifies xfs_attr_set_args to return -EAGAIN
> when a transaction needs to be rolled. All functions
> currently calling xfs_attr_set_args are modified to use
> the deferred attr operation, or handle the -EAGAIN return
> code
>
> Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_attr.c | 62 ++++++++++++++++++++++++++++++++++++++++--------
> fs/xfs/libxfs/xfs_attr.h | 2 +-
> fs/xfs/xfs_attr_item.c | 41 +++++++++++++++++++++++++++-----
> fs/xfs/xfs_trans.h | 2 ++
> fs/xfs/xfs_trans_attr.c | 56 +++++++++++++++++++++++++------------------
> 5 files changed, 123 insertions(+), 40 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 0042708..4ddd86b 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -236,10 +236,37 @@ int
> xfs_attr_set_args(
> struct xfs_da_args *args,
> struct xfs_buf **leaf_bp,
> + enum xfs_attr_state *state,
> bool roll_trans)
> {
> struct xfs_inode *dp = args->dp;
> int error = 0;
> + int sf_size;
> +
> + switch (*state) {
> + case (XFS_ATTR_STATE1):
> + goto state1;
> + case (XFS_ATTR_STATE2):
> + goto state2;
> + case (XFS_ATTR_STATE3):
> + goto state3;
> + }
I still don't understand what these there states are, though evidently
if we get to this line then we weren't in any of the three possible
states?
XFS_ATTR_STATE_INIT...?
> +
> + /*
> + * New inodes may 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;
> + }
> +
> + *state = XFS_ATTR_STATE1;
XFS_ATTR_STATE_ADDED_FORK...
> + return -EAGAIN;
> +state1:
>
> /*
> * If the attribute list is non-existent or a shortform list,
> @@ -248,7 +275,6 @@ xfs_attr_set_args(
> 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).
> */
> @@ -262,6 +288,9 @@ xfs_attr_set_args(
> if (error != -ENOSPC)
> return error;
>
> + *state = XFS_ATTR_STATE2;
XFS_ATTR_STATE_FAILED_SF_ADD...
> + return -EAGAIN;
> +state2:
> /*
> * It won't fit in the shortform, transform to a leaf block.
> * GROT: another possible req'mt for a double-split btree op.
> @@ -270,14 +299,14 @@ xfs_attr_set_args(
> if (error)
> return error;
>
> - 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);
> + /*
> + * 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);
>
> + if (roll_trans) {
> error = xfs_defer_finish(&args->trans);
> if (error)
> return error;
> @@ -293,6 +322,12 @@ xfs_attr_set_args(
> xfs_trans_bjoin(args->trans, *leaf_bp);
> *leaf_bp = NULL;
> }
> +
> + *state = XFS_ATTR_STATE3;
XFS_ATTR_STATE_LEAF_AVAIL...
> + return -EAGAIN;
> +state3:
> + if (*leaf_bp != NULL)
> + xfs_trans_brelse(args->trans, *leaf_bp);
> }
>
> if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> @@ -419,7 +454,9 @@ xfs_attr_set(
> goto out_trans_cancel;
>
> xfs_trans_ijoin(args.trans, dp, 0);
> - error = xfs_attr_set_args(&args, &leaf_bp, true);
> +
> + error = xfs_attr_set_deferred(dp, args.trans, name, namelen,
> + value, valuelen, flags);
Oh, I see, the XFS_ATTR_STATE[1-3] added in the previous patch are
supposed to record restart points when we have to duck out to roll a
transaction or something.
Hmm, why does this have to happen? Is it because the current attr
setting code will allocate and commit transactions, but now that we have
deferred attr items, each of those commits has to turn into backing out
to whomever allocated the transaction to get another?
Oh right, there's that whole mess where the log recovery transaction
isn't supposed to be rolled or committed, ever, so that the defer ops
can be ripped off and run after the recovered items are all more or less
written out. Ugh.
Uh... meeting time, I'll think about this and continue this reply later.
--D
> if (error)
> goto out_release_leaf;
> if (!args.trans) {
> @@ -554,8 +591,13 @@ xfs_attr_remove(
> */
> xfs_trans_ijoin(args.trans, dp, 0);
>
> - error = xfs_attr_remove_args(&args, true);
> + error = xfs_has_attr(&args);
> + if (error)
> + goto out;
> +
>
> + error = xfs_attr_remove_deferred(dp, args.trans,
> + name, namelen, flags);
> if (error)
> goto out;
>
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 4ce3b0a..da95e69 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -181,7 +181,7 @@ int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name,
> size_t namelen, unsigned char *value, int valuelen,
> int flags);
> int xfs_attr_set_args(struct xfs_da_args *args, struct xfs_buf **leaf_bp,
> - bool roll_trans);
> + enum xfs_attr_state *state, bool roll_trans);
> int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name,
> size_t namelen, int flags);
> int xfs_has_attr(struct xfs_da_args *args);
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 36e6d1e..292d608 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -464,8 +464,11 @@ xfs_attri_recover(
> struct xfs_attri_log_format *attrp;
> struct xfs_trans_res tres;
> int local;
> - int error = 0;
> + int error, err2 = 0;
> int rsvd = 0;
> + enum xfs_attr_state state = 0;
> + struct xfs_buf *leaf_bp = NULL;
> +
>
> ASSERT(!test_bit(XFS_ATTRI_RECOVERED, &attrip->flags));
>
> @@ -540,14 +543,40 @@ xfs_attri_recover(
> xfs_ilock(ip, XFS_ILOCK_EXCL);
>
> xfs_trans_ijoin(args.trans, ip, 0);
> - error = xfs_trans_attr(&args, attrdp, attrp->alfi_op_flags);
> - if (error)
> - goto abort_error;
>
> + do {
> + leaf_bp = NULL;
> +
> + error = xfs_trans_attr(&args, attrdp, &leaf_bp, &state,
> + attrp->alfi_op_flags);
> + if (error && error != -EAGAIN)
> + goto abort_error;
> +
> + xfs_trans_log_inode(args.trans, ip,
> + XFS_ILOG_CORE | XFS_ILOG_ADATA);
> +
> + err2 = xfs_trans_commit(args.trans);
> + if (err2) {
> + error = err2;
> + goto abort_error;
> + }
> +
> + if (error == -EAGAIN) {
> + err2 = xfs_trans_alloc(mp, &tres, args.total, 0,
> + XFS_TRANS_PERM_LOG_RES, &args.trans);
> + if (err2) {
> + error = err2;
> + goto abort_error;
> + }
> + xfs_trans_ijoin(args.trans, ip, 0);
> + }
> +
> + } while (error == -EAGAIN);
> +
> + if (leaf_bp)
> + xfs_trans_brelse(args.trans, leaf_bp);
>
> set_bit(XFS_ATTRI_RECOVERED, &attrip->flags);
> - xfs_trans_log_inode(args.trans, ip, XFS_ILOG_CORE | XFS_ILOG_ADATA);
> - error = xfs_trans_commit(args.trans);
> xfs_iunlock(ip, XFS_ILOCK_EXCL);
> return error;
>
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 7bb9d8e..c785cd7 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -239,6 +239,8 @@ xfs_trans_get_attrd(struct xfs_trans *tp,
> struct xfs_attri_log_item *attrip);
> int xfs_trans_attr(struct xfs_da_args *args,
> struct xfs_attrd_log_item *attrdp,
> + struct xfs_buf **leaf_bp,
> + void *state,
> uint32_t attr_op_flags);
>
> int xfs_trans_commit(struct xfs_trans *);
> diff --git a/fs/xfs/xfs_trans_attr.c b/fs/xfs/xfs_trans_attr.c
> index 3679348..a3339ea 100644
> --- a/fs/xfs/xfs_trans_attr.c
> +++ b/fs/xfs/xfs_trans_attr.c
> @@ -56,10 +56,11 @@ int
> xfs_trans_attr(
> struct xfs_da_args *args,
> struct xfs_attrd_log_item *attrdp,
> + struct xfs_buf **leaf_bp,
> + void *state,
> uint32_t op_flags)
> {
> int error;
> - struct xfs_buf *leaf_bp = NULL;
>
> error = xfs_qm_dqattach_locked(args->dp, 0);
> if (error)
> @@ -68,7 +69,8 @@ xfs_trans_attr(
> switch (op_flags) {
> case XFS_ATTR_OP_FLAGS_SET:
> args->op_flags |= XFS_DA_OP_ADDNAME;
> - error = xfs_attr_set_args(args, &leaf_bp, false);
> + error = xfs_attr_set_args(args, leaf_bp,
> + (enum xfs_attr_state *)state, false);
> break;
> case XFS_ATTR_OP_FLAGS_REMOVE:
> ASSERT(XFS_IFORK_Q((args->dp)));
> @@ -78,11 +80,6 @@ xfs_trans_attr(
> error = -EFSCORRUPTED;
> }
>
> - if (error) {
> - if (leaf_bp)
> - xfs_trans_brelse(args->trans, leaf_bp);
> - }
> -
> /*
> * Mark the transaction dirty, even on error. This ensures the
> * transaction is aborted, which:
> @@ -184,27 +181,40 @@ xfs_attr_finish_item(
> char *name_value;
> int error;
> int local;
> - struct xfs_da_args args;
> + struct xfs_da_args *args;
>
> attr = container_of(item, struct xfs_attr_item, xattri_list);
> - name_value = ((char *)attr) + sizeof(struct xfs_attr_item);
> -
> - error = xfs_attr_args_init(&args, attr->xattri_ip, name_value,
> - attr->xattri_name_len, attr->xattri_flags);
> - if (error)
> - goto out;
> + args = &attr->xattri_args;
> +
> + if (attr->xattri_state == 0) {
> + /* Only need to initialize args context once */
> + name_value = ((char *)attr) + sizeof(struct xfs_attr_item);
> + error = xfs_attr_args_init(args, attr->xattri_ip, name_value,
> + attr->xattri_name_len,
> + attr->xattri_flags);
> + if (error)
> + goto out;
> +
> + args->hashval = xfs_da_hashname(args->name, args->namelen);
> + args->value = &name_value[attr->xattri_name_len];
> + args->valuelen = attr->xattri_value_len;
> + args->op_flags = XFS_DA_OP_OKNOENT;
> + args->total = xfs_attr_calc_size(args, &local);
> + attr->xattri_leaf_bp = NULL;
> + }
>
> - args.hashval = xfs_da_hashname(args.name, args.namelen);
> - args.value = &name_value[attr->xattri_name_len];
> - args.valuelen = attr->xattri_value_len;
> - args.op_flags = XFS_DA_OP_OKNOENT;
> - args.total = xfs_attr_calc_size(&args, &local);
> - args.trans = tp;
> + /*
> + * Always reset trans after EAGAIN cycle
> + * since the transaction is new
> + */
> + args->trans = tp;
>
> - error = xfs_trans_attr(&args, done_item,
> - attr->xattri_op_flags);
> + error = xfs_trans_attr(args, done_item, &attr->xattri_leaf_bp,
> + &attr->xattri_state, attr->xattri_op_flags);
> out:
> - kmem_free(attr);
> + if (error != -EAGAIN)
> + kmem_free(attr);
> +
> return error;
> }
>
> --
> 2.7.4
>
