Re: [PATCH 2/2] xfs: fix double ijoin in xfs_reflink_cancel_cow_range

[Dave Chinner <david@fromorbit.com>]

On Wed, Mar 07, 2018 at 05:17:25AM -0800, Christoph Hellwig wrote:
> On Wed, Mar 07, 2018 at 08:10:20PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > AN inode is joined to teh same transaction twice in
> > xfs_reflink_cancel_cow_range() resulting in the following assert
> > failure:
> 
> Needs some major spelling love :)

Yeah, I sent it out as soon as I realised it was more than just an
isolated occurrence. Needs updating...

> > [   30.180485] XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
> 
> That assertations seems like something that only exists locally in your
> tree.  Any chance to send it out with this series?

It's in the first patch. I've got to revise it, anyway, because I
missed the xfs_trans_brelease() case that removes the log item from
the transaction and that throws a false positive in the rmap
finishing code.

> The other option would be to make xfs_trans_ijoin a no-op if the inode
> is already joined, except that this wouldn't work with the magic unlock
> on commit. 

I'd much prefer we catch all the places where we are not handling
permanent transaction state correctly as we pass it between
different functions. Especially as we need to do that to get rid of
the log item descriptor abstraction...

> Which is a feature I find horribly confusing, so we should
> get rid of it, for which we'd need to get rid of the concept of
> synchronous transactions in favour of leaving the log force to the
> caller, which again would be more logical.
> 
> Guess I need to look into doing these cleanups as I don't want to force
> them on anyone else.  Just need to finish all the other more important
> bits on the todo list first :)

As always :P

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com