Re: [PATCH v2 1/2] md: factor out a new helper to put mddev

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

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