xfs
[Top] [All Lists]

Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock.

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock.
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Wed, 21 Nov 2012 04:59:00 -0500
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, Jan Kara <jack@xxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20121120195344.GD2591@dastard>
References: <1353410831-22653-1-git-send-email-david@xxxxxxxxxxxxx> <1353410831-22653-3-git-send-email-david@xxxxxxxxxxxxx> <20121120161015.GB18244@xxxxxxxxxxxxx> <20121120195344.GD2591@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Nov 21, 2012 at 06:53:44AM +1100, Dave Chinner wrote:
> Right, I was concerned about blocking IO completion workers waiting
> for log reservations. I'm still concerned about that, but I don't
> see any way around it.

That's information that should be added to a comment..

> > >   /*
> > >    * The transaction was allocated in the I/O submission thread,
> > >    * thus we need to mark ourselves as beeing in a transaction
> > > -  * manually.
> > > +  * manually. Similarly for freeze protection.
> > >    */
> > >   current_set_flags_nested(&tp->t_pflags, PF_FSTRANS);
> > > + rwsem_acquire_read(&VFS_I(ip)->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
> > > +                    0, 1, _THIS_IP_);
> > 
> > The comment above isn't true anymore, and the flags hack should be
> > removed.
> 
> It's still used by buffered IO in that way.

It's conditionaly though, so there should at least be a "may" in the
sentence.

> > >   if (ioend->io_type == XFS_IO_UNWRITTEN) {
> > >           error = xfs_iomap_write_unwritten(ip, ioend->io_offset,
> > > +                                           ioend->io_size);
> > > +         goto done;
> > > + }
> > > +
> > > + /*
> > > +  * For direct I/O we do not know if we need to allocate blocks or not so
> > > +  * we can't preallocate an append transaction as that results in nested
> > > +  * reservations and log space deadlocks. Hence allocate the transaction
> > > +  * here.  For buffered I/O we preallocate a transaction when submitting
> > > +  * the IO.
> > > +  */
> > > + if (ioend->io_isdirect && xfs_ioend_is_append(ioend)) {
> > 
> > xfs_iomap_write_unwritten already updates the inode size, so this should
> > be an "else if"
> 
> The unwritten branch jumps over this completely if it is taken, so
> it makes no difference. I can change it is you want....

Oh, right - I missed that.  But it seems the else would do the same as
the goto done in your new version, and I generally prefer else if style
control flow for this over gotos.

<Prev in Thread] Current Thread [Next in Thread>