On Sat, Apr 11, 2015 at 05:15:18PM -0400, Brian Foster wrote:
> On Fri, Apr 10, 2015 at 11:37:57PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > Now we have an ioend being passed unconditionally to the direct IO
> > write completion context, we can pass a preallocated transaction
> > handle for on-disk inode size updates that are run in completion.
.....
> > --- a/fs/xfs/xfs_aops.c
> > +++ b/fs/xfs/xfs_aops.c
> > @@ -178,6 +178,25 @@ xfs_setfilesize_ioend(
> > return xfs_setfilesize(ip, tp, ioend->io_offset, ioend->io_size);
> > }
> >
> > +STATIC void
> > +xfs_setfilesize_ioend_cancel(
> > + struct xfs_ioend *ioend)
> > +{
> > + struct xfs_trans *tp = ioend->io_append_trans;
> > +
> > + /*
> > + * The transaction may have been allocated in the I/O submission thread,
> > + * thus we need to mark ourselves as being in a transaction manually.
> > + * Similarly for freeze protection.
> > + */
>
> This threw me off at first because we can call this from either the
> submission or the completion context, unlike the commit case where this
> comment is copied from. Could we move the comment above the function and
> clarify a bit? E.g., something like the following is a bit more clear to
> me:
>
> /*
> * The process transaction and freeze protection state is cleared immediately
> * after setfilesize transaction allocation to support transfer of the tp from
> * submission to completion context. Restore the context appropriately to cancel
> * the transaction.
> */
OK, I can do that, but given your next comments, it might just go
away.
> > + } else {
> > + ioend = xfs_alloc_ioend(inode, type);
> > + ioend->io_offset = offset;
> > + ioend->io_size = size;
> > + bh_result->b_private = ioend;
> > + trace_xfs_gbmap_direct_new(XFS_I(inode), offset, size, type,
> > + imap);
> > + }
> > +
> > + /* check if we need an append transaction allocated. */
> > + if (ioend->io_type == XFS_IO_OVERWRITE &&
> > + xfs_ioend_is_append(ioend) && !ioend->io_append_trans) {
> > + int error;
> > +
> > + error = xfs_setfilesize_trans_alloc(ioend);
>
> I'm not totally convinced this is safe. We previously moved this tp
> allocation from before a potential xfs_iomap_direct_write() call to the
> completion side to avoid nesting this allocation with unwritten extent
> allocation transactions. See the following for reference:
>
> 437a255a xfs: fix direct IO nested transaction deadlock
>
> Now we move it after that point of the codepath, and even then we know
> that this is an overwrite if we do the allocation here. If we continue
> on and hit a hole, it looks like there's still a sequence to allocate
> this transaction and call xfs_iomap_write_direct(), nesting the
> associated transaction reservations. Am I missing something?
No, I didn't really think this part through fully. I knew that we'd
get multiple calls, and we'd get multiple allocations, but for some
reason the penny didn't drop.
What it comes down to is that either we jump through lots of hoops
in __xfs_get_blocks() to handle this case (i.e.
cancel/allocate/reserve on repeat calls) or we just allocate it in
IO completion context as we currently are doing.
I'll have a look at what cancel/allocate/reserve looks like - it
might actually simplify the logic - and go from there.
Thanks for catching my silly thinko, Brain!
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
