Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Tue, 26 Sep 2023 15:19:34 +0200
From:   Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
To:     Yu Kuai <yukuai1@huaweicloud.com>
Cc:     xni@redhat.com, song@kernel.org, linux-raid@vger.kernel.org,
        linux-kernel@vger.kernel.org, yi.zhang@huawei.com,
        yangerkun@huawei.com, "yukuai (C)" <yukuai3@huawei.com>
Subject: Re: [PATCH v2 1/2] md: factor out a new helper to put mddev
Message-ID: <20230926151934.00003cb1@linux.intel.com>
In-Reply-To: <3d304e39-60c1-8f3c-f8b3-de8850d70b82@huaweicloud.com>
References: <20230926025827.671407-1-yukuai1@huaweicloud.com>
        <20230926025827.671407-2-yukuai1@huaweicloud.com>
        <20230926144536.0000017d@linux.intel.com>
        <3d304e39-60c1-8f3c-f8b3-de8850d70b82@huaweicloud.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=gbk
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Tue, 26 Sep 2023 20:54:01 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> Hi,
> 
> ?? 2023/09/26 20:45, Mariusz Tkaczyk д??:
> > On Tue, 26 Sep 2023 10:58:26 +0800
> > Yu Kuai <yukuai1@huaweicloud.com> wrote:
> >   
> >> From: Yu Kuai <yukuai3@huawei.com>
> >>
> >> There are no functional changes, the new helper will still hold
> >> 'all_mddevs_lock' after putting mddev, and it will be used to simplify
> >> md_seq_ops.
> >>
> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> >> ---
> >>   drivers/md/md.c | 18 +++++++++++++++---
> >>   1 file changed, 15 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> index 10cb4dfbf4ae..a5ef6f7da8ec 100644
> >> --- a/drivers/md/md.c
> >> +++ b/drivers/md/md.c
> >> @@ -616,10 +616,15 @@ static inline struct mddev *mddev_get(struct mddev
> >> *mddev)
> >>   static void mddev_delayed_delete(struct work_struct *ws);
> >>   
> >> -void mddev_put(struct mddev *mddev)
> >> +static void __mddev_put(struct mddev *mddev, bool locked)
> >>   {
> >> -	if (!atomic_dec_and_lock(&mddev->active, &all_mddevs_lock))
> >> +	if (locked) {
> >> +		spin_lock(&all_mddevs_lock);
> >> +		if (!atomic_dec_and_test(&mddev->active))
> >> +			return;  
> > 
> > It is "locked" and we are taking lock? It seems weird to me. Perhaps
> > "do_lock" would be better? Do you meant
> > "lockdep_assert_held(&all_mddevs_lock);"  
> 
> Yes, do_lock is a better name, true means this function will return with
> lock held.
> > 
> > Something is wrong here, we have two paths and in both cases we are
> > taking lock.  
> 
> No, in the first path, lock is held unconditionaly, that's what we
> expected in md_seq_show(); in the next path, lock will only be held if
> active is decreased to 0.
> 

Ok I see, you described it in commit message.
IMO it is bad practice to return with locked resource and not highlight it in
function name.In this case, I would prefer to respect that device is already
locked, not lock it here:

(assuming bool means "locked")
spin_lock(&all_mddevs_lock);
__mddev_put(mddev, true); <- function known that lock is held.
spin_unlock((mddev);

your "do_lock" approach:
__mddev_put(mddev, true); <- lock is taken here and we are returning 
spin_unlock((mddev);

You could change name to something like "all_mddev_lock_and_put(mddev)" to
indicate that we are locking all_mddevs. It fits for me too.
Note: it is just my preference, feel free to ignore :)

Mariusz