[PATCH] fs/super.c: Fix false lock inverse dependency

[Madhu Rajakumar Nallathamby <madhuraj@xxxxxxxxxxxx>]

Hello all,

  I came across the following lockdep warning while running kernel version 3.10.10. It is very hard to reproduce, and was seen only once while testing.

=========================================================
[ INFO: possible irq lock inversion dependency detected ]
3.10.10+ #1 Not tainted
---------------------------------------------------------
kswapd0/627 just changed the state of lock:
  (sb_writers#3){.+.+.?}, at: [<c01327a0>] do_fallocate+0xf4/0x174
but this lock took another, RECLAIM_FS-unsafe lock in the past:
  (&sb->s_type->i_mutex_key#8/1){+.+.+.}

And other interrupts could create inverse lock ordering between them.


 other info that might help us debug this:
  Possible interrupt unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
  lock(&sb->s_type->i_mutex_key#8/1);
                                local_irq_disable();
                                lock(sb_writers#3);
                                lock(&sb->s_type->i_mutex_key#8/1);
  <Interrupt>
    lock(sb_writers#3);

  *** DEADLOCK ***

>From the logs, it can be seen that sb_writers lock is taken by two different functions(mnt_want_write and do_fallocate), on the same file system. 
sb_writers lock is treated as a rw semaphore(taken using rwsem_acquire_read). It can be taken recursively, when multiple threads are modifying data or metadata of the same filesystem. Since this lock is taken with interrupts enabled, the above lock inverse order scenario could happen. But, it does not seem like this will cause a deadlock, as sb_writers is always taken only as a reader. 
So, I have overcome this warning, by disabling lockdep around sb_writers lock. Is my observation correct? Please let me know your suggestions to fix this warning.

I have attached a diff of my patch below. It applies on the 3.10.10 kernel.

diff --git a/fs/super.c b/fs/super.c
index 68307c0..f983d96 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1151,7 +1151,12 @@ void __sb_end_write(struct super_block *sb, int level)
        smp_mb();
        if (waitqueue_active(&sb->s_writers.wait))
                wake_up(&sb->s_writers.wait);
+
+       /*s_writers was taken with lockdep checks disabled, so turn off
+       lockdep checks here too*/
+       lockdep_off();
        rwsem_release(&sb->s_writers.lock_map[level-1], 1, _RET_IP_);
+       lockdep_on();
}
EXPORT_SYMBOL(__sb_end_write);

@@ -1177,7 +1182,18 @@ static void acquire_freeze_lock(struct super_block *sb, int level, bool trylock,
                                break;
                        }
        }
+
+       /* s_writers lock sometimes triggers the lockdep warning 'possible irq
+       lock inversion dependency detected'. s_writers is treated as a rw
+       semaphore, always taken only as a reader. It can be taken recursively,
+       when multiple threads are modifying data or metadata of the same
+       filesystem. Since this lock is taken with irqs enabled, it is not
+       always possible to guarantee an ordering between s_writers and other
+       locks. Since this will not actually cause a deadlock, turn off lockdep
+       checks for this case */
+       lockdep_off();
        rwsem_acquire_read(&sb->s_writers.lock_map[level-1], 0, trylock, ip);
+       lockdep_on();
}
#endif

Thanks,
Madhu Rajakumar

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html