[2.6.27-rc4] XFS i_lock vs i_iolock...

[2.6.27-rc4] XFS i_lock vs i_iolock...

[Daniel J Blueman]

On 2.6.27-rc4 with various debug options enabled, lockdep claims lock
ordering issues with XFS [1] - easiest reproducer is just running
xfs_fsr. Mount options I was using were
'nobarrier,noatime,nodiratime'.

Thanks,
  Daniel

--- [1]

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.27-rc4-224c #1
-------------------------------------------------------
xfs_fsr/5763 is trying to acquire lock:
 (&(&ip->i_lock)->mr_lock/2){--..}, at: [<ffffffff803ad8fc>] xfs_ilock+0x8c/0xb0

but task is already holding lock:
 (&(&ip->i_iolock)->mr_lock/3){--..}, at: [<ffffffff803ad915>]
xfs_ilock+0xa5/0xb0

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #1 (&(&ip->i_iolock)->mr_lock/3){--..}:
       [<ffffffff8026b011>] __lock_acquire+0xdb1/0x1150
       [<ffffffff8026b441>] lock_acquire+0x91/0xc0
       [<ffffffff8025d967>] down_write_nested+0x57/0x90
       [<ffffffff803ad915>] xfs_ilock+0xa5/0xb0
       [<ffffffff803cb086>] xfs_lock_two_inodes+0x106/0x120
       [<ffffffff803b78e0>] xfs_swap_extents+0x70/0x5b0
       [<ffffffff803b7f68>] xfs_swapext+0x148/0x150
       [<ffffffff803d8195>] xfs_ioctl+0x6a5/0x810
       [<ffffffff803d58bd>] xfs_file_ioctl_invis+0x3d/0x80
       [<ffffffff802d4586>] vfs_ioctl+0x36/0xb0
       [<ffffffff802d488b>] do_vfs_ioctl+0x28b/0x2f0
       [<ffffffff802d493f>] sys_ioctl+0x4f/0x80
       [<ffffffff8020c74b>] system_call_fastpath+0x16/0x1b
       [<ffffffffffffffff>] 0xffffffffffffffff

-> #0 (&(&ip->i_lock)->mr_lock/2){--..}:
       [<ffffffff8026b0f5>] __lock_acquire+0xe95/0x1150
       [<ffffffff8026b441>] lock_acquire+0x91/0xc0
       [<ffffffff8025d967>] down_write_nested+0x57/0x90
       [<ffffffff803ad8fc>] xfs_ilock+0x8c/0xb0
       [<ffffffff803caff0>] xfs_lock_two_inodes+0x70/0x120
       [<ffffffff803b7b03>] xfs_swap_extents+0x293/0x5b0
       [<ffffffff803b7f68>] xfs_swapext+0x148/0x150
       [<ffffffff803d8195>] xfs_ioctl+0x6a5/0x810
       [<ffffffff803d58bd>] xfs_file_ioctl_invis+0x3d/0x80
       [<ffffffff802d4586>] vfs_ioctl+0x36/0xb0
       [<ffffffff802d488b>] do_vfs_ioctl+0x28b/0x2f0
       [<ffffffff802d493f>] sys_ioctl+0x4f/0x80
       [<ffffffff8020c74b>] system_call_fastpath+0x16/0x1b
       [<ffffffffffffffff>] 0xffffffffffffffff

other info that might help us debug this:

2 locks held by xfs_fsr/5763:
 #0:  (&(&ip->i_iolock)->mr_lock/2){--..}, at: [<ffffffff803ad915>]
xfs_ilock+0xa5/0xb0
 #1:  (&(&ip->i_iolock)->mr_lock/3){--..}, at: [<ffffffff803ad915>]
xfs_ilock+0xa5/0xb0

stack backtrace:
Pid: 5763, comm: xfs_fsr Not tainted 2.6.27-rc4-224c #1

Call Trace:
 [<ffffffff80268d1f>] print_circular_bug_tail+0x9f/0xe0
 [<ffffffff8026b0f5>] __lock_acquire+0xe95/0x1150
 [<ffffffff8026b441>] lock_acquire+0x91/0xc0
 [<ffffffff803ad8fc>] ? xfs_ilock+0x8c/0xb0
 [<ffffffff8025d967>] down_write_nested+0x57/0x90
 [<ffffffff803ad8fc>] ? xfs_ilock+0x8c/0xb0
 [<ffffffff803ad8fc>] xfs_ilock+0x8c/0xb0
 [<ffffffff803caff0>] xfs_lock_two_inodes+0x70/0x120
 [<ffffffff803b7b03>] xfs_swap_extents+0x293/0x5b0
 [<ffffffff803b7f68>] xfs_swapext+0x148/0x150
 [<ffffffff803d8195>] xfs_ioctl+0x6a5/0x810
 [<ffffffff802148b0>] ? native_sched_clock+0x70/0xa0
 [<ffffffff802e24f2>] ? mnt_drop_write+0x62/0x140
 [<ffffffff803d58bd>] xfs_file_ioctl_invis+0x3d/0x80
 [<ffffffff802d4586>] vfs_ioctl+0x36/0xb0
 [<ffffffff802d488b>] do_vfs_ioctl+0x28b/0x2f0
 [<ffffffff8064128e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff802d493f>] sys_ioctl+0x4f/0x80
 [<ffffffff8020c74b>] system_call_fastpath+0x16/0x1b
-- 
Daniel J Blueman

Re: [2.6.27-rc4] XFS i_lock vs i_iolock...

[Dave Chinner]

On Fri, Aug 22, 2008 at 10:12:59PM +0100, Daniel J Blueman wrote:
> On 2.6.27-rc4 with various debug options enabled, lockdep claims lock
> ordering issues with XFS [1] - easiest reproducer is just running
> xfs_fsr. Mount options I was using were
> 'nobarrier,noatime,nodiratime'.
> 
> Thanks,
>   Daniel
> 
> --- [1]
> 
> =======================================================
> [ INFO: possible circular locking dependency detected ]
> 2.6.27-rc4-224c #1
> -------------------------------------------------------
> xfs_fsr/5763 is trying to acquire lock:
>  (&(&ip->i_lock)->mr_lock/2){--..}, at: [<ffffffff803ad8fc>] xfs_ilock+0x8c/0xb0
> 
> but task is already holding lock:
>  (&(&ip->i_iolock)->mr_lock/3){--..}, at: [<ffffffff803ad915>]
> xfs_ilock+0xa5/0xb0

False positive. We do:

	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
	.....
	xfs_iunlock(ip, XFS_ILOCK_EXCL);
	xfs_iunlock(tip, XFS_ILOCK_EXCL);
	.....
	xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);

Which is a perfectly valid thing to do.

The problem is that lockdep is complaining about the second call
to xfs_lock_two_inodes(), which uses the subclasses 2 and 3.
effectively it is seeing:

	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
		iolock/2
		ilock/2
		iolock/3
		ilock/3
	.....
	xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
		ilock/2
		ilock/3


But because the original lock order was ilock/2->iolock/3, the
second call to xfs_lock_two_inodes is seeing iolock/3->ilock/2
which it then complains about....

Christoph - I think we're going to need to pass a lockdep 'order'
flag into xfs_lock_two_inodes() to avoid this so the second call
can use different classes to the first call. Or perhaps a '_nested'
variant of the call...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [2.6.27-rc4] XFS i_lock vs i_iolock...

[Lachlan McIlroy]

Dave Chinner wrote:
> On Fri, Aug 22, 2008 at 10:12:59PM +0100, Daniel J Blueman wrote:
>> On 2.6.27-rc4 with various debug options enabled, lockdep claims lock
>> ordering issues with XFS [1] - easiest reproducer is just running
>> xfs_fsr. Mount options I was using were
>> 'nobarrier,noatime,nodiratime'.
>>
>> Thanks,
>>   Daniel
>>
>> --- [1]
>>
>> =======================================================
>> [ INFO: possible circular locking dependency detected ]
>> 2.6.27-rc4-224c #1
>> -------------------------------------------------------
>> xfs_fsr/5763 is trying to acquire lock:
>>  (&(&ip->i_lock)->mr_lock/2){--..}, at: [<ffffffff803ad8fc>] xfs_ilock+0x8c/0xb0
>>
>> but task is already holding lock:
>>  (&(&ip->i_iolock)->mr_lock/3){--..}, at: [<ffffffff803ad915>]
>> xfs_ilock+0xa5/0xb0
> 
> False positive. We do:
> 
> 	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);

Why not just change the above line to two lines:
	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL);
	xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);


> 	.....
> 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> 	xfs_iunlock(tip, XFS_ILOCK_EXCL);
> 	.....
> 	xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
> 
> Which is a perfectly valid thing to do.
> 
> The problem is that lockdep is complaining about the second call
> to xfs_lock_two_inodes(), which uses the subclasses 2 and 3.
> effectively it is seeing:
> 
> 	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
> 		iolock/2
> 		ilock/2
> 		iolock/3
> 		ilock/3
> 	.....
> 	xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
> 		ilock/2
> 		ilock/3
> 
> 
> But because the original lock order was ilock/2->iolock/3, the
> second call to xfs_lock_two_inodes is seeing iolock/3->ilock/2
> which it then complains about....
> 
> Christoph - I think we're going to need to pass a lockdep 'order'
> flag into xfs_lock_two_inodes() to avoid this so the second call
> can use different classes to the first call. Or perhaps a '_nested'
> variant of the call...
> 
> Cheers,
> 
> Dave.

Re: [2.6.27-rc4] XFS i_lock vs i_iolock...

[Dave Chinner]

On Mon, Aug 25, 2008 at 12:12:23PM +1000, Lachlan McIlroy wrote:
> Dave Chinner wrote:
>> On Fri, Aug 22, 2008 at 10:12:59PM +0100, Daniel J Blueman wrote:
>>> On 2.6.27-rc4 with various debug options enabled, lockdep claims lock
>>> ordering issues with XFS [1] - easiest reproducer is just running
>>> xfs_fsr. Mount options I was using were
>>> 'nobarrier,noatime,nodiratime'.
>>>
>>> Thanks,
>>>   Daniel
>>>
>>> --- [1]
>>>
>>> =======================================================
>>> [ INFO: possible circular locking dependency detected ]
>>> 2.6.27-rc4-224c #1
>>> -------------------------------------------------------
>>> xfs_fsr/5763 is trying to acquire lock:
>>>  (&(&ip->i_lock)->mr_lock/2){--..}, at: [<ffffffff803ad8fc>] xfs_ilock+0x8c/0xb0
>>>
>>> but task is already holding lock:
>>>  (&(&ip->i_iolock)->mr_lock/3){--..}, at: [<ffffffff803ad915>]
>>> xfs_ilock+0xa5/0xb0
>>
>> False positive. We do:
>>
>> 	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
>
> Why not just change the above line to two lines:
> 	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL);
> 	xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);

Yeah, that'd work, but it implllies that we no longer allow
xfs_lock_two_inodes() to take both inode locks at once. It
would need a comment blaming^Wexplaining why lockdep requires
us to do this, and then debug code in xfs_lock_two_inodes() to
catch this when someone makes this mistake again in the future.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [2.6.27-rc4] XFS i_lock vs i_iolock...

[Peter Zijlstra]

On Mon, 2008-08-25 at 13:55 +1000, Dave Chinner wrote:
> On Mon, Aug 25, 2008 at 12:12:23PM +1000, Lachlan McIlroy wrote:
> > Dave Chinner wrote:
> >> On Fri, Aug 22, 2008 at 10:12:59PM +0100, Daniel J Blueman wrote:
> >>> On 2.6.27-rc4 with various debug options enabled, lockdep claims lock
> >>> ordering issues with XFS [1] - easiest reproducer is just running
> >>> xfs_fsr. Mount options I was using were
> >>> 'nobarrier,noatime,nodiratime'.
> >>>
> >>> Thanks,
> >>>   Daniel
> >>>
> >>> --- [1]
> >>>
> >>> =======================================================
> >>> [ INFO: possible circular locking dependency detected ]
> >>> 2.6.27-rc4-224c #1
> >>> -------------------------------------------------------
> >>> xfs_fsr/5763 is trying to acquire lock:
> >>>  (&(&ip->i_lock)->mr_lock/2){--..}, at: [<ffffffff803ad8fc>] xfs_ilock+0x8c/0xb0
> >>>
> >>> but task is already holding lock:
> >>>  (&(&ip->i_iolock)->mr_lock/3){--..}, at: [<ffffffff803ad915>]
> >>> xfs_ilock+0xa5/0xb0
> >>
> >> False positive. We do:
> >>
> >> 	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
> >
> > Why not just change the above line to two lines:
> > 	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL);
> > 	xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
> 
> Yeah, that'd work, but it implllies that we no longer allow
> xfs_lock_two_inodes() to take both inode locks at once. 

How can you take two locks in one go? It seems to me you always need to
take them one after another, and as soon as you do that, you have
ordering constraints.

Of course it could be that doesn't matter, because there is another
serializing lock, but that isn't clear from this context.

> It
> would need a comment blaming^Wexplaining why lockdep requires
> us to do this, and then debug code in xfs_lock_two_inodes() to
> catch this when someone makes this mistake again in the future.

Re: [2.6.27-rc4] XFS i_lock vs i_iolock...

[Christoph Hellwig]

On Mon, Aug 25, 2008 at 08:59:33AM +0200, Peter Zijlstra wrote:
> How can you take two locks in one go? It seems to me you always need to
> take them one after another, and as soon as you do that, you have
> ordering constraints.

Yes, you would.  Except that in all other places we only have a single
iolock involved, so the ordering of the second iolock and second ilock
don't matter.

Because of that I think declaring that xfs_lock_two_inodes can just
lock on lock type at a time might be the better solution.

Re: [2.6.27-rc4] XFS i_lock vs i_iolock...

[Dave Chinner]

On Mon, Aug 25, 2008 at 11:55:32PM +0200, Christoph Hellwig wrote:
> On Mon, Aug 25, 2008 at 08:59:33AM +0200, Peter Zijlstra wrote:
> > How can you take two locks in one go? It seems to me you always need to
> > take them one after another, and as soon as you do that, you have
> > ordering constraints.
> 
> Yes, you would.  Except that in all other places we only have a single
> iolock involved, so the ordering of the second iolock and second ilock
> don't matter.
> 
> Because of that I think declaring that xfs_lock_two_inodes can just
> lock on lock type at a time might be the better solution.

Agreed. Patch below.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

XFS: prevent lockdep false positives when locking two inodes

If we call xfs_lock_two_inodes() to grab both the iolock and
the ilock, then drop the ilocks on both inodes, then grab
them again (as xfs_swap_extents() does) then lockdep will
report a locking order problem. This is a false positive.

To avoid this, disallow xfs_lock_two_inodes() fom locking both
inode locks at once - force calers to make two separate calls.
This means that nested dropping and regaining of the ilocks
will retain the same lockdep subclass and so lockdep will
not see anything wrong with this code.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_dfrag.c    |    9 ++++++++-
 fs/xfs/xfs_vnodeops.c |   10 ++++++++++
 2 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_dfrag.c b/fs/xfs/xfs_dfrag.c
index 760f4c5..75b0cd4 100644
--- a/fs/xfs/xfs_dfrag.c
+++ b/fs/xfs/xfs_dfrag.c
@@ -149,7 +149,14 @@ xfs_swap_extents(
 
 	sbp = &sxp->sx_stat;
 
-	xfs_lock_two_inodes(ip, tip, lock_flags);
+	/*
+	 * we have to do two separate lock calls here to keep lockdep
+	 * happy. If we try to get all the locks in one call, lock will
+	 * report false positives when we drop the ILOCK and regain them
+	 * below.
+	 */
+	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL);
+	xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
 	locked = 1;
 
 	/* Verify that both files have the same format */
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index f108102..cb1b5fd 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -1836,6 +1836,12 @@ again:
 #endif
 }
 
+/*
+ * xfs_lock_two_inodes() can only be used to lock one type of lock
+ * at a time - the iolock or the ilock, but not both at once. If
+ * we lock both at once, lockdep will report false positives saying
+ * we have violated locking orders.
+ */
 void
 xfs_lock_two_inodes(
 	xfs_inode_t		*ip0,
@@ -1846,7 +1852,11 @@ xfs_lock_two_inodes(
 	int			attempts = 0;
 	xfs_log_item_t		*lp;
 
+#ifdef DEBUG
+	if (lock_mode & (XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL))
+		ASSERT((lock_mode & (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)) == 0);
 	ASSERT(ip0->i_ino != ip1->i_ino);
+#endif
 
 	if (ip0->i_ino > ip1->i_ino) {
 		temp = ip0;

Re: [2.6.27-rc4] XFS i_lock vs i_iolock...

[Christoph Hellwig]

On Tue, Aug 26, 2008 at 12:45:47PM +1000, Dave Chinner wrote:
> XFS: prevent lockdep false positives when locking two inodes
> 
> If we call xfs_lock_two_inodes() to grab both the iolock and
> the ilock, then drop the ilocks on both inodes, then grab
> them again (as xfs_swap_extents() does) then lockdep will
> report a locking order problem. This is a false positive.
> 
> To avoid this, disallow xfs_lock_two_inodes() fom locking both
> inode locks at once - force calers to make two separate calls.
> This means that nested dropping and regaining of the ilocks
> will retain the same lockdep subclass and so lockdep will
> not see anything wrong with this code.

Looks good.  We probably don't need the #ifdef DEBUG as ASSERT is
debug-only anyway.

Re: [2.6.27-rc4] XFS i_lock vs i_iolock...

[Daniel J Blueman]

Hi Dave,

On Tue, Aug 26, 2008 at 3:45 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Mon, Aug 25, 2008 at 11:55:32PM +0200, Christoph Hellwig wrote:
>> On Mon, Aug 25, 2008 at 08:59:33AM +0200, Peter Zijlstra wrote:
>> > How can you take two locks in one go? It seems to me you always need to
>> > take them one after another, and as soon as you do that, you have
>> > ordering constraints.
>>
>> Yes, you would.  Except that in all other places we only have a single
>> iolock involved, so the ordering of the second iolock and second ilock
>> don't matter.
>>
>> Because of that I think declaring that xfs_lock_two_inodes can just
>> lock on lock type at a time might be the better solution.
>
> Agreed. Patch below.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
> XFS: prevent lockdep false positives when locking two inodes
>
> If we call xfs_lock_two_inodes() to grab both the iolock and
> the ilock, then drop the ilocks on both inodes, then grab
> them again (as xfs_swap_extents() does) then lockdep will
> report a locking order problem. This is a false positive.
>
> To avoid this, disallow xfs_lock_two_inodes() fom locking both
> inode locks at once - force calers to make two separate calls.
> This means that nested dropping and regaining of the ilocks
> will retain the same lockdep subclass and so lockdep will
> not see anything wrong with this code.
>
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> ---
>  fs/xfs/xfs_dfrag.c    |    9 ++++++++-
>  fs/xfs/xfs_vnodeops.c |   10 ++++++++++
>  2 files changed, 18 insertions(+), 1 deletions(-)
>
> diff --git a/fs/xfs/xfs_dfrag.c b/fs/xfs/xfs_dfrag.c
> index 760f4c5..75b0cd4 100644
> --- a/fs/xfs/xfs_dfrag.c
> +++ b/fs/xfs/xfs_dfrag.c
> @@ -149,7 +149,14 @@ xfs_swap_extents(
>
>        sbp = &sxp->sx_stat;
>
> -       xfs_lock_two_inodes(ip, tip, lock_flags);
> +       /*
> +        * we have to do two separate lock calls here to keep lockdep
> +        * happy. If we try to get all the locks in one call, lock will
> +        * report false positives when we drop the ILOCK and regain them
> +        * below.
> +        */
> +       xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL);
> +       xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
>        locked = 1;
>
>        /* Verify that both files have the same format */
> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> index f108102..cb1b5fd 100644
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
> @@ -1836,6 +1836,12 @@ again:
>  #endif
>  }
>
> +/*
> + * xfs_lock_two_inodes() can only be used to lock one type of lock
> + * at a time - the iolock or the ilock, but not both at once. If
> + * we lock both at once, lockdep will report false positives saying
> + * we have violated locking orders.
> + */
>  void
>  xfs_lock_two_inodes(
>        xfs_inode_t             *ip0,
> @@ -1846,7 +1852,11 @@ xfs_lock_two_inodes(
>        int                     attempts = 0;
>        xfs_log_item_t          *lp;
>
> +#ifdef DEBUG
> +       if (lock_mode & (XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL))
> +               ASSERT((lock_mode & (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)) == 0);
>        ASSERT(ip0->i_ino != ip1->i_ino);
> +#endif
>
>        if (ip0->i_ino > ip1->i_ino) {
>                temp = ip0;

Good to get your patch and HCH's ack...thanks!

I'll pursue testing and touchdown in < 24 hrs.

Daniel
-- 
Daniel J Blueman

Re: [2.6.27-rc4] XFS i_lock vs i_iolock...

[Daniel J Blueman]

On Tue, Aug 26, 2008 at 9:13 PM, Daniel J Blueman
<daniel.blueman@gmail.com> wrote:
> Hi Dave,
>
> On Tue, Aug 26, 2008 at 3:45 AM, Dave Chinner <david@fromorbit.com> wrote:
>> On Mon, Aug 25, 2008 at 11:55:32PM +0200, Christoph Hellwig wrote:
>>> On Mon, Aug 25, 2008 at 08:59:33AM +0200, Peter Zijlstra wrote:
>>> > How can you take two locks in one go? It seems to me you always need to
>>> > take them one after another, and as soon as you do that, you have
>>> > ordering constraints.
>>>
>>> Yes, you would.  Except that in all other places we only have a single
>>> iolock involved, so the ordering of the second iolock and second ilock
>>> don't matter.
>>>
>>> Because of that I think declaring that xfs_lock_two_inodes can just
>>> lock on lock type at a time might be the better solution.
>>
>> Agreed. Patch below.
>>
>> Cheers,
>>
>> Dave.
>> --
>> Dave Chinner
>> david@fromorbit.com
>>
>> XFS: prevent lockdep false positives when locking two inodes
>>
>> If we call xfs_lock_two_inodes() to grab both the iolock and
>> the ilock, then drop the ilocks on both inodes, then grab
>> them again (as xfs_swap_extents() does) then lockdep will
>> report a locking order problem. This is a false positive.
>>
>> To avoid this, disallow xfs_lock_two_inodes() fom locking both
>> inode locks at once - force calers to make two separate calls.
>> This means that nested dropping and regaining of the ilocks
>> will retain the same lockdep subclass and so lockdep will
>> not see anything wrong with this code.
>>
>> Signed-off-by: Dave Chinner <david@fromorbit.com>
>> ---
>>  fs/xfs/xfs_dfrag.c    |    9 ++++++++-
>>  fs/xfs/xfs_vnodeops.c |   10 ++++++++++
>>  2 files changed, 18 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_dfrag.c b/fs/xfs/xfs_dfrag.c
>> index 760f4c5..75b0cd4 100644
>> --- a/fs/xfs/xfs_dfrag.c
>> +++ b/fs/xfs/xfs_dfrag.c
>> @@ -149,7 +149,14 @@ xfs_swap_extents(
>>
>>        sbp = &sxp->sx_stat;
>>
>> -       xfs_lock_two_inodes(ip, tip, lock_flags);
>> +       /*
>> +        * we have to do two separate lock calls here to keep lockdep
>> +        * happy. If we try to get all the locks in one call, lock will
>> +        * report false positives when we drop the ILOCK and regain them
>> +        * below.
>> +        */
>> +       xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL);
>> +       xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
>>        locked = 1;
>>
>>        /* Verify that both files have the same format */
>> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
>> index f108102..cb1b5fd 100644
>> --- a/fs/xfs/xfs_vnodeops.c
>> +++ b/fs/xfs/xfs_vnodeops.c
>> @@ -1836,6 +1836,12 @@ again:
>>  #endif
>>  }
>>
>> +/*
>> + * xfs_lock_two_inodes() can only be used to lock one type of lock
>> + * at a time - the iolock or the ilock, but not both at once. If
>> + * we lock both at once, lockdep will report false positives saying
>> + * we have violated locking orders.
>> + */
>>  void
>>  xfs_lock_two_inodes(
>>        xfs_inode_t             *ip0,
>> @@ -1846,7 +1852,11 @@ xfs_lock_two_inodes(
>>        int                     attempts = 0;
>>        xfs_log_item_t          *lp;
>>
>> +#ifdef DEBUG
>> +       if (lock_mode & (XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL))
>> +               ASSERT((lock_mode & (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)) == 0);
>>        ASSERT(ip0->i_ino != ip1->i_ino);
>> +#endif
>>
>>        if (ip0->i_ino > ip1->i_ino) {
>>                temp = ip0;
>
> Good to get your patch and HCH's ack...thanks!
>
> I'll pursue testing and touchdown in < 24 hrs.

Excellent - confirmed it addresses the lockdep report I was seeing
before and doesn't introduce any regressions.

Thanks,
  Daniel
-- 
Daniel J Blueman

Re: [2.6.27-rc4] XFS i_lock vs i_iolock...

[Dave Chinner]

On Mon, Aug 25, 2008 at 08:59:33AM +0200, Peter Zijlstra wrote:
> On Mon, 2008-08-25 at 13:55 +1000, Dave Chinner wrote:
> > On Mon, Aug 25, 2008 at 12:12:23PM +1000, Lachlan McIlroy wrote:
> > > Dave Chinner wrote:
> > >> On Fri, Aug 22, 2008 at 10:12:59PM +0100, Daniel J Blueman wrote:
> > >>> =======================================================
> > >>> [ INFO: possible circular locking dependency detected ]
> > >>> 2.6.27-rc4-224c #1
> > >>> -------------------------------------------------------
> > >>> xfs_fsr/5763 is trying to acquire lock:
> > >>>  (&(&ip->i_lock)->mr_lock/2){--..}, at: [<ffffffff803ad8fc>] xfs_ilock+0x8c/0xb0
> > >>>
> > >>> but task is already holding lock:
> > >>>  (&(&ip->i_iolock)->mr_lock/3){--..}, at: [<ffffffff803ad915>]
> > >>> xfs_ilock+0xa5/0xb0
> > >>
> > >> False positive. We do:
> > >>
> > >> 	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
> > >
> > > Why not just change the above line to two lines:
> > > 	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL);
> > > 	xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
> > 
> > Yeah, that'd work, but it implllies that we no longer allow
> > xfs_lock_two_inodes() to take both inode locks at once. 
> 
> How can you take two locks in one go? It seems to me you always need to
> take them one after another, and as soon as you do that, you have
> ordering constraints.

It doesn't take them both inode locks in one go - it does them
separately in a given order via xfs_ilock(). Basically there are two
layers of constraints here - xfs_ilock() handles the order
withing a given inode, xfs_lock_two_inodes() handles order and
deadlock prevention between inodes.

What lockdep is complaining about is a difference in the lock
order between different locks in different inodes - a situation
that does not result in a deadlock...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [2.6.27-rc4] XFS i_lock vs i_iolock...

[Peter Zijlstra]

On Mon, 2008-08-25 at 11:02 +1000, Dave Chinner wrote:
> On Fri, Aug 22, 2008 at 10:12:59PM +0100, Daniel J Blueman wrote:
> > On 2.6.27-rc4 with various debug options enabled, lockdep claims lock
> > ordering issues with XFS [1] - easiest reproducer is just running
> > xfs_fsr. Mount options I was using were
> > 'nobarrier,noatime,nodiratime'.
> > 
> > Thanks,
> >   Daniel
> > 
> > --- [1]
> > 
> > =======================================================
> > [ INFO: possible circular locking dependency detected ]
> > 2.6.27-rc4-224c #1
> > -------------------------------------------------------
> > xfs_fsr/5763 is trying to acquire lock:
> >  (&(&ip->i_lock)->mr_lock/2){--..}, at: [<ffffffff803ad8fc>] xfs_ilock+0x8c/0xb0
> > 
> > but task is already holding lock:
> >  (&(&ip->i_iolock)->mr_lock/3){--..}, at: [<ffffffff803ad915>]
> > xfs_ilock+0xa5/0xb0
> 
> False positive. We do:
> 
> 	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
> 	.....
> 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> 	xfs_iunlock(tip, XFS_ILOCK_EXCL);
> 	.....
> 	xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
> 
> Which is a perfectly valid thing to do.
> 
> The problem is that lockdep is complaining about the second call
> to xfs_lock_two_inodes(), which uses the subclasses 2 and 3.
> effectively it is seeing:
> 
> 	xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
> 		iolock/2
> 		ilock/2
> 		iolock/3
> 		ilock/3
> 	.....
> 	xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
> 		ilock/2
> 		ilock/3
> 
> 
> But because the original lock order was ilock/2->iolock/3, the
> second call to xfs_lock_two_inodes is seeing iolock/3->ilock/2
> which it then complains about....

Does the annotation I used for
double_lock_balance()/double_unlock_balance() work?

Basically, it assumes the held lock (this_rq) has subclass 0, but
because double_lock_balance() can unlock and relock, depending on order,
it can end up being 1 at the end. So what we do is reset the subclass
(after unlocking the now 0 lock) to 0 using lock_set_subclass().