Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Subject: Re: [PATCH v3 2/3] md: don't leave 'MD_RECOVERY_FROZEN' in error path
 of md_set_readonly()
To:     Song Liu <song@kernel.org>, Yu Kuai <yukuai1@huaweicloud.com>
Cc:     xni@redhat.com, linux-raid@vger.kernel.org,
        linux-kernel@vger.kernel.org, yi.zhang@huawei.com,
        yangerkun@huawei.com, "yukuai (C)" <yukuai3@huawei.com>
References: <20231129043127.2245901-1-yukuai1@huaweicloud.com>
 <20231129043127.2245901-3-yukuai1@huaweicloud.com>
 <CAPhsuW75Qmn1QamykogAnMBDMGwMrfTKh+VeNCtxmjkyszgEag@mail.gmail.com>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <ad92f9cb-3d66-0ad8-aac3-b753bcadf7df@huaweicloud.com>
Date:   Sat, 2 Dec 2023 15:41:41 +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: <CAPhsuW75Qmn1QamykogAnMBDMGwMrfTKh+VeNCtxmjkyszgEag@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

在 2023/12/02 4:53, Song Liu 写道:
> On Tue, Nov 28, 2023 at 8:32 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> If md_set_readonly() failed, the array could still be read-write, however
>> 'MD_RECOVERY_FROZEN' could still be set, which leave the array in an
>> abnormal state that sync or recovery can't continue anymore.
>> Hence make sure the flag is cleared after md_set_readonly() returns.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> Acked-by: Xiao Ni <xni@redhat.com>
> 
> Since we are shipping this via the md-fixes branch, we need a Fixes tag.

Okay, I'll add following fix tag:

Fixes: 88724bfa68be ("md: wait for pending superblock updates before 
switching to read-only")
> 
>> ---
>>   drivers/md/md.c | 24 +++++++++++++-----------
>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 5640a948086b..2d8e45a1af23 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -6355,6 +6355,9 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
>>          int err = 0;
>>          int did_freeze = 0;
>>
>> +       if (mddev->external && test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags))
>> +               return -EBUSY;
>> +
>>          if (!test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) {
>>                  did_freeze = 1;
>>                  set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>> @@ -6369,8 +6372,6 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
>>           */
>>          md_wakeup_thread_directly(mddev->sync_thread);
>>
>> -       if (mddev->external && test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags))
>> -               return -EBUSY;
>>          mddev_unlock(mddev);
>>          wait_event(resync_wait, !test_bit(MD_RECOVERY_RUNNING,
>>                                            &mddev->recovery));
>> @@ -6383,29 +6384,30 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
>>              mddev->sync_thread ||
>>              test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
>>                  pr_warn("md: %s still in use.\n",mdname(mddev));
>> -               if (did_freeze) {
>> -                       clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>> -                       set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>> -                       md_wakeup_thread(mddev->thread);
>> -               }
> 
> This change (move did_freeze, etc.) is not explained in the commit log.
> Is it just refactor?

It is refactor, but it is also part of "make sure the flag is cleared
after md_set_readonly() returns", because now that MD_RECOVERY_FROZEN
will be cleared:

if ((mddev->pers && !err) || did_freeze)

Which means,
  - If set readonly succeed, or;
  - if something is wrong and did_freeze is set, exactly what this patch
    tries to do;

Thanks,
Kuai

> 
> Thanks,
> Song
> 
> 
>>                  err = -EBUSY;
>>                  goto out;
>>          }
>> +
>>          if (mddev->pers) {
>>                  __md_stop_writes(mddev);
>>
>> -               err  = -ENXIO;
>> -               if (mddev->ro == MD_RDONLY)
>> +               if (mddev->ro == MD_RDONLY) {
>> +                       err  = -ENXIO;
>>                          goto out;
>> +               }
>> +
>>                  mddev->ro = MD_RDONLY;
>>                  set_disk_ro(mddev->gendisk, 1);
>> +       }
>> +
>> +out:
>> +       if ((mddev->pers && !err) || did_freeze) {
>>                  clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>>                  set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>>                  md_wakeup_thread(mddev->thread);
>>                  sysfs_notify_dirent_safe(mddev->sysfs_state);
>> -               err = 0;
>>          }
>> -out:
>> +
>>          mutex_unlock(&mddev->open_mutex);
>>          return err;
>>   }
>> --
>> 2.39.2
>>
> .
>