Re: [PATCH] xfs: end sync buffer I/O properly on shutdown error

[Brian Foster <bfoster@redhat.com>]

On Tue, Jan 29, 2019 at 08:30:41AM +1100, Dave Chinner wrote:
> On Mon, Jan 28, 2019 at 09:55:48AM -0500, Brian Foster wrote:
> > As of commit e339dd8d8b ("xfs: use sync buffer I/O for sync delwri
> > queue submission"), the delwri submission code uses sync buffer I/O
> > for sync delwri I/O. Instead of waiting on async I/O to unlock the
> > buffer, it uses the underlying sync I/O completion mechanism.
> > 
> > If delwri buffer submission fails due to a shutdown scenario, an
> > error is set on the buffer and buffer completion never occurs. This
> > can cause xfs_buf_delwri_submit() to deadlock waiting on a
> > completion event.
> > 
> > We could check the error state before waiting on such buffers, but
> > that doesn't serialize against the case of an error set via a racing
> > I/O completion. Instead, invoke I/O completion in the shutdown case
> > regardless of buffer I/O type.
> 
> How did you find this? i.e. what are the symptoms of the bug? I'm
> guessing that it's a shutdown/unmount hang from the above, but I'm
> really not sure.
> 

A shutdown during log recovery via generic/034 reproduced the deadlock
described in the commit log. The shutdown itself was caused by developer
error (missing an xfs_buf_ops assignment when working on the magic
stuff), so I'm not aware of a current upstream reproducer (that wouldn't
be related to some already corrupted fs).

> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/xfs_buf.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index eedc5e0156ff..1f9857e3630a 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -1536,8 +1536,7 @@ __xfs_buf_submit(
> >  		xfs_buf_ioerror(bp, -EIO);
> >  		bp->b_flags &= ~XBF_DONE;
> >  		xfs_buf_stale(bp);
> > -		if (bp->b_flags & XBF_ASYNC)
> > -			xfs_buf_ioend(bp);
> > +		xfs_buf_ioend(bp);
> >  		return -EIO;
> >  	}
> 
> That said, it definitely looks like it fixes a bug. Will test.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 

Thanks.

Brian

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com