Re: [RFC - PATCH] xfs: hold xfs_buf locked between shortform->leaf conversion and the addition of the first attribute

Darrick J. Wong
2017-08-07T18:10:44Z

On Mon, Aug 07, 2017 at 01:26:09PM -0400, Brian Foster wrote:
> On Mon, Aug 07, 2017 at 09:37:44AM -0700, Darrick J. Wong wrote:
> > On Mon, Aug 07, 2017 at 11:16:54AM -0400, Brian Foster wrote:
> > > On Mon, Aug 07, 2017 at 03:54:03PM +0300, Alex Lyakas wrote:
> > > > 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.
> > > >
> > > > More info at:
> > > > https://www.spinics.net/lists/linux-xfs/msg08778.html
> > > >
> ...
> >
> > > > fs/xfs/libxfs/xfs_attr.c | 24 +++++++++++++++++++++---
> > > > fs/xfs/libxfs/xfs_attr_leaf.c | 5 ++++-
> > > > fs/xfs/libxfs/xfs_attr_leaf.h | 2 +-
> > > > 3 files changed, 26 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > > index 353fb42..c0ced12 100644
> > > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > > +++ b/fs/xfs/libxfs/xfs_attr.c
> ...
> > > > @@ -328,48 +329,65 @@ xfs_attr_set(
> ...
> > > > if (error) {
> > > > ASSERT(committed);
> > > > + if (leaf_bp)
> > > > + xfs_buf_relse(leaf_bp);
> > >
> > > Hmm, I don't think this is right. We don't want to release the buffer
> > > while the transaction still has a reference. Perhaps we should move this
> > > to the "out:" patch after the transaction cancel (and make sure the
> > > pointer is reset at the appropriate place)..?
> >
> > I would've thought that the buffer would be relse'd by xfs_trans_cancel,
> > having been bjoined to the first args.trans and bheld to any subsequent
> > transactions?

(Ugh, I can't follow myself either. Sorry for the confusing question.)

> I'm not following what you mean. I guess if the transaction has been
> committed by xfs_bmap_finish(), then the buffer is actually not in the
> current transaction, but I think we need to release it either way (so
> perhaps this code is fine).

I'm confused and going to step through through this code one piece at a time.

/*
* 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);

/* hold the leaf buffer locked, when "args.trans" transaction commits */
if (leaf_bp)
xfs_trans_bhold(args.trans, leaf_bp);

If we hit an error in xfs_attr_shortform_to_leaf leaf_bp will have been
bjoined to args.trans by xfs_attr_shortform_to_leaf ->
xfs_attr3_leaf_create -> xfs_da_get_buf -> xfs_trans_get_buf_map, so
when we goto out in the error handling clause below, we'll trans_cancel,
which releases leaf_bp, right? And we can't just xfs_buf_relse because
that would unlock a buffer that's still owned by args.trans (like Brian
said)...

if (!error)
error = xfs_defer_finish(&args.trans, args.dfops, dp);

...but if we hit an error here, we'll have committed the transaction but
held on to the locked buffer, so we /do/ need to xfs_buf_relse...

if (error) {
args.trans = NULL;
xfs_defer_cancel(&dfops);
goto out;
}

...in which case we no longer can use this error handling clause for
both cases because we have differing ownership of leaf_bp. In effect we
need something structured like this, I think?

xfs_defer_init()
error = xfs_attr_shortform_to_leaf(...);
if (error) {
xfs_defer_cancel(...);
goto out;
}
if (leaf_bp)
xfs_trans_bhold(args.trans, leaf_bp);

error = xfs_defer_finish(...);
if (error) {
xfs_defer_cancel(...);
if (leaf_bp)
xfs_brelse(leaf_bp);
goto out;
}

...and then when we're ready to add the new attr:

if (leaf_bp)
xfs_trans_bjoin(args.trans, leaf_bp);
error = xfs_attr_*_addname(...);

>
> > >
> > > > args.trans = NULL;
> > > > xfs_bmap_cancel(&flist);
> > > > goto out;
> > > > }
> > > >
> > > > /*
> > > > * bmap_finish() may have committed the last trans and started
> > > > * a new one. We need the inode to be in all transactions.
> > > > + * We also need to rejoin the leaf buffer to the new transaction
> > > > + * and prevent it from being unlocked, before we commit it in
> > > > xfs_trans_roll().
> > > > + * If bmap_finish() did not commit, then we are in the same
> > > > transaction,
> > > > + * and no need to call xfs_trans_bhold() again.
> > > > */
> > > > - if (committed)
> > > > + if (committed) {
> > > > xfs_trans_ijoin(args.trans, dp, 0);
> > > > + xfs_trans_bjoin(args.trans, leaf_bp);
> > > > + xfs_trans_bhold(args.trans, leaf_bp);
> > >
> > > I don't see why this is necessary. We still have the buffer held and
> > > locked, yes?
> >
> > The bhold means that after the transaction roll leaf_bp is still locked
> > but not associated with a transaction, so it is necessary to bjoin
> > leaf_bp to the new transaction. Since the very next thing we do is roll
> > the transaction again, we also want to bhold it for the second roll.
> >
>
> Ok, so we've created the empty block, logged it in the first
> transaction, marked it held and committed the tx. At that point the
> buffer can make it to the AIL, but we still have it locked (and held) so
> it cannot be written back. What difference does it make whether the
> buffer is added to this transaction? It won't be dirtied in the
> transaction, it can't be written back either way and we eventually join
> it to the transaction that creates the attr (after the roll).

Yes, you're right, leaf_bp is held by us and not the transaction, and we
don't need to touch leaf_bp for this empty roll, so we neither need to
bjoin it nor bhold it here.

> > (Question: is the second roll even necessary for this case?)
> >
>
> It's probably moot because I believe this code goes away in recent
> kernels as the committed thing was cleaned out, probably before we
> converted this over to xfs_defer_finish().

It's still there, though it's probably moot and benign.

> ...
> > > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > > > index b7cd0a0..2e03c32 100644
> > > > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > > > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> ...
> > > > @@ -733,20 +734,22 @@ xfs_attr_shortform_to_leaf(xfs_da_args_t *args)
> > > > ASSERT(error == -ENOATTR);
> > > > error = xfs_attr3_leaf_add(bp, &nargs);
> > > > ASSERT(error != -ENOSPC);
> > > > if (error)
> > > > goto out;
> > > > sfe = XFS_ATTR_SF_NEXTENTRY(sfe);
> > > > }
> > > > error = 0;
> > > >
> > > > out:
> > > > + if (error == 0)
> > > > + *bpp = bp;
> > >
> > > It looks like the only way error == 0 is where it is set just above, so
> > > we could probably move this before the label.
> > >
> > > I'm also wondering whether it might be cleaner to 1.) conditionally
> > > return the buffer when sf->hdr.count == 0 because that is the only case
> > > where the problem occurs and 2.) do the trans_bhold() here in
> > > shortform_to_leaf() when we do actually return it.
> >
> > I /think/ you could get rid of that second roll and do the bhold in
> > shortform_to_leaf, though you'd still have to re-bjoin it after
> > defer_finish.
> >
>
> Yeah, the semantic is basically to return a buffer iff the callee held
> it in the tx because it was empty (maybe reneame leaf_bp to empty_bp or
> something more clear?), thereby making the caller responsible to release
> it. The caller happens to do that by joining it to the transaction that
> will add the attr.

<nod>

--D

>
> Brian
>
> > --D
> >
> > >
> > > Brian
> > >
> > > > kmem_free(tmpbuffer);
> > > > return error;
> > > > }
> > > >
> > > > /*
> > > > * Check a leaf attribute block to see if all the entries would fit into
> > > > * a shortform attribute list.
> > > > */
> > > > int
> > > > xfs_attr_shortform_allfit(
> > > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
> > > > index 4f3a60a..d58e9e4 100644
> > > > --- a/fs/xfs/libxfs/xfs_attr_leaf.h
> > > > +++ b/fs/xfs/libxfs/xfs_attr_leaf.h
> > > > @@ -41,21 +41,21 @@ typedef struct xfs_attr_inactive_list {
> > > > * Function prototypes for the kernel.
> > > > *========================================================================*/
> > > >
> > > > /*
> > > > * Internal routines when attribute fork size < XFS_LITINO(mp).
> > > > */
> > > > 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, xfs_buf_t
> > > > **bpp);
> > > > int xfs_attr_shortform_remove(struct xfs_da_args *args);
> > > > int xfs_attr_shortform_list(struct xfs_attr_list_context *context);
> > > > int xfs_attr_shortform_allfit(struct xfs_buf *bp, struct xfs_inode *dp);
> > > > int xfs_attr_shortform_bytesfit(xfs_inode_t *dp, int bytes);
> > > > void xfs_attr_fork_remove(struct xfs_inode *ip, struct xfs_trans *tp);
> > > >
> > > > /*
> > > > * Internal routines when attribute fork size == XFS_LBSIZE(mp).
> > > > */
> > > > int xfs_attr3_leaf_to_node(struct xfs_da_args *args);
> > > > --
> > > > 1.9.1
> > > >
> > > > --
> > > > 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
> > > --
> > > 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
> --
> 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