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

[Dave Chinner <david@fromorbit.com>]

On Wed, Nov 21, 2012 at 04:59:00AM -0500, Christoph Hellwig wrote:
> 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.

OK.

> > > >  	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.

OK, I'll fix that.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs