Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON
On Thu, Apr 5, 2012 at 11:51 AM, Ilya Dryomov idryo...@gmail.com wrote: On Thu, Apr 05, 2012 at 12:23:01PM -0400, Bobby Powers wrote: On Wed, Apr 4, 2012 at 10:04 PM, Bobby Powers bobbypow...@gmail.com wrote: spin_is_locked always returns 0 on non-SMP systems, which causes btrfs to fail the mount. There is documentation pending as to why checking I guess I should be explicit in stating that this is a regression, so this patch or something else that addresses it should be pulled into 3.4. Yes, this is a regression and spin_is_locked() definitely has to go. I don't have a strong opinion on this assert, if there are objections to v2 I'm OK with ripping that BUG_ON entirely and replacing it with a comment (this function and its callers are WIP). I'm still hitting this BUG_ON after applying this patch on my single CPU AthlonXP x86 system. I'm running a 3.3.1 kernel merged with the for-linus branch, and had this patch applied. I am currently testing with the BUG_ON commented out. The btrfs partitions mount without error, and I haven't encountered any immediate issues. -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON
On Wed, Apr 04, 2012 at 10:04:12PM -0400, Bobby Powers wrote:
spin_is_locked always returns 0 on non-SMP systems, which causes btrfs
to fail the mount. There is documentation pending as to why checking
for spin_is_locked is a bad idea:
https://lkml.org/lkml/2012/3/27/413
The suggested lockdep_assert_held() is not appropriate in this case,
as what get_restripe_target() is checking for is that either
volume_mutex is held or balance_lock is held. Luckily
lockdep_assert_held() is a simple macro:
WARN_ON(debug_locks !lockdep_is_held(l))
We can mimic the structure in get_restripe_target(), but we need to
make sure lockdep_is_held() is defined for the !LOCKDEP case.
CC: Ilya Dryomov idryo...@gmail.com
CC: Chris Mason chris.ma...@oracle.com
CC: Andi Kleen a...@linux.intel.com
CC: Jeff Mahoney je...@suse.de
CC: Ingo Molnar mi...@redhat.com
CC: linux-ker...@vger.kernel.org
Signed-off-by: Bobby Powers bobbypow...@gmail.com
---
fs/btrfs/extent-tree.c |5 +++--
include/linux/lockdep.h |1 +
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a844204..4d13eb1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -24,6 +24,7 @@
#include linux/kthread.h
#include linux/slab.h
#include linux/ratelimit.h
+#include linux/lockdep.h
#include compat.h
#include hash.h
#include ctree.h
@@ -3158,8 +3159,8 @@ static u64 get_restripe_target(struct btrfs_fs_info
*fs_info, u64 flags)
struct btrfs_balance_control *bctl = fs_info-balance_ctl;
u64 target = 0;
- BUG_ON(!mutex_is_locked(fs_info-volume_mutex)
-!spin_is_locked(fs_info-balance_lock));
+ BUG_ON(debug_locks !lockdep_is_held(fs_info-volume_mutex)
+!lockdep_is_held(fs_info-balance_lock));
if (!bctl)
return 0;
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index d36619e..94c0edb 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -392,6 +392,7 @@ struct lock_class_key { };
#define lockdep_depth(tsk) (0)
+#define lockdep_is_held(l) (0)
#define lockdep_assert_held(l) do { } while (0)
#define lockdep_recursing(tsk) (0)
--
1.7.10.rc3.3.g19a6c
OK, Mitch's report prompted me to actually take a look. This patch is
wrong: by defining lockdep_is_held(l) to 0 in !LOCKDEP case you
effectively mimic the behaviour of spin_is_locked() which is what
getting us in the first place.
get_restripe_target() interface is WIP so I will replace BUG_ON with a
comment and send a patch through btrfs tree.
Thanks,
Ilya
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON
On Fri, Apr 6, 2012 at 4:05 PM, Ilya Dryomov idryo...@gmail.com wrote:
On Wed, Apr 04, 2012 at 10:04:12PM -0400, Bobby Powers wrote:
spin_is_locked always returns 0 on non-SMP systems, which causes btrfs
to fail the mount. There is documentation pending as to why checking
for spin_is_locked is a bad idea:
https://lkml.org/lkml/2012/3/27/413
The suggested lockdep_assert_held() is not appropriate in this case,
as what get_restripe_target() is checking for is that either
volume_mutex is held or balance_lock is held. Luckily
lockdep_assert_held() is a simple macro:
WARN_ON(debug_locks !lockdep_is_held(l))
We can mimic the structure in get_restripe_target(), but we need to
make sure lockdep_is_held() is defined for the !LOCKDEP case.
CC: Ilya Dryomov idryo...@gmail.com
CC: Chris Mason chris.ma...@oracle.com
CC: Andi Kleen a...@linux.intel.com
CC: Jeff Mahoney je...@suse.de
CC: Ingo Molnar mi...@redhat.com
CC: linux-ker...@vger.kernel.org
Signed-off-by: Bobby Powers bobbypow...@gmail.com
---
fs/btrfs/extent-tree.c | 5 +++--
include/linux/lockdep.h | 1 +
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a844204..4d13eb1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -24,6 +24,7 @@
#include linux/kthread.h
#include linux/slab.h
#include linux/ratelimit.h
+#include linux/lockdep.h
#include compat.h
#include hash.h
#include ctree.h
@@ -3158,8 +3159,8 @@ static u64 get_restripe_target(struct btrfs_fs_info
*fs_info, u64 flags)
struct btrfs_balance_control *bctl = fs_info-balance_ctl;
u64 target = 0;
- BUG_ON(!mutex_is_locked(fs_info-volume_mutex)
- !spin_is_locked(fs_info-balance_lock));
+ BUG_ON(debug_locks !lockdep_is_held(fs_info-volume_mutex)
+ !lockdep_is_held(fs_info-balance_lock));
if (!bctl)
return 0;
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index d36619e..94c0edb 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -392,6 +392,7 @@ struct lock_class_key { };
#define lockdep_depth(tsk) (0)
+#define lockdep_is_held(l) (0)
#define lockdep_assert_held(l) do { } while (0)
#define lockdep_recursing(tsk) (0)
--
1.7.10.rc3.3.g19a6c
OK, Mitch's report prompted me to actually take a look. This patch is
wrong: by defining lockdep_is_held(l) to 0 in !LOCKDEP case you
effectively mimic the behaviour of spin_is_locked() which is what
getting us in the first place.
get_restripe_target() interface is WIP so I will replace BUG_ON with a
comment and send a patch through btrfs tree.
Hah, good point...
Thanks,
Ilya
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON
On Wed, Apr 4, 2012 at 10:04 PM, Bobby Powers bobbypow...@gmail.com wrote:
spin_is_locked always returns 0 on non-SMP systems, which causes btrfs
to fail the mount. There is documentation pending as to why checking
I guess I should be explicit in stating that this is a regression, so
this patch or something else that addresses it should be pulled into
3.4.
for spin_is_locked is a bad idea:
https://lkml.org/lkml/2012/3/27/413
The suggested lockdep_assert_held() is not appropriate in this case,
as what get_restripe_target() is checking for is that either
volume_mutex is held or balance_lock is held. Luckily
lockdep_assert_held() is a simple macro:
WARN_ON(debug_locks !lockdep_is_held(l))
We can mimic the structure in get_restripe_target(), but we need to
make sure lockdep_is_held() is defined for the !LOCKDEP case.
CC: Ilya Dryomov idryo...@gmail.com
CC: Chris Mason chris.ma...@oracle.com
CC: Andi Kleen a...@linux.intel.com
CC: Jeff Mahoney je...@suse.de
CC: Ingo Molnar mi...@redhat.com
CC: linux-ker...@vger.kernel.org
Signed-off-by: Bobby Powers bobbypow...@gmail.com
---
fs/btrfs/extent-tree.c | 5 +++--
include/linux/lockdep.h | 1 +
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a844204..4d13eb1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -24,6 +24,7 @@
#include linux/kthread.h
#include linux/slab.h
#include linux/ratelimit.h
+#include linux/lockdep.h
#include compat.h
#include hash.h
#include ctree.h
@@ -3158,8 +3159,8 @@ static u64 get_restripe_target(struct btrfs_fs_info
*fs_info, u64 flags)
struct btrfs_balance_control *bctl = fs_info-balance_ctl;
u64 target = 0;
- BUG_ON(!mutex_is_locked(fs_info-volume_mutex)
- !spin_is_locked(fs_info-balance_lock));
+ BUG_ON(debug_locks !lockdep_is_held(fs_info-volume_mutex)
+ !lockdep_is_held(fs_info-balance_lock));
if (!bctl)
return 0;
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index d36619e..94c0edb 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -392,6 +392,7 @@ struct lock_class_key { };
#define lockdep_depth(tsk) (0)
+#define lockdep_is_held(l) (0)
#define lockdep_assert_held(l) do { } while (0)
#define lockdep_recursing(tsk) (0)
--
1.7.10.rc3.3.g19a6c
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON
On Thu, Apr 05, 2012 at 12:23:01PM -0400, Bobby Powers wrote: On Wed, Apr 4, 2012 at 10:04 PM, Bobby Powers bobbypow...@gmail.com wrote: spin_is_locked always returns 0 on non-SMP systems, which causes btrfs to fail the mount. There is documentation pending as to why checking I guess I should be explicit in stating that this is a regression, so this patch or something else that addresses it should be pulled into 3.4. Yes, this is a regression and spin_is_locked() definitely has to go. I don't have a strong opinion on this assert, if there are objections to v2 I'm OK with ripping that BUG_ON entirely and replacing it with a comment (this function and its callers are WIP). Thanks, Ilya -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON
spin_is_locked always returns 0 on non-SMP systems, which causes btrfs
to fail the mount. There is documentation pending as to why checking
for spin_is_locked is a bad idea:
https://lkml.org/lkml/2012/3/27/413
The suggested lockdep_assert_held() is not appropriate in this case,
as what get_restripe_target() is checking for is that either
volume_mutex is held or balance_lock is held. Luckily
lockdep_assert_held() is a simple macro:
WARN_ON(debug_locks !lockdep_is_held(l))
We can mimic the structure in get_restripe_target(), but we need to
make sure lockdep_is_held() is defined for the !LOCKDEP case.
CC: Ilya Dryomov idryo...@gmail.com
CC: Chris Mason chris.ma...@oracle.com
CC: Andi Kleen a...@linux.intel.com
CC: Jeff Mahoney je...@suse.de
CC: Ingo Molnar mi...@redhat.com
CC: linux-ker...@vger.kernel.org
Signed-off-by: Bobby Powers bobbypow...@gmail.com
---
fs/btrfs/extent-tree.c |5 +++--
include/linux/lockdep.h |1 +
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a844204..4d13eb1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -24,6 +24,7 @@
#include linux/kthread.h
#include linux/slab.h
#include linux/ratelimit.h
+#include linux/lockdep.h
#include compat.h
#include hash.h
#include ctree.h
@@ -3158,8 +3159,8 @@ static u64 get_restripe_target(struct btrfs_fs_info
*fs_info, u64 flags)
struct btrfs_balance_control *bctl = fs_info-balance_ctl;
u64 target = 0;
- BUG_ON(!mutex_is_locked(fs_info-volume_mutex)
- !spin_is_locked(fs_info-balance_lock));
+ BUG_ON(debug_locks !lockdep_is_held(fs_info-volume_mutex)
+ !lockdep_is_held(fs_info-balance_lock));
if (!bctl)
return 0;
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index d36619e..94c0edb 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -392,6 +392,7 @@ struct lock_class_key { };
#define lockdep_depth(tsk) (0)
+#define lockdep_is_held(l) (0)
#define lockdep_assert_held(l) do { } while (0)
#define lockdep_recursing(tsk) (0)
--
1.7.10.rc3.3.g19a6c
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
