xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock.
From: Jan Kara <jack@xxxxxxx>
Date: Tue, 20 Nov 2012 17:37:34 +0100
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, Jan Kara <jack@xxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20121120161015.GB18244@xxxxxxxxxxxxx>
References: <1353410831-22653-1-git-send-email-david@xxxxxxxxxxxxx> <1353410831-22653-3-git-send-email-david@xxxxxxxxxxxxx> <20121120161015.GB18244@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Tue 20-11-12 11:10:15, Christoph Hellwig wrote:
> On Tue, Nov 20, 2012 at 10:27:11PM +1100, Dave Chinner wrote:
> > This was discovered on a filesystem with a log of only 10MB, and a
> > log stripe unit of 256k whih increased the base reservations by
> > 512k. Hence a allocation transaction requires 1.2MB of log space to
> > be available instead of only 260k, and so greatly increased the
> > chance that there wouldn't be enough log space available for the
> > nested transaction to succeed. The key to reproducing it is this
> > mkfs command:
> > 
> > mkfs.xfs -f -d agcount=16,su=256k,sw=12 -l su=256k,size=2560b $SCRATCH_DEV
> > 
> > The test case was a 1000 fsstress processes running with random
> > freeze and unfreezes every few seconds. Thanks to Eryu Guan
> > (eguan@xxxxxxxxxx) for writing the test that found this on a system
> > with a somewhat unique default configuration....
> 
> That sounds like something that could fit xfstests fairly easily.
> 
> Re the patch - you're moving the transaction allocation back into the
> end_io handler.  That's what my original version did, and I'm pretty
> sure you talked me out of it back then.  I can't remember the details
> but the list should have it.
> 
> > @@ -151,9 +151,11 @@ xfs_setfilesize(
> >     /*
> >      * 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 still seems to be true for buffered IO or am I misreading the code?

> I'm also not sure the freeze protection still works if the acquire is
> outside the original broader scope protection.  I'll defer to Jan on
> this as I don't really understand this magic enough.q
> should be removed respectively replaced with sb_start_intwrite/sb_end_intwrite
  It seems to work OK. If it were not for buffered IO path which allocates
a transaction (and thus freeze protection) in xfs_vm_writepage() we could
get rid of this lockdep magic. But so far we can't...

                                                                Honza
-- 
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR

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