Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 2 Mar 2022 20:30:42 +0100
From:   David Sterba <dsterba@suse.cz>
To:     Niels Dossche <dossche.niels@gmail.com>
Cc:     linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>,
        Johannes Thumshirn <johannes.thumshirn@wdc.com>
Subject: Re: [PATCH] btrfs: add lockdep_assert_held to need_preemptive_reclaim
Message-ID: <20220302193042.GV12643@twin.jikos.cz>
Reply-To: dsterba@suse.cz
Mail-Followup-To: dsterba@suse.cz, Niels Dossche <dossche.niels@gmail.com>,
        linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>,
        Johannes Thumshirn <johannes.thumshirn@wdc.com>
References: <20220228225215.16552-1-dossche.niels@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220228225215.16552-1-dossche.niels@gmail.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Precedence: bulk

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.