On Tue, 26 Sep 2023 20:54:01 +0800
Yu Kuai [off-list ref] wrote:

Hi,

在 2023/09/26 20:45, Mariusz Tkaczyk 写道:

quoted

On Tue, 26 Sep 2023 10:58:26 +0800
Yu Kuai [off-list ref] wrote:
  

quoted

From: Yu Kuai <redacted>

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

quoted

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