On Tue, Sep 03, 2013 at 02:25:03PM -0400, Brian Foster wrote:
> The transaction allocated in xfs_inactive() can be passed down into
> xfs_inactive_symlink() or xfs_itruncate_extents(), both of which
> can commit and reallocate a new transaction. This leads to
> reservation issues if the transaction is subsequently passed into
> xfs_ifree(), which requires a larger reservation to manage the
> finobt.
>
> Reorganize xfs_inactive() to commit any transaction handed back
> from symlink or truncate processing and unconditionally allocate
> a new transaction for xfs_ifree() with the appropriate reservation.
Ok, I've had a bit of a look at this now, and I like how the code
turns out. However, I don't think it goes far enough, or fix the
problem that causes all the transaction nastiness in xfs_inactive().
Firstly, we are not doing rolling transactions here - there is no
need for all the changes to be atomic because the inode is on the
unlinked list if it is going to be freed. Hence we don't need to
pass transaction pointers around.
xfs_inactive_symlink() can do a transaction completely internally,
and, well, it doesn't even log the inode if the symlink is in-line
and so may not even need a transaction. Hence really only
xfs_inactive_symlink_rmt() needs to run a transaction, and it can do
that internally just fine.
For the xfs_itruncate_extents() data fork transaction, just add a
new wrapper called xfs_inactive_truncate() that holds the
transaction context internally - that moves the only other
transaction context that you need to commit out of xfs_inactive()
altogether, as the attr fork already uses a private transaction
context.
And, finally, you can then factor the xfs_ifree() and it's
transaction context into a helper function as well, so there aren't
any transaction contexts left in xfs_inactive() at all.
That would leave us with:
if (ISLNK) {
error = xfs_inactive_symlink(ip);
} else if (truncate)
error = xfs_inactive_truncate(ip);
}
if (error)
goto out;
if (ip->i_d.di_anextents > 0)
error = xfs_attr_inactive(ip);
if (error)
goto out;
error = xfs_inactive_ifree(ip);
xfs_qm_dqdetach(ip);
out:
return;
This gives us a natural separation of the different transaction
reservations and contexts needed to perform the operations, and does
result in any extraneous work being done because we don't know what
the transaction context passed to us contains at all...
FWIW, there are other reasons for suggesting this structure - have a
read of "[RFD 14/17] xfs: separate inode freeing from inactivation"
and you'll see that what I've suggested above sets the code up for
implementing the optimisations documented in the RFD.
http://oss.sgi.com/archives/xfs/2013-08/msg00345.html
It might be best to put this as 3-4 patches at the start of the
series, rather than in the middle of it as it's really a separate
piece of cleanup work....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
