On Wed, Aug 23, 2023 at 1:37 AM Yu Kuai [off-list ref] wrote:

[...]

quoted

diff --git i/drivers/md/md.c w/drivers/md/md.c
index 78be7811a89f..8cb855d03e0a 100644
--- i/drivers/md/md.c
+++ w/drivers/md/md.c

@@ -9117,6 +9117,20 @@ void md_do_sync(struct md_thread *thread)
  }
  EXPORT_SYMBOL_GPL(md_do_sync);

+static bool rdev_addable(struct md_rdev *rdev)
+{
+       if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
+           test_bit(Faulty, &rdev->flags))
+               return false;
+       return true;
+}
+
+static bool rdev_is_readd(struct md_rdev *rdev)
+{
+       return rdev->saved_raid_disk >= 0 ||
+               !test_bit(Bitmap_sync, &rdev->flags);

This should use '&&' instead of '||' ?

quoted

+}
+
  static int remove_and_add_spares(struct mddev *mddev,
                                  struct md_rdev *this)
  {

@@ -9176,25 +9190,24 @@ static int remove_and_add_spares(struct mddev *mddev,
         rdev_for_each(rdev, mddev) {
                 if (this && this != rdev)
                         continue;
-               if (test_bit(Candidate, &rdev->flags))
-                       continue;
                 if (rdev->raid_disk >= 0 &&
                     !test_bit(In_sync, &rdev->flags) &&
                     !test_bit(Journal, &rdev->flags) &&
                     !test_bit(Faulty, &rdev->flags))
                         spares++;
-               if (rdev->raid_disk >= 0)
+
+               if (!rdev_addable(rdev))
                         continue;
-               if (test_bit(Faulty, &rdev->flags))
+
+               if (test_bit(Journal, &rdev->flags))
+                       goto hot_add_disk;
+

I understand what you mean now, but I must use the exact same judgement
in the new helper md_spares_need_change() in patch 7, there will be
redundant code this way.

How about this, rework rdev_addable():

Yeah, this was another option that I was thinking about. Let's go with
this version.

Thanks,
Song

   static bool rdev_addable(struct md_rdev *rdev)
   {
+         /* rdev is already used, don't add it again. */
           if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
               test_bit(Faulty, &rdev->flags))
                   return false;

~         /* Allow to add journal disk. */
~         if (test_bit(Journal, &rdev->flags))
~_                return true;

~         /* Allow to add if array is read-write. */
+         if (md_is_rdwr(rdev->mddev))
+                 return true;
+
+         /*
+          * For read-only array, only allow to readd a rdev. And if
bitmap is
+          * used, don't allow to readd a rdev that is too old.
+          */
+         if (rdev->saved_raid_disk >=0 && !test_bit(Bitmap_sync,
&rdev->flags))
+                 return true;
+
+         return false;
   }