Re: [PATCH] "volatile considered harmful" document

Re: [PATCH] "volatile considered harmful" document

[Robert Hancock]

Bill Davidsen wrote:
> Jonathan Corbet wrote:
> 
>> +There are still a few rare situations where volatile makes sense in the
>> +kernel:
>> +
>> +  - The above-mentioned accessor functions might use volatile on
>> +    architectures where direct I/O memory access does work.  
>> Essentially,
>> +    each accessor call becomes a little critical section on its own and
>> +    ensures that the access happens as expected by the programmer.
>> +
>> +  - Inline assembly code which changes memory, but which has no other
>> +    visible side effects, risks being deleted by GCC.  Adding the 
>> volatile
>> +    keyword to asm statements will prevent this removal.
>> +
>> +  - The jiffies variable is special in that it can have a different 
>> value
>> +    every time it is referenced, but it can be read without any special
>> +    locking.  So jiffies can be volatile, but the addition of other
>> +    variables of this type is frowned upon.  Jiffies is considered to 
>> be a
>> +    "stupid legacy" issue in this regard.
> 
> It would seem that any variable which is (a) subject to change by other 
> threads or hardware, and (b) the value of which is going to be used 
> without writing the variable, would be a valid use for volatile.

You don't need volatile in that case, rmb() can be used.

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: [PATCH] "volatile considered harmful" document

[Krzysztof Halasa]

Robert Hancock <hancockr@shaw.ca> writes:

> You don't need volatile in that case, rmb() can be used.

rmb() invalidates all compiler assumptions, it can be much slower.
-- 
Krzysztof Halasa

Re: [PATCH] "volatile considered harmful" document

[Bill Davidsen]

Krzysztof Halasa wrote:
> Robert Hancock <hancockr@shaw.ca> writes:
>
>   
>> You don't need volatile in that case, rmb() can be used.
>>     
>
> rmb() invalidates all compiler assumptions, it can be much slower.
>   
Yes, why would you use rmb() when a read of a volatile generates optimal 
code?

-- 
bill davidsen <davidsen@tmr.com>
  CTO TMR Associates, Inc
  Doing interesting things with small computers since 1979

Re: [PATCH] "volatile considered harmful" document

[Jeff Garzik]

On Sun, May 13, 2007 at 07:26:13PM -0400, Bill Davidsen wrote:
> Krzysztof Halasa wrote:
> >Robert Hancock <hancockr@shaw.ca> writes:
> >>You don't need volatile in that case, rmb() can be used.

> >rmb() invalidates all compiler assumptions, it can be much slower.

It does not invalidate /all/ assumptions.


> Yes, why would you use rmb() when a read of a volatile generates optimal 
> code?

Read of a volatile is guaranteed to generate the least optimal code.
That's what volatile does, guarantee no optimization of that particular
access.

	Jeff

Re: [PATCH] "volatile considered harmful" document

[Bill Davidsen]

Jeff Garzik wrote:
> On Sun, May 13, 2007 at 07:26:13PM -0400, Bill Davidsen wrote:
>   
>> Krzysztof Halasa wrote:
>>     
>>> Robert Hancock <hancockr@shaw.ca> writes:
>>>       
>>>> You don't need volatile in that case, rmb() can be used.
>>>>         
>
>   
>>> rmb() invalidates all compiler assumptions, it can be much slower.
>>>       
>
> It does not invalidate /all/ assumptions.
>
>
>   
>> Yes, why would you use rmb() when a read of a volatile generates optimal 
>> code?
>>     
>
> Read of a volatile is guaranteed to generate the least optimal code.
> That's what volatile does, guarantee no optimization of that particular
> access.
>   
By optimal you seem to mean "generate fewer CPU cycles by risking use of 
an obsolete value," while by the same term I mean read the correct and 
current value from the memory location without the overhead of locks. If 
your logic doesn't require the correct value, why read it at all? And if 
it does, how fewer cycles and cache impact can anything have than a 
single register load from memory?

Locks are useful when the value will be changed by a thread, or when the 
value must not be changed briefly. That's not always the case.

-- 
bill davidsen <davidsen@tmr.com>
  CTO TMR Associates, Inc
  Doing interesting things with small computers since 1979

[2.6.21] circular locking dependency found in QUOTA OFF

[Folkert van Heusden]

Hi,

When I cleanly reboot my pc running 2.6.21 on a P4 with HT and 2GB of ram
and system on an 1-filesystem IDE disk, I get the following circular
locking dependency error:

[330961.226405] =======================================================
[330961.226489] [ INFO: possible circular locking dependency detected ]
[330961.226531] 2.6.21 #5
[330961.226569] -------------------------------------------------------
[330961.226611] quotaoff/12249 is trying to acquire lock:
[330961.226652]  (&sb->s_type->i_mutex_key#4){--..}, at: [<c120e2a1>]
mutex_lock+0x8/0xa
[330961.226861]
[330961.226862] but task is already holding lock:
[330961.226938]  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
mutex_lock+0x8/0xa
[330961.227111]
[330961.227111] which lock already depends on the new lock.
[330961.227112]
[330961.227225]
[330961.227225] the existing dependency chain (in reverse order) is:
[330961.227303]
[330961.227303] -> #1 (&s->s_dquot.dqonoff_mutex){--..}:
[330961.227473]        [<c1039b02>] check_prev_add+0x15b/0x281
[330961.227766]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
[330961.228056]        [<c103b683>] __lock_acquire+0x692/0xb81
[330961.228353]        [<c103bfda>] lock_acquire+0x62/0x81
[330961.228643]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
[330961.228934]        [<c120e2a1>] mutex_lock+0x8/0xa
[330961.229221]        [<c109fbbe>] vfs_quota_on_inode+0xc1/0x25f
[330961.229513]        [<c109fdd1>] vfs_quota_on+0x75/0x79
[330961.229803]        [<c10bc92d>] ext3_quota_on+0x95/0xb0
[330961.230093]        [<c10a1eb2>] do_quotactl+0xc9/0x2dd
[330961.230384]        [<c10a214a>] sys_quotactl+0x84/0xd6
[330961.230673]        [<c1003f74>] syscall_call+0x7/0xb
[330961.230963]        [<ffffffff>] 0xffffffff
[330961.231268]
[330961.231268] -> #0 (&sb->s_type->i_mutex_key#4){--..}:
[330961.231469]        [<c10399db>] check_prev_add+0x34/0x281
[330961.231759]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
[330961.232049]        [<c103b683>] __lock_acquire+0x692/0xb81
[330961.232344]        [<c103bfda>] lock_acquire+0x62/0x81
[330961.232632]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
[330961.232923]        [<c120e2a1>] mutex_lock+0x8/0xa
[330961.233211]        [<c109fa6c>] vfs_quota_off+0x1cf/0x260
[330961.233500]        [<c10a2088>] do_quotactl+0x29f/0x2dd
[330961.233792]        [<c10a214a>] sys_quotactl+0x84/0xd6
[330961.234081]        [<c1003f74>] syscall_call+0x7/0xb
[330961.234503]        [<ffffffff>] 0xffffffff
[330961.234795]
[330961.234795] other info that might help us debug this:
[330961.234796]
[330961.234908] 2 locks held by quotaoff/12249:
[330961.234947]  #0:  (&type->s_umount_key#15){----}, at: [<c1070b5d>]
get_super+0x53/0x94
[330961.235183]  #1:  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
mutex_lock+0x8/0xa
[330961.235386]
[330961.235387] stack backtrace:
[330961.235462]  [<c1004d53>] show_trace_log_lvl+0x1a/0x30
[330961.235535]  [<c1004d7b>] show_trace+0x12/0x14
[330961.235606]  [<c1004e75>] dump_stack+0x16/0x18
[330961.235679]  [<c1039352>] print_circular_bug_tail+0x6f/0x71
[330961.235753]  [<c10399db>] check_prev_add+0x34/0x281
[330961.235825]  [<c1039cb3>] check_prevs_add+0x8b/0xe8
[330961.235897]  [<c103b683>] __lock_acquire+0x692/0xb81
[330961.235969]  [<c103bfda>] lock_acquire+0x62/0x81
[330961.236041]  [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
[330961.236113]  [<c120e2a1>] mutex_lock+0x8/0xa
[330961.236185]  [<c109fa6c>] vfs_quota_off+0x1cf/0x260
[330961.236257]  [<c10a2088>] do_quotactl+0x29f/0x2dd
[330961.236330]  [<c10a214a>] sys_quotactl+0x84/0xd6
[330961.236402]  [<c1003f74>] syscall_call+0x7/0xb
[330961.236473]  =======================

Re: [2.6.21] circular locking dependency found in QUOTA OFF

[Michal Piotrowski]

[adding Jan and fsdevel to CC]

Hi Folkert,

On 14/05/07, Folkert van Heusden <folkert@vanheusden.com> wrote:
> Hi,
>
> When I cleanly reboot my pc running 2.6.21 on a P4 with HT and 2GB of ram
> and system on an 1-filesystem IDE disk, I get the following circular
> locking dependency error:
>
> [330961.226405] =======================================================
> [330961.226489] [ INFO: possible circular locking dependency detected ]
> [330961.226531] 2.6.21 #5
> [330961.226569] -------------------------------------------------------
> [330961.226611] quotaoff/12249 is trying to acquire lock:
> [330961.226652]  (&sb->s_type->i_mutex_key#4){--..}, at: [<c120e2a1>]
> mutex_lock+0x8/0xa
> [330961.226861]
> [330961.226862] but task is already holding lock:
> [330961.226938]  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
> mutex_lock+0x8/0xa
> [330961.227111]
> [330961.227111] which lock already depends on the new lock.
> [330961.227112]
> [330961.227225]
> [330961.227225] the existing dependency chain (in reverse order) is:
> [330961.227303]
> [330961.227303] -> #1 (&s->s_dquot.dqonoff_mutex){--..}:
> [330961.227473]        [<c1039b02>] check_prev_add+0x15b/0x281
> [330961.227766]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
> [330961.228056]        [<c103b683>] __lock_acquire+0x692/0xb81
> [330961.228353]        [<c103bfda>] lock_acquire+0x62/0x81
> [330961.228643]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> [330961.228934]        [<c120e2a1>] mutex_lock+0x8/0xa
> [330961.229221]        [<c109fbbe>] vfs_quota_on_inode+0xc1/0x25f
> [330961.229513]        [<c109fdd1>] vfs_quota_on+0x75/0x79
> [330961.229803]        [<c10bc92d>] ext3_quota_on+0x95/0xb0
> [330961.230093]        [<c10a1eb2>] do_quotactl+0xc9/0x2dd
> [330961.230384]        [<c10a214a>] sys_quotactl+0x84/0xd6
> [330961.230673]        [<c1003f74>] syscall_call+0x7/0xb
> [330961.230963]        [<ffffffff>] 0xffffffff
> [330961.231268]
> [330961.231268] -> #0 (&sb->s_type->i_mutex_key#4){--..}:
> [330961.231469]        [<c10399db>] check_prev_add+0x34/0x281
> [330961.231759]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
> [330961.232049]        [<c103b683>] __lock_acquire+0x692/0xb81
> [330961.232344]        [<c103bfda>] lock_acquire+0x62/0x81
> [330961.232632]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> [330961.232923]        [<c120e2a1>] mutex_lock+0x8/0xa
> [330961.233211]        [<c109fa6c>] vfs_quota_off+0x1cf/0x260
> [330961.233500]        [<c10a2088>] do_quotactl+0x29f/0x2dd
> [330961.233792]        [<c10a214a>] sys_quotactl+0x84/0xd6
> [330961.234081]        [<c1003f74>] syscall_call+0x7/0xb
> [330961.234503]        [<ffffffff>] 0xffffffff
> [330961.234795]
> [330961.234795] other info that might help us debug this:
> [330961.234796]
> [330961.234908] 2 locks held by quotaoff/12249:
> [330961.234947]  #0:  (&type->s_umount_key#15){----}, at: [<c1070b5d>]
> get_super+0x53/0x94
> [330961.235183]  #1:  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
> mutex_lock+0x8/0xa
> [330961.235386]
> [330961.235387] stack backtrace:
> [330961.235462]  [<c1004d53>] show_trace_log_lvl+0x1a/0x30
> [330961.235535]  [<c1004d7b>] show_trace+0x12/0x14
> [330961.235606]  [<c1004e75>] dump_stack+0x16/0x18
> [330961.235679]  [<c1039352>] print_circular_bug_tail+0x6f/0x71
> [330961.235753]  [<c10399db>] check_prev_add+0x34/0x281
> [330961.235825]  [<c1039cb3>] check_prevs_add+0x8b/0xe8
> [330961.235897]  [<c103b683>] __lock_acquire+0x692/0xb81
> [330961.235969]  [<c103bfda>] lock_acquire+0x62/0x81
> [330961.236041]  [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> [330961.236113]  [<c120e2a1>] mutex_lock+0x8/0xa
> [330961.236185]  [<c109fa6c>] vfs_quota_off+0x1cf/0x260
> [330961.236257]  [<c10a2088>] do_quotactl+0x29f/0x2dd
> [330961.236330]  [<c10a214a>] sys_quotactl+0x84/0xd6
> [330961.236402]  [<c1003f74>] syscall_call+0x7/0xb
> [330961.236473]  =======================
>

Is this a 2.6.21 regression?

Regards,
Michal

-- 
Michal K. K. Piotrowski
Kernel Monkeys
(http://kernel.wikidot.com/start)

Re: [2.6.21] circular locking dependency found in QUOTA OFF

[Folkert van Heusden]

> [adding Jan and fsdevel to CC]
> Hi Folkert,
> >When I cleanly reboot my pc running 2.6.21 on a P4 with HT and 2GB of ram
> >and system on an 1-filesystem IDE disk, I get the following circular
> >locking dependency error:
> >
> >[330961.226405] =======================================================
> >[330961.226489] [ INFO: possible circular locking dependency detected ]
> >[330961.226531] 2.6.21 #5
...
> >[330961.236402]  [<c1003f74>] syscall_call+0x7/0xb
> >[330961.236473]  =======================
> 
> Is this a 2.6.21 regression?

This is new for 2.6.21, yes.


Folkert van Heusden

-- 
MultiTail est un flexible tool pour suivre de logfiles et execution de
commandements. Filtrer, pourvoir de couleur, merge, 'diff-view', etc.
http://www.vanheusden.com/multitail/
----------------------------------------------------------------------
Phone: +31-6-41278122, PGP-key: 1F28D8AE, www.vanheusden.com

Re: [2.6.21] circular locking dependency found in QUOTA OFF

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 4579 bytes --]

  Hi,

  Thanks for report. It seems like a lockdep problem. i_mutex for quota
files is below dqonoff_sem. We were already fixing this for filesystem
specific quota IO functions but obviously we've missed a few cases. I
wonder why it showed up only now... Anyway, attached is a fix. Andrew,
would you add it? Thanks.

								Honza

On Mon 14-05-07 19:43:18, Michal Piotrowski wrote:
> [adding Jan and fsdevel to CC]
> 
> Hi Folkert,
> 
> On 14/05/07, Folkert van Heusden <folkert@vanheusden.com> wrote:
> >Hi,
> >
> >When I cleanly reboot my pc running 2.6.21 on a P4 with HT and 2GB of ram
> >and system on an 1-filesystem IDE disk, I get the following circular
> >locking dependency error:
> >
> >[330961.226405] =======================================================
> >[330961.226489] [ INFO: possible circular locking dependency detected ]
> >[330961.226531] 2.6.21 #5
> >[330961.226569] -------------------------------------------------------
> >[330961.226611] quotaoff/12249 is trying to acquire lock:
> >[330961.226652]  (&sb->s_type->i_mutex_key#4){--..}, at: [<c120e2a1>]
> >mutex_lock+0x8/0xa
> >[330961.226861]
> >[330961.226862] but task is already holding lock:
> >[330961.226938]  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
> >mutex_lock+0x8/0xa
> >[330961.227111]
> >[330961.227111] which lock already depends on the new lock.
> >[330961.227112]
> >[330961.227225]
> >[330961.227225] the existing dependency chain (in reverse order) is:
> >[330961.227303]
> >[330961.227303] -> #1 (&s->s_dquot.dqonoff_mutex){--..}:
> >[330961.227473]        [<c1039b02>] check_prev_add+0x15b/0x281
> >[330961.227766]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
> >[330961.228056]        [<c103b683>] __lock_acquire+0x692/0xb81
> >[330961.228353]        [<c103bfda>] lock_acquire+0x62/0x81
> >[330961.228643]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> >[330961.228934]        [<c120e2a1>] mutex_lock+0x8/0xa
> >[330961.229221]        [<c109fbbe>] vfs_quota_on_inode+0xc1/0x25f
> >[330961.229513]        [<c109fdd1>] vfs_quota_on+0x75/0x79
> >[330961.229803]        [<c10bc92d>] ext3_quota_on+0x95/0xb0
> >[330961.230093]        [<c10a1eb2>] do_quotactl+0xc9/0x2dd
> >[330961.230384]        [<c10a214a>] sys_quotactl+0x84/0xd6
> >[330961.230673]        [<c1003f74>] syscall_call+0x7/0xb
> >[330961.230963]        [<ffffffff>] 0xffffffff
> >[330961.231268]
> >[330961.231268] -> #0 (&sb->s_type->i_mutex_key#4){--..}:
> >[330961.231469]        [<c10399db>] check_prev_add+0x34/0x281
> >[330961.231759]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
> >[330961.232049]        [<c103b683>] __lock_acquire+0x692/0xb81
> >[330961.232344]        [<c103bfda>] lock_acquire+0x62/0x81
> >[330961.232632]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> >[330961.232923]        [<c120e2a1>] mutex_lock+0x8/0xa
> >[330961.233211]        [<c109fa6c>] vfs_quota_off+0x1cf/0x260
> >[330961.233500]        [<c10a2088>] do_quotactl+0x29f/0x2dd
> >[330961.233792]        [<c10a214a>] sys_quotactl+0x84/0xd6
> >[330961.234081]        [<c1003f74>] syscall_call+0x7/0xb
> >[330961.234503]        [<ffffffff>] 0xffffffff
> >[330961.234795]
> >[330961.234795] other info that might help us debug this:
> >[330961.234796]
> >[330961.234908] 2 locks held by quotaoff/12249:
> >[330961.234947]  #0:  (&type->s_umount_key#15){----}, at: [<c1070b5d>]
> >get_super+0x53/0x94
> >[330961.235183]  #1:  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
> >mutex_lock+0x8/0xa
> >[330961.235386]
> >[330961.235387] stack backtrace:
> >[330961.235462]  [<c1004d53>] show_trace_log_lvl+0x1a/0x30
> >[330961.235535]  [<c1004d7b>] show_trace+0x12/0x14
> >[330961.235606]  [<c1004e75>] dump_stack+0x16/0x18
> >[330961.235679]  [<c1039352>] print_circular_bug_tail+0x6f/0x71
> >[330961.235753]  [<c10399db>] check_prev_add+0x34/0x281
> >[330961.235825]  [<c1039cb3>] check_prevs_add+0x8b/0xe8
> >[330961.235897]  [<c103b683>] __lock_acquire+0x692/0xb81
> >[330961.235969]  [<c103bfda>] lock_acquire+0x62/0x81
> >[330961.236041]  [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> >[330961.236113]  [<c120e2a1>] mutex_lock+0x8/0xa
> >[330961.236185]  [<c109fa6c>] vfs_quota_off+0x1cf/0x260
> >[330961.236257]  [<c10a2088>] do_quotactl+0x29f/0x2dd
> >[330961.236330]  [<c10a214a>] sys_quotactl+0x84/0xd6
> >[330961.236402]  [<c1003f74>] syscall_call+0x7/0xb
> >[330961.236473]  =======================
> >
> 
> Is this a 2.6.21 regression?
> 
> Regards,
> Michal
> 
> -- 
> Michal K. K. Piotrowski
> Kernel Monkeys
> (http://kernel.wikidot.com/start)
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

[-- Attachment #2: quota-2.6.21-1-quota_lockdep.diff --]
[-- Type: text/x-patch, Size: 2940 bytes --]

i_mutex on quota files is special. Unlike i_mutexes for other inodes it is
acquired under dqonoff_mutex. Tell lockdep about this lock ranking. Also
comment and code in quota_sync_sb() seem to be bogus (as i_mutex for quota
file can be acquired under dqonoff_mutex). Move truncate_inode_pages()
call under dqonoff_mutex and save some problems with races...

Signed-off-by: Jan Kara <jack@suse.cz>

diff -rupX /home/jack/.kerndiffexclude linux-2.6.21/fs/dquot.c linux-2.6.21-1-quota_lockdep/fs/dquot.c
--- linux-2.6.21/fs/dquot.c	2007-05-15 14:18:47.000000000 +0200
+++ linux-2.6.21-1-quota_lockdep/fs/dquot.c	2007-05-15 14:22:47.000000000 +0200
@@ -1421,7 +1421,7 @@ int vfs_quota_off(struct super_block *sb
 			/* If quota was reenabled in the meantime, we have
 			 * nothing to do */
 			if (!sb_has_quota_enabled(sb, cnt)) {
-				mutex_lock(&toputinode[cnt]->i_mutex);
+				mutex_lock_nested(&toputinode[cnt]->i_mutex, I_MUTEX_QUOTA);
 				toputinode[cnt]->i_flags &= ~(S_IMMUTABLE |
 				  S_NOATIME | S_NOQUOTA);
 				truncate_inode_pages(&toputinode[cnt]->i_data, 0);
diff -rupX /home/jack/.kerndiffexclude linux-2.6.21/fs/quota.c linux-2.6.21-1-quota_lockdep/fs/quota.c
--- linux-2.6.21/fs/quota.c	2006-11-29 22:57:37.000000000 +0100
+++ linux-2.6.21-1-quota_lockdep/fs/quota.c	2007-05-15 15:15:44.000000000 +0200
@@ -158,7 +158,6 @@ static int check_quotactl_valid(struct s
 static void quota_sync_sb(struct super_block *sb, int type)
 {
 	int cnt;
-	struct inode *discard[MAXQUOTAS];
 
 	sb->s_qcop->quota_sync(sb, type);
 	/* This is not very clever (and fast) but currently I don't know about
@@ -168,29 +167,21 @@ static void quota_sync_sb(struct super_b
 		sb->s_op->sync_fs(sb, 1);
 	sync_blockdev(sb->s_bdev);
 
-	/* Now when everything is written we can discard the pagecache so
-	 * that userspace sees the changes. We need i_mutex and so we could
-	 * not do it inside dqonoff_mutex. Moreover we need to be carefull
-	 * about races with quotaoff() (that is the reason why we have own
-	 * reference to inode). */
+	/*
+	 * Now when everything is written we can discard the pagecache so
+	 * that userspace sees the changes.
+	 */
 	mutex_lock(&sb_dqopt(sb)->dqonoff_mutex);
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
-		discard[cnt] = NULL;
 		if (type != -1 && cnt != type)
 			continue;
 		if (!sb_has_quota_enabled(sb, cnt))
 			continue;
-		discard[cnt] = igrab(sb_dqopt(sb)->files[cnt]);
+		mutex_lock_nested(&sb_dqopt(sb)->files[cnt]->i_mutex, I_MUTEX_NESTED);
+		truncate_inode_pages(&sb_dqopt(sb)->files[cnt]->i_data, 0);
+		mutex_unlock(&sb_dqopt(sb)->files[cnt]->i_mutex);
 	}
 	mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
-	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
-		if (discard[cnt]) {
-			mutex_lock(&discard[cnt]->i_mutex);
-			truncate_inode_pages(&discard[cnt]->i_data, 0);
-			mutex_unlock(&discard[cnt]->i_mutex);
-			iput(discard[cnt]);
-		}
-	}
 }
 
 void sync_dquots(struct super_block *sb, int type)

Re: [2.6.21] circular locking dependency found in QUOTA OFF

[Folkert van Heusden]

I'm afraid it doesn't compile:

  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  CHK     include/linux/compile.h
  CC      fs/dquot.o
  CC      fs/quota.o
fs/quota.c: In function `quota_sync_sb':
fs/quota.c:180: error: `I_MUTEX_NESTED' undeclared (first use in this function)
fs/quota.c:180: error: (Each undeclared identifier is reported only once
fs/quota.c:180: error: for each function it appears in.)
make[1]: *** [fs/quota.o] Error 1
make: *** [fs] Error 2


On Tue, May 15, 2007 at 04:09:39PM +0200, Jan Kara wrote:
>   Hi,
> 
>   Thanks for report. It seems like a lockdep problem. i_mutex for quota
> files is below dqonoff_sem. We were already fixing this for filesystem
> specific quota IO functions but obviously we've missed a few cases. I
> wonder why it showed up only now... Anyway, attached is a fix. Andrew,
> would you add it? Thanks.
> 
> 								Honza
> 
> On Mon 14-05-07 19:43:18, Michal Piotrowski wrote:
> > [adding Jan and fsdevel to CC]
> > 
> > Hi Folkert,
> > 
> > On 14/05/07, Folkert van Heusden <folkert@vanheusden.com> wrote:
> > >Hi,
> > >
> > >When I cleanly reboot my pc running 2.6.21 on a P4 with HT and 2GB of ram
> > >and system on an 1-filesystem IDE disk, I get the following circular
> > >locking dependency error:
> > >
> > >[330961.226405] =======================================================
> > >[330961.226489] [ INFO: possible circular locking dependency detected ]
> > >[330961.226531] 2.6.21 #5
> > >[330961.226569] -------------------------------------------------------
> > >[330961.226611] quotaoff/12249 is trying to acquire lock:
> > >[330961.226652]  (&sb->s_type->i_mutex_key#4){--..}, at: [<c120e2a1>]
> > >mutex_lock+0x8/0xa
> > >[330961.226861]
> > >[330961.226862] but task is already holding lock:
> > >[330961.226938]  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
> > >mutex_lock+0x8/0xa
> > >[330961.227111]
> > >[330961.227111] which lock already depends on the new lock.
> > >[330961.227112]
> > >[330961.227225]
> > >[330961.227225] the existing dependency chain (in reverse order) is:
> > >[330961.227303]
> > >[330961.227303] -> #1 (&s->s_dquot.dqonoff_mutex){--..}:
> > >[330961.227473]        [<c1039b02>] check_prev_add+0x15b/0x281
> > >[330961.227766]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
> > >[330961.228056]        [<c103b683>] __lock_acquire+0x692/0xb81
> > >[330961.228353]        [<c103bfda>] lock_acquire+0x62/0x81
> > >[330961.228643]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> > >[330961.228934]        [<c120e2a1>] mutex_lock+0x8/0xa
> > >[330961.229221]        [<c109fbbe>] vfs_quota_on_inode+0xc1/0x25f
> > >[330961.229513]        [<c109fdd1>] vfs_quota_on+0x75/0x79
> > >[330961.229803]        [<c10bc92d>] ext3_quota_on+0x95/0xb0
> > >[330961.230093]        [<c10a1eb2>] do_quotactl+0xc9/0x2dd
> > >[330961.230384]        [<c10a214a>] sys_quotactl+0x84/0xd6
> > >[330961.230673]        [<c1003f74>] syscall_call+0x7/0xb
> > >[330961.230963]        [<ffffffff>] 0xffffffff
> > >[330961.231268]
> > >[330961.231268] -> #0 (&sb->s_type->i_mutex_key#4){--..}:
> > >[330961.231469]        [<c10399db>] check_prev_add+0x34/0x281
> > >[330961.231759]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
> > >[330961.232049]        [<c103b683>] __lock_acquire+0x692/0xb81
> > >[330961.232344]        [<c103bfda>] lock_acquire+0x62/0x81
> > >[330961.232632]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> > >[330961.232923]        [<c120e2a1>] mutex_lock+0x8/0xa
> > >[330961.233211]        [<c109fa6c>] vfs_quota_off+0x1cf/0x260
> > >[330961.233500]        [<c10a2088>] do_quotactl+0x29f/0x2dd
> > >[330961.233792]        [<c10a214a>] sys_quotactl+0x84/0xd6
> > >[330961.234081]        [<c1003f74>] syscall_call+0x7/0xb
> > >[330961.234503]        [<ffffffff>] 0xffffffff
> > >[330961.234795]
> > >[330961.234795] other info that might help us debug this:
> > >[330961.234796]
> > >[330961.234908] 2 locks held by quotaoff/12249:
> > >[330961.234947]  #0:  (&type->s_umount_key#15){----}, at: [<c1070b5d>]
> > >get_super+0x53/0x94
> > >[330961.235183]  #1:  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
> > >mutex_lock+0x8/0xa
> > >[330961.235386]
> > >[330961.235387] stack backtrace:
> > >[330961.235462]  [<c1004d53>] show_trace_log_lvl+0x1a/0x30
> > >[330961.235535]  [<c1004d7b>] show_trace+0x12/0x14
> > >[330961.235606]  [<c1004e75>] dump_stack+0x16/0x18
> > >[330961.235679]  [<c1039352>] print_circular_bug_tail+0x6f/0x71
> > >[330961.235753]  [<c10399db>] check_prev_add+0x34/0x281
> > >[330961.235825]  [<c1039cb3>] check_prevs_add+0x8b/0xe8
> > >[330961.235897]  [<c103b683>] __lock_acquire+0x692/0xb81
> > >[330961.235969]  [<c103bfda>] lock_acquire+0x62/0x81
> > >[330961.236041]  [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> > >[330961.236113]  [<c120e2a1>] mutex_lock+0x8/0xa
> > >[330961.236185]  [<c109fa6c>] vfs_quota_off+0x1cf/0x260
> > >[330961.236257]  [<c10a2088>] do_quotactl+0x29f/0x2dd
> > >[330961.236330]  [<c10a214a>] sys_quotactl+0x84/0xd6
> > >[330961.236402]  [<c1003f74>] syscall_call+0x7/0xb
> > >[330961.236473]  =======================
> > >
> > 
> > Is this a 2.6.21 regression?
> > 
> > Regards,
> > Michal
> > 
> > -- 
> > Michal K. K. Piotrowski
> > Kernel Monkeys
> > (http://kernel.wikidot.com/start)
> -- 
> Jan Kara <jack@suse.cz>
> SuSE CR Labs

> i_mutex on quota files is special. Unlike i_mutexes for other inodes it is
> acquired under dqonoff_mutex. Tell lockdep about this lock ranking. Also
> comment and code in quota_sync_sb() seem to be bogus (as i_mutex for quota
> file can be acquired under dqonoff_mutex). Move truncate_inode_pages()
> call under dqonoff_mutex and save some problems with races...
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> 
> diff -rupX /home/jack/.kerndiffexclude linux-2.6.21/fs/dquot.c linux-2.6.21-1-quota_lockdep/fs/dquot.c
> --- linux-2.6.21/fs/dquot.c	2007-05-15 14:18:47.000000000 +0200
> +++ linux-2.6.21-1-quota_lockdep/fs/dquot.c	2007-05-15 14:22:47.000000000 +0200
> @@ -1421,7 +1421,7 @@ int vfs_quota_off(struct super_block *sb
>  			/* If quota was reenabled in the meantime, we have
>  			 * nothing to do */
>  			if (!sb_has_quota_enabled(sb, cnt)) {
> -				mutex_lock(&toputinode[cnt]->i_mutex);
> +				mutex_lock_nested(&toputinode[cnt]->i_mutex, I_MUTEX_QUOTA);
>  				toputinode[cnt]->i_flags &= ~(S_IMMUTABLE |
>  				  S_NOATIME | S_NOQUOTA);
>  				truncate_inode_pages(&toputinode[cnt]->i_data, 0);
> diff -rupX /home/jack/.kerndiffexclude linux-2.6.21/fs/quota.c linux-2.6.21-1-quota_lockdep/fs/quota.c
> --- linux-2.6.21/fs/quota.c	2006-11-29 22:57:37.000000000 +0100
> +++ linux-2.6.21-1-quota_lockdep/fs/quota.c	2007-05-15 15:15:44.000000000 +0200
> @@ -158,7 +158,6 @@ static int check_quotactl_valid(struct s
>  static void quota_sync_sb(struct super_block *sb, int type)
>  {
>  	int cnt;
> -	struct inode *discard[MAXQUOTAS];
>  
>  	sb->s_qcop->quota_sync(sb, type);
>  	/* This is not very clever (and fast) but currently I don't know about
> @@ -168,29 +167,21 @@ static void quota_sync_sb(struct super_b
>  		sb->s_op->sync_fs(sb, 1);
>  	sync_blockdev(sb->s_bdev);
>  
> -	/* Now when everything is written we can discard the pagecache so
> -	 * that userspace sees the changes. We need i_mutex and so we could
> -	 * not do it inside dqonoff_mutex. Moreover we need to be carefull
> -	 * about races with quotaoff() (that is the reason why we have own
> -	 * reference to inode). */
> +	/*
> +	 * Now when everything is written we can discard the pagecache so
> +	 * that userspace sees the changes.
> +	 */
>  	mutex_lock(&sb_dqopt(sb)->dqonoff_mutex);
>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> -		discard[cnt] = NULL;
>  		if (type != -1 && cnt != type)
>  			continue;
>  		if (!sb_has_quota_enabled(sb, cnt))
>  			continue;
> -		discard[cnt] = igrab(sb_dqopt(sb)->files[cnt]);
> +		mutex_lock_nested(&sb_dqopt(sb)->files[cnt]->i_mutex, I_MUTEX_NESTED);
> +		truncate_inode_pages(&sb_dqopt(sb)->files[cnt]->i_data, 0);
> +		mutex_unlock(&sb_dqopt(sb)->files[cnt]->i_mutex);
>  	}
>  	mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
> -	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> -		if (discard[cnt]) {
> -			mutex_lock(&discard[cnt]->i_mutex);
> -			truncate_inode_pages(&discard[cnt]->i_data, 0);
> -			mutex_unlock(&discard[cnt]->i_mutex);
> -			iput(discard[cnt]);
> -		}
> -	}
>  }
>  
>  void sync_dquots(struct super_block *sb, int type)



Folkert van Heusden

-- 
www.vanheusden.com/multitail - win een vlaai van multivlaai! zorg
ervoor dat multitail opgenomen wordt in Fedora Core, AIX, Solaris of
HP/UX en win een vlaai naar keuze
----------------------------------------------------------------------
Phone: +31-6-41278122, PGP-key: 1F28D8AE, www.vanheusden.com

Re: [2.6.21] circular locking dependency found in QUOTA OFF

[Jan Kara]

On Tue 15-05-07 19:52:03, Folkert van Heusden wrote:
> I'm afraid it doesn't compile:
> 
>   CHK     include/linux/version.h
>   CHK     include/linux/utsrelease.h
>   CHK     include/linux/compile.h
>   CC      fs/dquot.o
>   CC      fs/quota.o
> fs/quota.c: In function `quota_sync_sb':
> fs/quota.c:180: error: `I_MUTEX_NESTED' undeclared (first use in this function)
> fs/quota.c:180: error: (Each undeclared identifier is reported only once
> fs/quota.c:180: error: for each function it appears in.)
> make[1]: *** [fs/quota.o] Error 1
> make: *** [fs] Error 2
  Huh, I has compiled just fine for me... Can you send me your .config?
Thanks.

									Honza

> On Tue, May 15, 2007 at 04:09:39PM +0200, Jan Kara wrote:
> >   Hi,
> > 
> >   Thanks for report. It seems like a lockdep problem. i_mutex for quota
> > files is below dqonoff_sem. We were already fixing this for filesystem
> > specific quota IO functions but obviously we've missed a few cases. I
> > wonder why it showed up only now... Anyway, attached is a fix. Andrew,
> > would you add it? Thanks.
> > 
> > 								Honza
> > 
> > On Mon 14-05-07 19:43:18, Michal Piotrowski wrote:
> > > [adding Jan and fsdevel to CC]
> > > 
> > > Hi Folkert,
> > > 
> > > On 14/05/07, Folkert van Heusden <folkert@vanheusden.com> wrote:
> > > >Hi,
> > > >
> > > >When I cleanly reboot my pc running 2.6.21 on a P4 with HT and 2GB of ram
> > > >and system on an 1-filesystem IDE disk, I get the following circular
> > > >locking dependency error:
> > > >
> > > >[330961.226405] =======================================================
> > > >[330961.226489] [ INFO: possible circular locking dependency detected ]
> > > >[330961.226531] 2.6.21 #5
> > > >[330961.226569] -------------------------------------------------------
> > > >[330961.226611] quotaoff/12249 is trying to acquire lock:
> > > >[330961.226652]  (&sb->s_type->i_mutex_key#4){--..}, at: [<c120e2a1>]
> > > >mutex_lock+0x8/0xa
> > > >[330961.226861]
> > > >[330961.226862] but task is already holding lock:
> > > >[330961.226938]  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
> > > >mutex_lock+0x8/0xa
> > > >[330961.227111]
> > > >[330961.227111] which lock already depends on the new lock.
> > > >[330961.227112]
> > > >[330961.227225]
> > > >[330961.227225] the existing dependency chain (in reverse order) is:
> > > >[330961.227303]
> > > >[330961.227303] -> #1 (&s->s_dquot.dqonoff_mutex){--..}:
> > > >[330961.227473]        [<c1039b02>] check_prev_add+0x15b/0x281
> > > >[330961.227766]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
> > > >[330961.228056]        [<c103b683>] __lock_acquire+0x692/0xb81
> > > >[330961.228353]        [<c103bfda>] lock_acquire+0x62/0x81
> > > >[330961.228643]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> > > >[330961.228934]        [<c120e2a1>] mutex_lock+0x8/0xa
> > > >[330961.229221]        [<c109fbbe>] vfs_quota_on_inode+0xc1/0x25f
> > > >[330961.229513]        [<c109fdd1>] vfs_quota_on+0x75/0x79
> > > >[330961.229803]        [<c10bc92d>] ext3_quota_on+0x95/0xb0
> > > >[330961.230093]        [<c10a1eb2>] do_quotactl+0xc9/0x2dd
> > > >[330961.230384]        [<c10a214a>] sys_quotactl+0x84/0xd6
> > > >[330961.230673]        [<c1003f74>] syscall_call+0x7/0xb
> > > >[330961.230963]        [<ffffffff>] 0xffffffff
> > > >[330961.231268]
> > > >[330961.231268] -> #0 (&sb->s_type->i_mutex_key#4){--..}:
> > > >[330961.231469]        [<c10399db>] check_prev_add+0x34/0x281
> > > >[330961.231759]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
> > > >[330961.232049]        [<c103b683>] __lock_acquire+0x692/0xb81
> > > >[330961.232344]        [<c103bfda>] lock_acquire+0x62/0x81
> > > >[330961.232632]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> > > >[330961.232923]        [<c120e2a1>] mutex_lock+0x8/0xa
> > > >[330961.233211]        [<c109fa6c>] vfs_quota_off+0x1cf/0x260
> > > >[330961.233500]        [<c10a2088>] do_quotactl+0x29f/0x2dd
> > > >[330961.233792]        [<c10a214a>] sys_quotactl+0x84/0xd6
> > > >[330961.234081]        [<c1003f74>] syscall_call+0x7/0xb
> > > >[330961.234503]        [<ffffffff>] 0xffffffff
> > > >[330961.234795]
> > > >[330961.234795] other info that might help us debug this:
> > > >[330961.234796]
> > > >[330961.234908] 2 locks held by quotaoff/12249:
> > > >[330961.234947]  #0:  (&type->s_umount_key#15){----}, at: [<c1070b5d>]
> > > >get_super+0x53/0x94
> > > >[330961.235183]  #1:  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
> > > >mutex_lock+0x8/0xa
> > > >[330961.235386]
> > > >[330961.235387] stack backtrace:
> > > >[330961.235462]  [<c1004d53>] show_trace_log_lvl+0x1a/0x30
> > > >[330961.235535]  [<c1004d7b>] show_trace+0x12/0x14
> > > >[330961.235606]  [<c1004e75>] dump_stack+0x16/0x18
> > > >[330961.235679]  [<c1039352>] print_circular_bug_tail+0x6f/0x71
> > > >[330961.235753]  [<c10399db>] check_prev_add+0x34/0x281
> > > >[330961.235825]  [<c1039cb3>] check_prevs_add+0x8b/0xe8
> > > >[330961.235897]  [<c103b683>] __lock_acquire+0x692/0xb81
> > > >[330961.235969]  [<c103bfda>] lock_acquire+0x62/0x81
> > > >[330961.236041]  [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> > > >[330961.236113]  [<c120e2a1>] mutex_lock+0x8/0xa
> > > >[330961.236185]  [<c109fa6c>] vfs_quota_off+0x1cf/0x260
> > > >[330961.236257]  [<c10a2088>] do_quotactl+0x29f/0x2dd
> > > >[330961.236330]  [<c10a214a>] sys_quotactl+0x84/0xd6
> > > >[330961.236402]  [<c1003f74>] syscall_call+0x7/0xb
> > > >[330961.236473]  =======================
> > > >
> > > 
> > > Is this a 2.6.21 regression?
> > > 
> > > Regards,
> > > Michal
> > > 
> > > -- 
> > > Michal K. K. Piotrowski
> > > Kernel Monkeys
> > > (http://kernel.wikidot.com/start)
> > -- 
> > Jan Kara <jack@suse.cz>
> > SuSE CR Labs
> 
> > i_mutex on quota files is special. Unlike i_mutexes for other inodes it is
> > acquired under dqonoff_mutex. Tell lockdep about this lock ranking. Also
> > comment and code in quota_sync_sb() seem to be bogus (as i_mutex for quota
> > file can be acquired under dqonoff_mutex). Move truncate_inode_pages()
> > call under dqonoff_mutex and save some problems with races...
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > 
> > diff -rupX /home/jack/.kerndiffexclude linux-2.6.21/fs/dquot.c linux-2.6.21-1-quota_lockdep/fs/dquot.c
> > --- linux-2.6.21/fs/dquot.c	2007-05-15 14:18:47.000000000 +0200
> > +++ linux-2.6.21-1-quota_lockdep/fs/dquot.c	2007-05-15 14:22:47.000000000 +0200
> > @@ -1421,7 +1421,7 @@ int vfs_quota_off(struct super_block *sb
> >  			/* If quota was reenabled in the meantime, we have
> >  			 * nothing to do */
> >  			if (!sb_has_quota_enabled(sb, cnt)) {
> > -				mutex_lock(&toputinode[cnt]->i_mutex);
> > +				mutex_lock_nested(&toputinode[cnt]->i_mutex, I_MUTEX_QUOTA);
> >  				toputinode[cnt]->i_flags &= ~(S_IMMUTABLE |
> >  				  S_NOATIME | S_NOQUOTA);
> >  				truncate_inode_pages(&toputinode[cnt]->i_data, 0);
> > diff -rupX /home/jack/.kerndiffexclude linux-2.6.21/fs/quota.c linux-2.6.21-1-quota_lockdep/fs/quota.c
> > --- linux-2.6.21/fs/quota.c	2006-11-29 22:57:37.000000000 +0100
> > +++ linux-2.6.21-1-quota_lockdep/fs/quota.c	2007-05-15 15:15:44.000000000 +0200
> > @@ -158,7 +158,6 @@ static int check_quotactl_valid(struct s
> >  static void quota_sync_sb(struct super_block *sb, int type)
> >  {
> >  	int cnt;
> > -	struct inode *discard[MAXQUOTAS];
> >  
> >  	sb->s_qcop->quota_sync(sb, type);
> >  	/* This is not very clever (and fast) but currently I don't know about
> > @@ -168,29 +167,21 @@ static void quota_sync_sb(struct super_b
> >  		sb->s_op->sync_fs(sb, 1);
> >  	sync_blockdev(sb->s_bdev);
> >  
> > -	/* Now when everything is written we can discard the pagecache so
> > -	 * that userspace sees the changes. We need i_mutex and so we could
> > -	 * not do it inside dqonoff_mutex. Moreover we need to be carefull
> > -	 * about races with quotaoff() (that is the reason why we have own
> > -	 * reference to inode). */
> > +	/*
> > +	 * Now when everything is written we can discard the pagecache so
> > +	 * that userspace sees the changes.
> > +	 */
> >  	mutex_lock(&sb_dqopt(sb)->dqonoff_mutex);
> >  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> > -		discard[cnt] = NULL;
> >  		if (type != -1 && cnt != type)
> >  			continue;
> >  		if (!sb_has_quota_enabled(sb, cnt))
> >  			continue;
> > -		discard[cnt] = igrab(sb_dqopt(sb)->files[cnt]);
> > +		mutex_lock_nested(&sb_dqopt(sb)->files[cnt]->i_mutex, I_MUTEX_NESTED);
> > +		truncate_inode_pages(&sb_dqopt(sb)->files[cnt]->i_data, 0);
> > +		mutex_unlock(&sb_dqopt(sb)->files[cnt]->i_mutex);
> >  	}
> >  	mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
> > -	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> > -		if (discard[cnt]) {
> > -			mutex_lock(&discard[cnt]->i_mutex);
> > -			truncate_inode_pages(&discard[cnt]->i_data, 0);
> > -			mutex_unlock(&discard[cnt]->i_mutex);
> > -			iput(discard[cnt]);
> > -		}
> > -	}
> >  }
> >  
> >  void sync_dquots(struct super_block *sb, int type)
> 
> 
> 
> Folkert van Heusden
> 
> -- 
> www.vanheusden.com/multitail - win een vlaai van multivlaai! zorg
> ervoor dat multitail opgenomen wordt in Fedora Core, AIX, Solaris of
> HP/UX en win een vlaai naar keuze
> ----------------------------------------------------------------------
> Phone: +31-6-41278122, PGP-key: 1F28D8AE, www.vanheusden.com
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

Re: [2.6.21] circular locking dependency found in QUOTA OFF

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 9431 bytes --]

On Tue 15-05-07 19:52:03, Folkert van Heusden wrote:
> I'm afraid it doesn't compile:
> 
>   CHK     include/linux/version.h
>   CHK     include/linux/utsrelease.h
>   CHK     include/linux/compile.h
>   CC      fs/dquot.o
>   CC      fs/quota.o
> fs/quota.c: In function `quota_sync_sb':
> fs/quota.c:180: error: `I_MUTEX_NESTED' undeclared (first use in this function)
> fs/quota.c:180: error: (Each undeclared identifier is reported only once
> fs/quota.c:180: error: for each function it appears in.)
> make[1]: *** [fs/quota.o] Error 1
> make: *** [fs] Error 2
  Fixed patch follows (I messed up testing as I thought that lockdep is
under config option Debug mutexes but it is not...). I'm sorry for the
stupid mistake. Andrew, please discard my previous patch and use this new
one...

									Honza
> On Tue, May 15, 2007 at 04:09:39PM +0200, Jan Kara wrote:
> >   Hi,
> > 
> >   Thanks for report. It seems like a lockdep problem. i_mutex for quota
> > files is below dqonoff_sem. We were already fixing this for filesystem
> > specific quota IO functions but obviously we've missed a few cases. I
> > wonder why it showed up only now... Anyway, attached is a fix. Andrew,
> > would you add it? Thanks.
> > 
> > 								Honza
> > 
> > On Mon 14-05-07 19:43:18, Michal Piotrowski wrote:
> > > [adding Jan and fsdevel to CC]
> > > 
> > > Hi Folkert,
> > > 
> > > On 14/05/07, Folkert van Heusden <folkert@vanheusden.com> wrote:
> > > >Hi,
> > > >
> > > >When I cleanly reboot my pc running 2.6.21 on a P4 with HT and 2GB of ram
> > > >and system on an 1-filesystem IDE disk, I get the following circular
> > > >locking dependency error:
> > > >
> > > >[330961.226405] =======================================================
> > > >[330961.226489] [ INFO: possible circular locking dependency detected ]
> > > >[330961.226531] 2.6.21 #5
> > > >[330961.226569] -------------------------------------------------------
> > > >[330961.226611] quotaoff/12249 is trying to acquire lock:
> > > >[330961.226652]  (&sb->s_type->i_mutex_key#4){--..}, at: [<c120e2a1>]
> > > >mutex_lock+0x8/0xa
> > > >[330961.226861]
> > > >[330961.226862] but task is already holding lock:
> > > >[330961.226938]  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
> > > >mutex_lock+0x8/0xa
> > > >[330961.227111]
> > > >[330961.227111] which lock already depends on the new lock.
> > > >[330961.227112]
> > > >[330961.227225]
> > > >[330961.227225] the existing dependency chain (in reverse order) is:
> > > >[330961.227303]
> > > >[330961.227303] -> #1 (&s->s_dquot.dqonoff_mutex){--..}:
> > > >[330961.227473]        [<c1039b02>] check_prev_add+0x15b/0x281
> > > >[330961.227766]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
> > > >[330961.228056]        [<c103b683>] __lock_acquire+0x692/0xb81
> > > >[330961.228353]        [<c103bfda>] lock_acquire+0x62/0x81
> > > >[330961.228643]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> > > >[330961.228934]        [<c120e2a1>] mutex_lock+0x8/0xa
> > > >[330961.229221]        [<c109fbbe>] vfs_quota_on_inode+0xc1/0x25f
> > > >[330961.229513]        [<c109fdd1>] vfs_quota_on+0x75/0x79
> > > >[330961.229803]        [<c10bc92d>] ext3_quota_on+0x95/0xb0
> > > >[330961.230093]        [<c10a1eb2>] do_quotactl+0xc9/0x2dd
> > > >[330961.230384]        [<c10a214a>] sys_quotactl+0x84/0xd6
> > > >[330961.230673]        [<c1003f74>] syscall_call+0x7/0xb
> > > >[330961.230963]        [<ffffffff>] 0xffffffff
> > > >[330961.231268]
> > > >[330961.231268] -> #0 (&sb->s_type->i_mutex_key#4){--..}:
> > > >[330961.231469]        [<c10399db>] check_prev_add+0x34/0x281
> > > >[330961.231759]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
> > > >[330961.232049]        [<c103b683>] __lock_acquire+0x692/0xb81
> > > >[330961.232344]        [<c103bfda>] lock_acquire+0x62/0x81
> > > >[330961.232632]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> > > >[330961.232923]        [<c120e2a1>] mutex_lock+0x8/0xa
> > > >[330961.233211]        [<c109fa6c>] vfs_quota_off+0x1cf/0x260
> > > >[330961.233500]        [<c10a2088>] do_quotactl+0x29f/0x2dd
> > > >[330961.233792]        [<c10a214a>] sys_quotactl+0x84/0xd6
> > > >[330961.234081]        [<c1003f74>] syscall_call+0x7/0xb
> > > >[330961.234503]        [<ffffffff>] 0xffffffff
> > > >[330961.234795]
> > > >[330961.234795] other info that might help us debug this:
> > > >[330961.234796]
> > > >[330961.234908] 2 locks held by quotaoff/12249:
> > > >[330961.234947]  #0:  (&type->s_umount_key#15){----}, at: [<c1070b5d>]
> > > >get_super+0x53/0x94
> > > >[330961.235183]  #1:  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
> > > >mutex_lock+0x8/0xa
> > > >[330961.235386]
> > > >[330961.235387] stack backtrace:
> > > >[330961.235462]  [<c1004d53>] show_trace_log_lvl+0x1a/0x30
> > > >[330961.235535]  [<c1004d7b>] show_trace+0x12/0x14
> > > >[330961.235606]  [<c1004e75>] dump_stack+0x16/0x18
> > > >[330961.235679]  [<c1039352>] print_circular_bug_tail+0x6f/0x71
> > > >[330961.235753]  [<c10399db>] check_prev_add+0x34/0x281
> > > >[330961.235825]  [<c1039cb3>] check_prevs_add+0x8b/0xe8
> > > >[330961.235897]  [<c103b683>] __lock_acquire+0x692/0xb81
> > > >[330961.235969]  [<c103bfda>] lock_acquire+0x62/0x81
> > > >[330961.236041]  [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
> > > >[330961.236113]  [<c120e2a1>] mutex_lock+0x8/0xa
> > > >[330961.236185]  [<c109fa6c>] vfs_quota_off+0x1cf/0x260
> > > >[330961.236257]  [<c10a2088>] do_quotactl+0x29f/0x2dd
> > > >[330961.236330]  [<c10a214a>] sys_quotactl+0x84/0xd6
> > > >[330961.236402]  [<c1003f74>] syscall_call+0x7/0xb
> > > >[330961.236473]  =======================
> > > >
> > > 
> > > Is this a 2.6.21 regression?
> > > 
> > > Regards,
> > > Michal
> > > 
> > > -- 
> > > Michal K. K. Piotrowski
> > > Kernel Monkeys
> > > (http://kernel.wikidot.com/start)
> > -- 
> > Jan Kara <jack@suse.cz>
> > SuSE CR Labs
> 
> > i_mutex on quota files is special. Unlike i_mutexes for other inodes it is
> > acquired under dqonoff_mutex. Tell lockdep about this lock ranking. Also
> > comment and code in quota_sync_sb() seem to be bogus (as i_mutex for quota
> > file can be acquired under dqonoff_mutex). Move truncate_inode_pages()
> > call under dqonoff_mutex and save some problems with races...
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > 
> > diff -rupX /home/jack/.kerndiffexclude linux-2.6.21/fs/dquot.c linux-2.6.21-1-quota_lockdep/fs/dquot.c
> > --- linux-2.6.21/fs/dquot.c	2007-05-15 14:18:47.000000000 +0200
> > +++ linux-2.6.21-1-quota_lockdep/fs/dquot.c	2007-05-15 14:22:47.000000000 +0200
> > @@ -1421,7 +1421,7 @@ int vfs_quota_off(struct super_block *sb
> >  			/* If quota was reenabled in the meantime, we have
> >  			 * nothing to do */
> >  			if (!sb_has_quota_enabled(sb, cnt)) {
> > -				mutex_lock(&toputinode[cnt]->i_mutex);
> > +				mutex_lock_nested(&toputinode[cnt]->i_mutex, I_MUTEX_QUOTA);
> >  				toputinode[cnt]->i_flags &= ~(S_IMMUTABLE |
> >  				  S_NOATIME | S_NOQUOTA);
> >  				truncate_inode_pages(&toputinode[cnt]->i_data, 0);
> > diff -rupX /home/jack/.kerndiffexclude linux-2.6.21/fs/quota.c linux-2.6.21-1-quota_lockdep/fs/quota.c
> > --- linux-2.6.21/fs/quota.c	2006-11-29 22:57:37.000000000 +0100
> > +++ linux-2.6.21-1-quota_lockdep/fs/quota.c	2007-05-15 15:15:44.000000000 +0200
> > @@ -158,7 +158,6 @@ static int check_quotactl_valid(struct s
> >  static void quota_sync_sb(struct super_block *sb, int type)
> >  {
> >  	int cnt;
> > -	struct inode *discard[MAXQUOTAS];
> >  
> >  	sb->s_qcop->quota_sync(sb, type);
> >  	/* This is not very clever (and fast) but currently I don't know about
> > @@ -168,29 +167,21 @@ static void quota_sync_sb(struct super_b
> >  		sb->s_op->sync_fs(sb, 1);
> >  	sync_blockdev(sb->s_bdev);
> >  
> > -	/* Now when everything is written we can discard the pagecache so
> > -	 * that userspace sees the changes. We need i_mutex and so we could
> > -	 * not do it inside dqonoff_mutex. Moreover we need to be carefull
> > -	 * about races with quotaoff() (that is the reason why we have own
> > -	 * reference to inode). */
> > +	/*
> > +	 * Now when everything is written we can discard the pagecache so
> > +	 * that userspace sees the changes.
> > +	 */
> >  	mutex_lock(&sb_dqopt(sb)->dqonoff_mutex);
> >  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> > -		discard[cnt] = NULL;
> >  		if (type != -1 && cnt != type)
> >  			continue;
> >  		if (!sb_has_quota_enabled(sb, cnt))
> >  			continue;
> > -		discard[cnt] = igrab(sb_dqopt(sb)->files[cnt]);
> > +		mutex_lock_nested(&sb_dqopt(sb)->files[cnt]->i_mutex, I_MUTEX_NESTED);
> > +		truncate_inode_pages(&sb_dqopt(sb)->files[cnt]->i_data, 0);
> > +		mutex_unlock(&sb_dqopt(sb)->files[cnt]->i_mutex);
> >  	}
> >  	mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
> > -	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> > -		if (discard[cnt]) {
> > -			mutex_lock(&discard[cnt]->i_mutex);
> > -			truncate_inode_pages(&discard[cnt]->i_data, 0);
> > -			mutex_unlock(&discard[cnt]->i_mutex);
> > -			iput(discard[cnt]);
> > -		}
> > -	}
> >  }
> >  
> >  void sync_dquots(struct super_block *sb, int type)
> 
> 
> 
> Folkert van Heusden
> 
> -- 
> www.vanheusden.com/multitail - win een vlaai van multivlaai! zorg
> ervoor dat multitail opgenomen wordt in Fedora Core, AIX, Solaris of
> HP/UX en win een vlaai naar keuze
> ----------------------------------------------------------------------
> Phone: +31-6-41278122, PGP-key: 1F28D8AE, www.vanheusden.com

[-- Attachment #2: quota-2.6.21-1-quota_lockdep.diff --]
[-- Type: text/x-patch, Size: 2939 bytes --]

i_mutex on quota files is special. Unlike i_mutexes for other inodes it is
acquired under dqonoff_mutex. Tell lockdep about this lock ranking. Also
comment and code in quota_sync_sb() seem to be bogus (as i_mutex for quota
file can be acquired under dqonoff_mutex). Move truncate_inode_pages()
call under dqonoff_mutex and save some problems with races...

Signed-off-by: Jan Kara <jack@suse.cz>

diff -rupX /home/jack/.kerndiffexclude linux-2.6.21/fs/dquot.c linux-2.6.21-1-quota_lockdep/fs/dquot.c
--- linux-2.6.21/fs/dquot.c	2007-05-15 14:18:47.000000000 +0200
+++ linux-2.6.21-1-quota_lockdep/fs/dquot.c	2007-05-15 14:22:47.000000000 +0200
@@ -1421,7 +1421,7 @@ int vfs_quota_off(struct super_block *sb
 			/* If quota was reenabled in the meantime, we have
 			 * nothing to do */
 			if (!sb_has_quota_enabled(sb, cnt)) {
-				mutex_lock(&toputinode[cnt]->i_mutex);
+				mutex_lock_nested(&toputinode[cnt]->i_mutex, I_MUTEX_QUOTA);
 				toputinode[cnt]->i_flags &= ~(S_IMMUTABLE |
 				  S_NOATIME | S_NOQUOTA);
 				truncate_inode_pages(&toputinode[cnt]->i_data, 0);
diff -rupX /home/jack/.kerndiffexclude linux-2.6.21/fs/quota.c linux-2.6.21-1-quota_lockdep/fs/quota.c
--- linux-2.6.21/fs/quota.c	2006-11-29 22:57:37.000000000 +0100
+++ linux-2.6.21-1-quota_lockdep/fs/quota.c	2007-05-15 20:36:18.000000000 +0200
@@ -158,7 +158,6 @@ static int check_quotactl_valid(struct s
 static void quota_sync_sb(struct super_block *sb, int type)
 {
 	int cnt;
-	struct inode *discard[MAXQUOTAS];
 
 	sb->s_qcop->quota_sync(sb, type);
 	/* This is not very clever (and fast) but currently I don't know about
@@ -168,29 +167,21 @@ static void quota_sync_sb(struct super_b
 		sb->s_op->sync_fs(sb, 1);
 	sync_blockdev(sb->s_bdev);
 
-	/* Now when everything is written we can discard the pagecache so
-	 * that userspace sees the changes. We need i_mutex and so we could
-	 * not do it inside dqonoff_mutex. Moreover we need to be carefull
-	 * about races with quotaoff() (that is the reason why we have own
-	 * reference to inode). */
+	/*
+	 * Now when everything is written we can discard the pagecache so
+	 * that userspace sees the changes.
+	 */
 	mutex_lock(&sb_dqopt(sb)->dqonoff_mutex);
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
-		discard[cnt] = NULL;
 		if (type != -1 && cnt != type)
 			continue;
 		if (!sb_has_quota_enabled(sb, cnt))
 			continue;
-		discard[cnt] = igrab(sb_dqopt(sb)->files[cnt]);
+		mutex_lock_nested(&sb_dqopt(sb)->files[cnt]->i_mutex, I_MUTEX_QUOTA);
+		truncate_inode_pages(&sb_dqopt(sb)->files[cnt]->i_data, 0);
+		mutex_unlock(&sb_dqopt(sb)->files[cnt]->i_mutex);
 	}
 	mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
-	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
-		if (discard[cnt]) {
-			mutex_lock(&discard[cnt]->i_mutex);
-			truncate_inode_pages(&discard[cnt]->i_data, 0);
-			mutex_unlock(&discard[cnt]->i_mutex);
-			iput(discard[cnt]);
-		}
-	}
 }
 
 void sync_dquots(struct super_block *sb, int type)

[2.6.21] circular locking dependency found in QUOTA OFF

[Folkert van Heusden]

Hi,

When I cleanly reboot my pc running 2.6.21 on a P4 with HT and 2GB of ram
and system on an 1-filesystem IDE disk, I get the following circular
locking dependency error:

[330961.226405] =======================================================
[330961.226489] [ INFO: possible circular locking dependency detected ]
[330961.226531] 2.6.21 #5
[330961.226569] -------------------------------------------------------
[330961.226611] quotaoff/12249 is trying to acquire lock:
[330961.226652]  (&sb->s_type->i_mutex_key#4){--..}, at: [<c120e2a1>]
mutex_lock+0x8/0xa
[330961.226861]
[330961.226862] but task is already holding lock:
[330961.226938]  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
mutex_lock+0x8/0xa
[330961.227111]
[330961.227111] which lock already depends on the new lock.
[330961.227112]
[330961.227225]
[330961.227225] the existing dependency chain (in reverse order) is:
[330961.227303]
[330961.227303] -> #1 (&s->s_dquot.dqonoff_mutex){--..}:
[330961.227473]        [<c1039b02>] check_prev_add+0x15b/0x281
[330961.227766]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
[330961.228056]        [<c103b683>] __lock_acquire+0x692/0xb81
[330961.228353]        [<c103bfda>] lock_acquire+0x62/0x81
[330961.228643]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
[330961.228934]        [<c120e2a1>] mutex_lock+0x8/0xa
[330961.229221]        [<c109fbbe>] vfs_quota_on_inode+0xc1/0x25f
[330961.229513]        [<c109fdd1>] vfs_quota_on+0x75/0x79
[330961.229803]        [<c10bc92d>] ext3_quota_on+0x95/0xb0
[330961.230093]        [<c10a1eb2>] do_quotactl+0xc9/0x2dd
[330961.230384]        [<c10a214a>] sys_quotactl+0x84/0xd6
[330961.230673]        [<c1003f74>] syscall_call+0x7/0xb
[330961.230963]        [<ffffffff>] 0xffffffff
[330961.231268]
[330961.231268] -> #0 (&sb->s_type->i_mutex_key#4){--..}:
[330961.231469]        [<c10399db>] check_prev_add+0x34/0x281
[330961.231759]        [<c1039cb3>] check_prevs_add+0x8b/0xe8
[330961.232049]        [<c103b683>] __lock_acquire+0x692/0xb81
[330961.232344]        [<c103bfda>] lock_acquire+0x62/0x81
[330961.232632]        [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
[330961.232923]        [<c120e2a1>] mutex_lock+0x8/0xa
[330961.233211]        [<c109fa6c>] vfs_quota_off+0x1cf/0x260
[330961.233500]        [<c10a2088>] do_quotactl+0x29f/0x2dd
[330961.233792]        [<c10a214a>] sys_quotactl+0x84/0xd6
[330961.234081]        [<c1003f74>] syscall_call+0x7/0xb
[330961.234503]        [<ffffffff>] 0xffffffff
[330961.234795]
[330961.234795] other info that might help us debug this:
[330961.234796]
[330961.234908] 2 locks held by quotaoff/12249:
[330961.234947]  #0:  (&type->s_umount_key#15){----}, at: [<c1070b5d>]
get_super+0x53/0x94
[330961.235183]  #1:  (&s->s_dquot.dqonoff_mutex){--..}, at: [<c120e2a1>]
mutex_lock+0x8/0xa
[330961.235386]
[330961.235387] stack backtrace:
[330961.235462]  [<c1004d53>] show_trace_log_lvl+0x1a/0x30
[330961.235535]  [<c1004d7b>] show_trace+0x12/0x14
[330961.235606]  [<c1004e75>] dump_stack+0x16/0x18
[330961.235679]  [<c1039352>] print_circular_bug_tail+0x6f/0x71
[330961.235753]  [<c10399db>] check_prev_add+0x34/0x281
[330961.235825]  [<c1039cb3>] check_prevs_add+0x8b/0xe8
[330961.235897]  [<c103b683>] __lock_acquire+0x692/0xb81
[330961.235969]  [<c103bfda>] lock_acquire+0x62/0x81
[330961.236041]  [<c120e322>] __mutex_lock_slowpath+0x75/0x28c
[330961.236113]  [<c120e2a1>] mutex_lock+0x8/0xa
[330961.236185]  [<c109fa6c>] vfs_quota_off+0x1cf/0x260
[330961.236257]  [<c10a2088>] do_quotactl+0x29f/0x2dd
[330961.236330]  [<c10a214a>] sys_quotactl+0x84/0xd6
[330961.236402]  [<c1003f74>] syscall_call+0x7/0xb
[330961.236473]  =======================