Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Subject: Re: [PATCH -next v2 2/6] md: remove flag RemoveSynchronized
To:     Song Liu <song@kernel.org>, Yu Kuai <yukuai1@huaweicloud.com>
Cc:     linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org,
        yi.zhang@huawei.com, yangerkun@huawei.com,
        "yukuai (C)" <yukuai3@huawei.com>
References: <20231021102059.3198284-1-yukuai1@huaweicloud.com>
 <20231021102059.3198284-3-yukuai1@huaweicloud.com>
 <CAPhsuW6kZ4voZQPsYQK4O9=5LtFvB8UY_oSWXkOTnek=aJPidw@mail.gmail.com>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <fb6a482a-a546-3e6b-0a0a-238f81b0bb23@huaweicloud.com>
Date:   Fri, 24 Nov 2023 17:13:03 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <CAPhsuW6kZ4voZQPsYQK4O9=5LtFvB8UY_oSWXkOTnek=aJPidw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

在 2023/11/24 16:28, Song Liu 写道:
> On Fri, Oct 20, 2023 at 7:25 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> rcu is not used correctly here, because synchronize_rcu() is called
>> before replacing old value, for example:
>>
>> remove_and_add_spares   // other path
>>   synchronize_rcu
>>   // called before replacing old value
>>   set_bit(RemoveSynchronized)
>>                          rcu_read_lock()
>>                          rdev = conf->mirros[].rdev
>>   pers->hot_remove_disk
>>    conf->mirros[].rdev = NULL;
>>    if (!test_bit(RemoveSynchronized))
>>     synchronize_rcu
>>     /*
>>      * won't be called, and won't wait
>>      * for concurrent readers to be done.
>>      */
>>                          // access rdev after remove_and_add_spares()
>>                          rcu_read_unlock()
>>
>> Fortunately, there is a separate rcu protection to prevent such rdev
>> to be freed:
>>
>> md_kick_rdev_from_array         //other path
>>                                  rcu_read_lock()
>>                                  rdev = conf->mirros[].rdev
>> list_del_rcu(&rdev->same_set)
>>
>>                                  rcu_read_unlock()
>>                                  /*
>>                                   * rdev can be removed from conf, but
>>                                   * rdev won't be freed.
>>                                   */
>> synchronize_rcu()
>> free rdev
>>
>> Hence remove this useless flag and prepare to remove rcu protection to
>> access rdev from 'conf'.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> 
> RemoveSynchronized is not removed from md.h.

Yes, thanks for the notice.

Kuai
> 
> 
>> ---
>>   drivers/md/md-multipath.c |  9 ---------
>>   drivers/md/md.c           | 37 ++++++-------------------------------
>>   drivers/md/raid1.c        |  9 ---------
>>   drivers/md/raid10.c       |  9 ---------
>>   drivers/md/raid5.c        |  9 ---------
>>   5 files changed, 6 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/md/md-multipath.c b/drivers/md/md-multipath.c
>> index d22276870283..aa77133f3188 100644
>> --- a/drivers/md/md-multipath.c
>> +++ b/drivers/md/md-multipath.c
>> @@ -258,15 +258,6 @@ static int multipath_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
>>                          goto abort;
>>                  }
>>                  p->rdev = NULL;
>> -               if (!test_bit(RemoveSynchronized, &rdev->flags)) {
>> -                       synchronize_rcu();
>> -                       if (atomic_read(&rdev->nr_pending)) {
>> -                               /* lost the race, try later */
>> -                               err = -EBUSY;
>> -                               p->rdev = rdev;
>> -                               goto abort;
>> -                       }
>> -               }
>>                  err = md_integrity_register(mddev);
>>          }
>>   abort:
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 09686d8db983..68f3bb6e89cb 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -9250,44 +9250,19 @@ static int remove_and_add_spares(struct mddev *mddev,
>>          struct md_rdev *rdev;
>>          int spares = 0;
>>          int removed = 0;
>> -       bool remove_some = false;
>>
>>          if (this && test_bit(MD_RECOVERY_RUNNING, &mddev->recovery))
>>                  /* Mustn't remove devices when resync thread is running */
>>                  return 0;
>>
>>          rdev_for_each(rdev, mddev) {
>> -               if ((this == NULL || rdev == this) &&
>> -                   rdev->raid_disk >= 0 &&
>> -                   !test_bit(Blocked, &rdev->flags) &&
>> -                   test_bit(Faulty, &rdev->flags) &&
>> -                   atomic_read(&rdev->nr_pending)==0) {
>> -                       /* Faulty non-Blocked devices with nr_pending == 0
>> -                        * never get nr_pending incremented,
>> -                        * never get Faulty cleared, and never get Blocked set.
>> -                        * So we can synchronize_rcu now rather than once per device
>> -                        */
>> -                       remove_some = true;
>> -                       set_bit(RemoveSynchronized, &rdev->flags);
>> -               }
>> -       }
>> -
>> -       if (remove_some)
>> -               synchronize_rcu();
>> -       rdev_for_each(rdev, mddev) {
>> -               if ((this == NULL || rdev == this) &&
>> -                   (test_bit(RemoveSynchronized, &rdev->flags) ||
>> -                    rdev_removeable(rdev))) {
>> -                       if (mddev->pers->hot_remove_disk(
>> -                                   mddev, rdev) == 0) {
>> -                               sysfs_unlink_rdev(mddev, rdev);
>> -                               rdev->saved_raid_disk = rdev->raid_disk;
>> -                               rdev->raid_disk = -1;
>> -                               removed++;
>> -                       }
>> +               if ((this == NULL || rdev == this) && rdev_removeable(rdev) &&
>> +                   !mddev->pers->hot_remove_disk(mddev, rdev)) {
>> +                       sysfs_unlink_rdev(mddev, rdev);
>> +                       rdev->saved_raid_disk = rdev->raid_disk;
>> +                       rdev->raid_disk = -1;
>> +                       removed++;
>>                  }
>> -               if (remove_some && test_bit(RemoveSynchronized, &rdev->flags))
>> -                       clear_bit(RemoveSynchronized, &rdev->flags);
>>          }
>>
>>          if (removed && mddev->kobj.sd)
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index c13088eae401..4348d670439d 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -1836,15 +1836,6 @@ static int raid1_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
>>                          goto abort;
>>                  }
>>                  p->rdev = NULL;
>> -               if (!test_bit(RemoveSynchronized, &rdev->flags)) {
>> -                       synchronize_rcu();
>> -                       if (atomic_read(&rdev->nr_pending)) {
>> -                               /* lost the race, try later */
>> -                               err = -EBUSY;
>> -                               p->rdev = rdev;
>> -                               goto abort;
>> -                       }
>> -               }
>>                  if (conf->mirrors[conf->raid_disks + number].rdev) {
>>                          /* We just removed a device that is being replaced.
>>                           * Move down the replacement.  We drain all IO before
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index 4b5f34f320c8..33ab00323cae 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -2219,15 +2219,6 @@ static int raid10_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
>>                  goto abort;
>>          }
>>          *rdevp = NULL;
>> -       if (!test_bit(RemoveSynchronized, &rdev->flags)) {
>> -               synchronize_rcu();
>> -               if (atomic_read(&rdev->nr_pending)) {
>> -                       /* lost the race, try later */
>> -                       err = -EBUSY;
>> -                       *rdevp = rdev;
>> -                       goto abort;
>> -               }
>> -       }
>>          if (p->replacement) {
>>                  /* We must have just cleared 'rdev' */
>>                  p->rdev = p->replacement;
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index 27a4dce51c92..a80be51b4825 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -8202,15 +8202,6 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
>>                  goto abort;
>>          }
>>          *rdevp = NULL;
>> -       if (!test_bit(RemoveSynchronized, &rdev->flags)) {
>> -               lockdep_assert_held(&mddev->reconfig_mutex);
>> -               synchronize_rcu();
>> -               if (atomic_read(&rdev->nr_pending)) {
>> -                       /* lost the race, try later */
>> -                       err = -EBUSY;
>> -                       rcu_assign_pointer(*rdevp, rdev);
>> -               }
>> -       }
>>          if (!err) {
>>                  err = log_modify(conf, rdev, false);
>>                  if (err)
>> --
>> 2.39.2
>>
> 
> .
>