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 As this was the only location in fs/btrfs/extent-tree.c that did lock-correctness checking in a BUG_ON, simply remove it. Signed-off-by: Bobby Powers <bobbypowers@gmail.com> CC: Ilya Dryomov <idryomov@gmail.com> CC: Chris Mason <chris.mason@oracle.com> CC: Andi Kleen <ak@linux.intel.com> CC: linux-btrfs@vger.kernel.org CC: linux-kernel@vger.kernel.org --- fs/btrfs/extent-tree.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index a844204..c98b073 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -3158,9 +3158,6 @@ 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)); - if (!bctl) return 0; -- 1.7.10.rc3.3.g19a6c
Jeff Mahoney
2012-Apr-05 00:46 UTC
Re: [PATCH] Btrfs: remove BUG_ON from get_restripe_target
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 04/04/2012 04:19 PM, 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 > > As this was the only location in fs/btrfs/extent-tree.c that did > lock-correctness checking in a BUG_ON, simply remove it. > > Signed-off-by: Bobby Powers <bobbypowers@gmail.com> CC: Ilya > Dryomov <idryomov@gmail.com> CC: Chris Mason > <chris.mason@oracle.com> CC: Andi Kleen <ak@linux.intel.com> CC: > linux-btrfs@vger.kernel.org CC: linux-kernel@vger.kernel.org --- > fs/btrfs/extent-tree.c | 3 --- 1 file changed, 3 deletions(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index > a844204..c98b073 100644 --- a/fs/btrfs/extent-tree.c +++ > b/fs/btrfs/extent-tree.c @@ -3158,9 +3158,6 @@ 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)); - if (!bctl) return 0; >Why not replace both of these with lockdep_assert_held as Andi suggested in his doc? - -Jeff - -- Jeff Mahoney SUSE Labs -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.18 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQIcBAEBAgAGBQJPfOtVAAoJEB57S2MheeWyew4P/ju2FiKPWvFEhShP2BQzmPPL wYoYP9cEmuoJbr1FXnpVXRy3RJsS+Dv9IHyUt2l8WRddO5PeTC08fS3a1PsR6Lky dnDBpOTsvBhdUPClA+9cu9HtNZ488PKALTgNX7kocVYm+vd0vkE54Iv0OWRvIMEA 5nmm/r2MJwgQmTsFwAWojxfiSEJNzRSJ6GXZFbIfwGOaJIx7MmnPg4R3PKU1SZiF ogKIwocfsTA/T7eplK58+EqtQnfTGTKzAbaEQvX/w1ryRRXHqD2zdk2p+zSAtpN6 swnhu156Bb9t2EUPTv9KiYth0BoYhYy9ppdp2Wyh0hX3lYCAP1SrwBVIYCqpvdqr CuipFWmqbupW41IjQc6bjoIwaVlGNsqwDY3NNrjR+kb29k3+/3MT8QH1ZXLhlpTB cUixluE62J8QllW4u3Wa2mLMTqdolcWCCTdh4yUqE+8jxguXUKhoTni1vwApmkru PM158CLPdxWCnDe1TaGcRYcBoweWPl6UDaVj8W+LdSVcYsycZhwehvDg2amX6pdg 9QFUf25PbDzEVw99w3f2hMhRG5pERRheLqPcFUVbnqZYkZBACt9XtIYBINlREoYW ACjr9dJszluF9dEKWOmlKhsah3gAGJJoC5+QU8oR+vpxKdrI+8vQ+NYtVhCr0hVA CO/KEEcwNaobsCWiAbSr =5Op/ -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bobby Powers
2012-Apr-05 01:19 UTC
Re: [PATCH] Btrfs: remove BUG_ON from get_restripe_target
On Wed, Apr 4, 2012 at 8:46 PM, Jeff Mahoney <jeffm@suse.de> wrote:> -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 04/04/2012 04:19 PM, 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 >> >> As this was the only location in fs/btrfs/extent-tree.c that did >> lock-correctness checking in a BUG_ON, simply remove it. >> >> Signed-off-by: Bobby Powers <bobbypowers@gmail.com> CC: Ilya >> Dryomov <idryomov@gmail.com> CC: Chris Mason >> <chris.mason@oracle.com> CC: Andi Kleen <ak@linux.intel.com> CC: >> linux-btrfs@vger.kernel.org CC: linux-kernel@vger.kernel.org --- >> fs/btrfs/extent-tree.c | 3 --- 1 file changed, 3 deletions(-) >> >> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index >> a844204..c98b073 100644 --- a/fs/btrfs/extent-tree.c +++ >> b/fs/btrfs/extent-tree.c @@ -3158,9 +3158,6 @@ 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)); - if (!bctl) return 0; >> > > Why not replace both of these with lockdep_assert_held as Andi > suggested in his doc?The complication here is that the existing statement was asserting that _either_ volume_mutex was held _or_ balance_lock was held - lockdep_assert_held is defined as: #define lockdep_assert_held(l) WARN_ON(debug_locks && !lockdep_is_held(l)) which doesn''t map to the existing logic. Although I could be missing something. Sorry for the double email, forgot to turn off html mail initially. yours, Bobby> - -Jeff > > - -- > Jeff Mahoney > SUSE Labs > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v2.0.18 (GNU/Linux) > Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ > > iQIcBAEBAgAGBQJPfOtVAAoJEB57S2MheeWyew4P/ju2FiKPWvFEhShP2BQzmPPL > wYoYP9cEmuoJbr1FXnpVXRy3RJsS+Dv9IHyUt2l8WRddO5PeTC08fS3a1PsR6Lky > dnDBpOTsvBhdUPClA+9cu9HtNZ488PKALTgNX7kocVYm+vd0vkE54Iv0OWRvIMEA > 5nmm/r2MJwgQmTsFwAWojxfiSEJNzRSJ6GXZFbIfwGOaJIx7MmnPg4R3PKU1SZiF > ogKIwocfsTA/T7eplK58+EqtQnfTGTKzAbaEQvX/w1ryRRXHqD2zdk2p+zSAtpN6 > swnhu156Bb9t2EUPTv9KiYth0BoYhYy9ppdp2Wyh0hX3lYCAP1SrwBVIYCqpvdqr > CuipFWmqbupW41IjQc6bjoIwaVlGNsqwDY3NNrjR+kb29k3+/3MT8QH1ZXLhlpTB > cUixluE62J8QllW4u3Wa2mLMTqdolcWCCTdh4yUqE+8jxguXUKhoTni1vwApmkru > PM158CLPdxWCnDe1TaGcRYcBoweWPl6UDaVj8W+LdSVcYsycZhwehvDg2amX6pdg > 9QFUf25PbDzEVw99w3f2hMhRG5pERRheLqPcFUVbnqZYkZBACt9XtIYBINlREoYW > ACjr9dJszluF9dEKWOmlKhsah3gAGJJoC5+QU8oR+vpxKdrI+8vQ+NYtVhCr0hVA > CO/KEEcwNaobsCWiAbSr > =5Op/ > -----END PGP SIGNATURE------- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bobby Powers
2012-Apr-05 01:48 UTC
Re: [PATCH] Btrfs: remove BUG_ON from get_restripe_target
On Wed, Apr 4, 2012 at 9:19 PM, Bobby Powers <bobbypowers@gmail.com> wrote:> On Wed, Apr 4, 2012 at 8:46 PM, Jeff Mahoney <jeffm@suse.de> wrote: >> -----BEGIN PGP SIGNED MESSAGE----- >> Hash: SHA1 >> >> On 04/04/2012 04:19 PM, 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 >>> >>> As this was the only location in fs/btrfs/extent-tree.c that did >>> lock-correctness checking in a BUG_ON, simply remove it. >>> >>> Signed-off-by: Bobby Powers <bobbypowers@gmail.com> CC: Ilya >>> Dryomov <idryomov@gmail.com> CC: Chris Mason >>> <chris.mason@oracle.com> CC: Andi Kleen <ak@linux.intel.com> CC: >>> linux-btrfs@vger.kernel.org CC: linux-kernel@vger.kernel.org --- >>> fs/btrfs/extent-tree.c | 3 --- 1 file changed, 3 deletions(-) >>> >>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index >>> a844204..c98b073 100644 --- a/fs/btrfs/extent-tree.c +++ >>> b/fs/btrfs/extent-tree.c @@ -3158,9 +3158,6 @@ 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)); - if (!bctl) return 0; >>> >> >> Why not replace both of these with lockdep_assert_held as Andi >> suggested in his doc? > > The complication here is that the existing statement was asserting > that _either_ volume_mutex was held _or_ balance_lock was held - > lockdep_assert_held is defined as: > > #define lockdep_assert_held(l) WARN_ON(debug_locks && !lockdep_is_held(l))Well, I guess it works fine if lockdep.h defines lockdep_is_held(l) for the !LOCKDEP case: BUG_ON(debug_locks && !lockdep_is_held(&fs_info->volume_mutex) && !lockdep_is_held(&fs_info->balance_lock));> which doesn''t map to the existing logic. Although I could be missing something. > > Sorry for the double email, forgot to turn off html mail initially. > > yours, > Bobby > >> - -Jeff >> >> - -- >> Jeff Mahoney >> SUSE Labs >> -----BEGIN PGP SIGNATURE----- >> Version: GnuPG v2.0.18 (GNU/Linux) >> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ >> >> iQIcBAEBAgAGBQJPfOtVAAoJEB57S2MheeWyew4P/ju2FiKPWvFEhShP2BQzmPPL >> wYoYP9cEmuoJbr1FXnpVXRy3RJsS+Dv9IHyUt2l8WRddO5PeTC08fS3a1PsR6Lky >> dnDBpOTsvBhdUPClA+9cu9HtNZ488PKALTgNX7kocVYm+vd0vkE54Iv0OWRvIMEA >> 5nmm/r2MJwgQmTsFwAWojxfiSEJNzRSJ6GXZFbIfwGOaJIx7MmnPg4R3PKU1SZiF >> ogKIwocfsTA/T7eplK58+EqtQnfTGTKzAbaEQvX/w1ryRRXHqD2zdk2p+zSAtpN6 >> swnhu156Bb9t2EUPTv9KiYth0BoYhYy9ppdp2Wyh0hX3lYCAP1SrwBVIYCqpvdqr >> CuipFWmqbupW41IjQc6bjoIwaVlGNsqwDY3NNrjR+kb29k3+/3MT8QH1ZXLhlpTB >> cUixluE62J8QllW4u3Wa2mLMTqdolcWCCTdh4yUqE+8jxguXUKhoTni1vwApmkru >> PM158CLPdxWCnDe1TaGcRYcBoweWPl6UDaVj8W+LdSVcYsycZhwehvDg2amX6pdg >> 9QFUf25PbDzEVw99w3f2hMhRG5pERRheLqPcFUVbnqZYkZBACt9XtIYBINlREoYW >> ACjr9dJszluF9dEKWOmlKhsah3gAGJJoC5+QU8oR+vpxKdrI+8vQ+NYtVhCr0hVA >> CO/KEEcwNaobsCWiAbSr >> =5Op/ >> -----END PGP SIGNATURE------- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bobby Powers
2012-Apr-05 02:04 UTC
[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 <idryomov@gmail.com>
CC: Chris Mason <chris.mason@oracle.com>
CC: Andi Kleen <ak@linux.intel.com>
CC: Jeff Mahoney <jeffm@suse.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: linux-kernel@vger.kernel.org
Signed-off-by: Bobby Powers <bobbypowers@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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bobby Powers
2012-Apr-05 16:23 UTC
Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON
On Wed, Apr 4, 2012 at 10:04 PM, Bobby Powers <bobbypowers@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 checkingI 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 <idryomov@gmail.com> > CC: Chris Mason <chris.mason@oracle.com> > CC: Andi Kleen <ak@linux.intel.com> > CC: Jeff Mahoney <jeffm@suse.de> > CC: Ingo Molnar <mingo@redhat.com> > CC: linux-kernel@vger.kernel.org > Signed-off-by: Bobby Powers <bobbypowers@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ilya Dryomov
2012-Apr-05 16:51 UTC
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 <bobbypowers@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
Mitch Harder
2012-Apr-06 17:20 UTC
Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON
On Thu, Apr 5, 2012 at 11:51 AM, Ilya Dryomov <idryomov@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 <bobbypowers@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.
Ilya Dryomov
2012-Apr-06 20:05 UTC
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 <idryomov@gmail.com> > CC: Chris Mason <chris.mason@oracle.com> > CC: Andi Kleen <ak@linux.intel.com> > CC: Jeff Mahoney <jeffm@suse.de> > CC: Ingo Molnar <mingo@redhat.com> > CC: linux-kernel@vger.kernel.org > Signed-off-by: Bobby Powers <bobbypowers@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.g19a6cOK, 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
Bobby Powers
2012-Apr-07 01:06 UTC
Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON
On Fri, Apr 6, 2012 at 4:05 PM, Ilya Dryomov <idryomov@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 <idryomov@gmail.com> >> CC: Chris Mason <chris.mason@oracle.com> >> CC: Andi Kleen <ak@linux.intel.com> >> CC: Jeff Mahoney <jeffm@suse.de> >> CC: Ingo Molnar <mingo@redhat.com> >> CC: linux-kernel@vger.kernel.org >> Signed-off-by: Bobby Powers <bobbypowers@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