[PATCH] Btrfs: remove BUG_ON from get_restripe_target

[PATCH] Btrfs: remove BUG_ON from get_restripe_target

[Bobby Powers]

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

Re: [PATCH] Btrfs: remove BUG_ON from get_restripe_target

[Jeff Mahoney]

-----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-----

Re: [PATCH] Btrfs: remove BUG_ON from get_restripe_target

[Bobby Powers]

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. =A0There 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 | =A0 =A03 --- 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 =3D fs_info->balance_ctl; u64
>> target =3D 0;
>>
>> - =A0 =A0 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 s=
omething.

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
> =3D5Op/
> -----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

Re: [PATCH] Btrfs: remove BUG_ON from get_restripe_target

[Bobby Powers]

On Wed, Apr 4, 2012 at 9:19 PM, Bobby Powers <bobbypowers@gmail.com> wr=
ote:
> 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. =A0There is documentation pending as to wh=
y
>>> 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 | =A0 =A03 --- 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 =3D fs_info->balance_ctl; u64
>>> target =3D 0;
>>>
>>> - =A0 =A0 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) =A0WARN_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. =A0Although I could be missi=
ng 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
>> =3D5Op/
>> -----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

[PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON

[Bobby Powers]

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

Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON

[Bobby Powers]

On Wed, Apr 4, 2012 at 10:04 PM, Bobby Powers <bobbypowers@gmail.com> w=
rote:
> spin_is_locked always returns 0 on non-SMP systems, which causes btrf=
s
> to fail the mount. =A0There is documentation pending as to why checki=
ng

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. =A0Luckily
> 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>
> ---
> =A0fs/btrfs/extent-tree.c =A0| =A0 =A05 +++--
> =A0include/linux/lockdep.h | =A0 =A01 +
> =A02 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 @@
> =A0#include <linux/kthread.h>
> =A0#include <linux/slab.h>
> =A0#include <linux/ratelimit.h>
> +#include <linux/lockdep.h>
> =A0#include "compat.h"
> =A0#include "hash.h"
> =A0#include "ctree.h"
> @@ -3158,8 +3159,8 @@ static u64 get_restripe_target(struct btrfs_fs_=
info *fs_info, u64 flags)
> =A0 =A0 =A0 =A0struct btrfs_balance_control *bctl =3D fs_info->balanc=
e_ctl;
> =A0 =A0 =A0 =A0u64 target =3D 0;
>
> - =A0 =A0 =A0 BUG_ON(!mutex_is_locked(&fs_info->volume_mutex) &&
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0!spin_is_locked(&fs_info->balance_lock))=
;
> + =A0 =A0 =A0 BUG_ON(debug_locks && !lockdep_is_held(&fs_info->volume=
_mutex) &&
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0!lockdep_is_held(&fs_info->balance_lock)=
);
>
> =A0 =A0 =A0 =A0if (!bctl)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 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 { };
>
> =A0#define lockdep_depth(tsk) =A0 =A0 (0)
>
> +#define lockdep_is_held(l) =A0 =A0 (0)
> =A0#define lockdep_assert_held(l) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 do =
{ } while (0)
>
> =A0#define lockdep_recursing(tsk) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (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

Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON

[Ilya Dryomov]

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 bt=
rfs
> > to fail the mount. =C2=A0There is documentation pending as to why c=
hecking
>=20
> 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

Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON

[Mitch Harder]

On Thu, Apr 5, 2012 at 11:51 AM, Ilya Dryomov <idryomov@gmail.com> wrot=
e:
> 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 b=
trfs
>> > to fail the mount. =A0There is documentation pending as to why che=
cking
>>
>> I guess I should be explicit in stating that this is a regression, s=
o
>> 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. =
=A0I
> don't have a strong opinion on this assert, if there are objections t=
o
> 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.

Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON

[Ilya Dryomov]

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.

Thanks,

		Ilya

Re: [PATCH v2] Btrfs, lockdep: get_restripe_target: use lockdep in BUG_ON

[Bobby Powers]

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 btr=
fs
>> to fail the mount. =A0There is documentation pending as to why check=
ing
>> 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. =A0Luckily
>> 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>
>> ---
>> =A0fs/btrfs/extent-tree.c =A0| =A0 =A05 +++--
>> =A0include/linux/lockdep.h | =A0 =A01 +
>> =A02 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 @@
>> =A0#include <linux/kthread.h>
>> =A0#include <linux/slab.h>
>> =A0#include <linux/ratelimit.h>
>> +#include <linux/lockdep.h>
>> =A0#include "compat.h"
>> =A0#include "hash.h"
>> =A0#include "ctree.h"
>> @@ -3158,8 +3159,8 @@ static u64 get_restripe_target(struct btrfs_fs=
_info *fs_info, u64 flags)
>> =A0 =A0 =A0 struct btrfs_balance_control *bctl =3D fs_info->balance_=
ctl;
>> =A0 =A0 =A0 u64 target =3D 0;
>>
>> - =A0 =A0 BUG_ON(!mutex_is_locked(&fs_info->volume_mutex) &&
>> - =A0 =A0 =A0 =A0 =A0 =A0!spin_is_locked(&fs_info->balance_lock));
>> + =A0 =A0 BUG_ON(debug_locks && !lockdep_is_held(&fs_info->volume_mu=
tex) &&
>> + =A0 =A0 =A0 =A0 =A0 =A0!lockdep_is_held(&fs_info->balance_lock));
>>
>> =A0 =A0 =A0 if (!bctl)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 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 { };
>>
>> =A0#define lockdep_depth(tsk) =A0 (0)
>>
>> +#define lockdep_is_held(l) =A0 (0)
>> =A0#define lockdep_assert_held(l) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 do { } while (0)
>>
>> =A0#define lockdep_recursing(tsk) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 (0)
>> --
>> 1.7.10.rc3.3.g19a6c
>
> OK, Mitch's report prompted me to actually take a look. =A0This 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,
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Ilya