[PATCH] btrfs: add lockdep_assert_held to need_preemptive_reclaim

[PATCH] btrfs: add lockdep_assert_held to need_preemptive_reclaim

[Niels Dossche]

In a previous patch I extended the locking for member accesses of
space_info. It was then suggested to also add a lockdep assertion for
space_info->lock to need_preemptive_reclaim.

Suggested-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
---
 fs/btrfs/space-info.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 294242c194d8..5464bd168d5b 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -734,9 +734,13 @@ static bool need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
 {
 	u64 global_rsv_size = fs_info->global_block_rsv.reserved;
 	u64 ordered, delalloc;
-	u64 thresh = div_factor_fine(space_info->total_bytes, 90);
+	u64 thresh;
 	u64 used;
 
+	lockdep_assert_held(&space_info->lock);
+
+	thresh = div_factor_fine(space_info->total_bytes, 90);
+
 	/* If we're just plain full then async reclaim just slows us down. */
 	if ((space_info->bytes_used + space_info->bytes_reserved +
 	     global_rsv_size) >= thresh)
-- 
2.35.1

Re: [PATCH] btrfs: add lockdep_assert_held to need_preemptive_reclaim

[David Sterba]

On Mon, Feb 28, 2022 at 11:52:16PM +0100, Niels Dossche wrote:
> In a previous patch I extended the locking for member accesses of
> space_info.

A reference to another patch would be by a subject or a specific commit
id (not applicable in this case) or you can write it without any
reference if the change is standalone. The changelog should describe the
reason for the change, user visible effects, what can go wrong etc.

> It was then suggested to also add a lockdep assertion for
> space_info->lock to need_preemptive_reclaim.
> 
> Suggested-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
> ---
>  fs/btrfs/space-info.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index 294242c194d8..5464bd168d5b 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -734,9 +734,13 @@ static bool need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
>  {
>  	u64 global_rsv_size = fs_info->global_block_rsv.reserved;
>  	u64 ordered, delalloc;
> -	u64 thresh = div_factor_fine(space_info->total_bytes, 90);
> +	u64 thresh;
>  	u64 used;
>  
> +	lockdep_assert_held(&space_info->lock);
> +
> +	thresh = div_factor_fine(space_info->total_bytes, 90);

I'm not sure this is necessary, as this is not locking where the
initialization would have to be inside. The lockdep assertion is just a
warning, so it does not matter where the intialization is done, I'd
prefer to keep it as is.

Re: [PATCH] btrfs: add lockdep_assert_held to need_preemptive_reclaim

[Niels Dossche]

On 02/03/2022 20:30, David Sterba wrote:
> On Mon, Feb 28, 2022 at 11:52:16PM +0100, Niels Dossche wrote:
>> In a previous patch I extended the locking for member accesses of
>> space_info.
> 
> A reference to another patch would be by a subject or a specific commit
> id (not applicable in this case) or you can write it without any
> reference if the change is standalone. The changelog should describe the
> reason for the change, user visible effects, what can go wrong etc.
> 

I will make sure to do that in the future. Thanks.

>> It was then suggested to also add a lockdep assertion for
>> space_info->lock to need_preemptive_reclaim.
>>
>> Suggested-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
>> ---
>>  fs/btrfs/space-info.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
>> index 294242c194d8..5464bd168d5b 100644
>> --- a/fs/btrfs/space-info.c
>> +++ b/fs/btrfs/space-info.c
>> @@ -734,9 +734,13 @@ static bool need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
>>  {
>>  	u64 global_rsv_size = fs_info->global_block_rsv.reserved;
>>  	u64 ordered, delalloc;
>> -	u64 thresh = div_factor_fine(space_info->total_bytes, 90);
>> +	u64 thresh;
>>  	u64 used;
>>  
>> +	lockdep_assert_held(&space_info->lock);
>> +
>> +	thresh = div_factor_fine(space_info->total_bytes, 90);
> 
> I'm not sure this is necessary, as this is not locking where the
> initialization would have to be inside. The lockdep assertion is just a
> warning, so it does not matter where the intialization is done, I'd
> prefer to keep it as is.

I understand. Thank you for your feedback. I will send a v2 shortly.