On Tue, Nov 21, 2017 at 04:31:20PM +0100, Libor Klepáč wrote:
> Hello again,
> i'm sorry to bug you, but i would like to ask, if this patch made it to kernel
> yet?
> I was looking on pull request messages and did not see it.
>
> We were struck by "Metadata corruption detected at
> xfs_attr3_leaf_write_verify" under high load, after months without problem.
>
> But we are still on 4.9.30 on this server, there is possibility to upgrade to
> 4.9.51 from backports.
>
IIRC, the issue that prevented this patch from being merged was that the
buffer needed to be joined and relogged across deferred operations. That
had a dependency on some rectoring of the buffer logging code, which has
since been merged, but I don't recall seeing anything regarding a
deferred op buffer relogging mechanism and using it here. Alex?
Brian
> With regards,
> Libor
>
>
> On středa 9. srpna 2017 13:06:12 CET alex@xxxxxxxxxxxxxxxxx wrote:
> > From: Alex Lyakas <alex@xxxxxxxxxxxxxxxxx>
> >
> > The new attribute leaf buffer is not held locked across
> > the transaction roll between the shortform->leaf modification
> > and the addition of the new entry. As a result, the attribute
> > buffer modification being made is not atomic from
> > an operational perspective. Hence the AIL push can grab it in
> > the transient state of "just created" after the initial
> > transaction is rolled, because the buffer has been released.
> > This leads to xfs_attr3_leaf_verify() asserting that
> > hdr.count is zero, treating this as in-memory corruption,
> > and shutting down the filesystem.
> >
> > Signed-off-by: Alex Lyakas <alex@xxxxxxxxxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_attr.c | 19 ++++++++++++++++++-
> > fs/xfs/libxfs/xfs_attr_leaf.c | 4 +++-
> > fs/xfs/libxfs/xfs_attr_leaf.h | 3 ++-
> > 3 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index de7b9bd..982e322 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -216,10 +216,11 @@
> > struct xfs_defer_ops dfops;
> > struct xfs_trans_res tres;
> > xfs_fsblock_t firstblock;
> > int rsvd = (flags & ATTR_ROOT) != 0;
> > int error, err2, local;
> > + struct xfs_buf *leaf_bp = NULL;
> >
> > XFS_STATS_INC(mp, xs_attr_set);
> >
> > if (XFS_FORCED_SHUTDOWN(dp->i_mount))
> > return -EIO;
> > @@ -325,11 +326,17 @@
> > /*
> > * 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);
> > + error = xfs_attr_shortform_to_leaf(&args, &leaf_bp);
> > + /*
> > + * Prevent the leaf buffer from being unlocked
> > + * when "args.trans" transaction commits.
> > + */
> > + if (leaf_bp)
> > + xfs_trans_bhold(args.trans, leaf_bp);
> > if (!error)
> > error = xfs_defer_finish(&args.trans, args.dfops, dp);
> > if (error) {
> > args.trans = NULL;
> > xfs_defer_cancel(&dfops);
> > @@ -343,10 +350,18 @@
> >
> > error = xfs_trans_roll(&args.trans, dp);
> > if (error)
> > goto out;
> >
> > + /*
> > + * Rejoin the leaf buffer to the new transaction.
> > + * This allows a subsequent read to find the buffer in the
> > + * transaction (and avoid a deadlock).
> > + */
> > + xfs_trans_bjoin(args.trans, leaf_bp);
> > + /* Prevent from being released at the end of the function */
> > + leaf_bp = NULL;
> > }
> >
> > if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > error = xfs_attr_leaf_addname(&args);
> > else
> > @@ -374,10 +389,12 @@
> > return error;
> >
> > out:
> > if (args.trans)
> > xfs_trans_cancel(args.trans);
> > + if (leaf_bp)
> > + xfs_buf_relse(leaf_bp);
> > xfs_iunlock(dp, XFS_ILOCK_EXCL);
> > return error;
> > }
> >
> > /*
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > index c6c15e5..ab73e4b 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > @@ -738,13 +738,14 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args
> *args,
> > return -ENOATTR;
> > }
> >
> > /*
> > * Convert from using the shortform to the leaf.
> > + * Upon success, return the leaf buffer.
> > */
> > int
> > -xfs_attr_shortform_to_leaf(xfs_da_args_t *args)
> > +xfs_attr_shortform_to_leaf(xfs_da_args_t *args, struct xfs_buf **bpp)
> > {
> > xfs_inode_t *dp;
> > xfs_attr_shortform_t *sf;
> > xfs_attr_sf_entry_t *sfe;
> > xfs_da_args_t nargs;
> > @@ -820,10 +821,11 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args
> *args,
> > if (error)
> > goto out;
> > sfe = XFS_ATTR_SF_NEXTENTRY(sfe);
> > }
> > error = 0;
> > + *bpp = bp;
> >
> > out:
> > kmem_free(tmpbuffer);
> > return error;
> > }
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
> > index f7dda0c..2b3c69df 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.h
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.h
> > @@ -46,11 +46,12 @@
> > */
> > void xfs_attr_shortform_create(struct xfs_da_args *args);
> > void xfs_attr_shortform_add(struct xfs_da_args *args, int forkoff);
> > int xfs_attr_shortform_lookup(struct xfs_da_args *args);
> > int xfs_attr_shortform_getvalue(struct xfs_da_args *args);
> > -int xfs_attr_shortform_to_leaf(struct xfs_da_args *args);
> > +int xfs_attr_shortform_to_leaf(struct xfs_da_args *args,
> > + struct xfs_buf **bpp);
> > int xfs_attr_shortform_remove(struct xfs_da_args *args);
> > int xfs_attr_shortform_allfit(struct xfs_buf *bp, struct xfs_inode *dp);
> > int xfs_attr_shortform_bytesfit(struct xfs_inode *dp, int bytes);
> > void xfs_attr_fork_remove(struct xfs_inode *ip, struct xfs_trans *tp);
> >
> >
>
>
> --
> 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
