From: Xiao Ni <xni@redhat.com>
To: Yu Kuai <yukuai1@huaweicloud.com>
Cc: song@kernel.org, bmarzins@redhat.com, heinzm@redhat.com,
snitzer@kernel.org, ncroxon@redhat.com,
linux-raid@vger.kernel.org, dm-devel@lists.linux.dev,
"yukuai (C)" <yukuai3@huawei.com>
Subject: Re: [PATCH 1/6] md: Revert "md: Don't register sync_thread for reshape directly"
Date: Fri, 1 Mar 2024 12:41:16 +0800 [thread overview]
Message-ID: <CALTww29xwFA83m6=PjJZwr6Y=p4A10LMMmkMS6ofstBJiQzfVw@mail.gmail.com> (raw)
In-Reply-To: <042093c6-6deb-535c-918e-78160e7addc4@huaweicloud.com>
On Fri, Mar 1, 2024 at 10:38 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> Hi,
>
> 在 2024/02/29 23:49, Xiao Ni 写道:
> > This reverts commit ad39c08186f8a0f221337985036ba86731d6aafe.
> >
> > Function stop_sync_thread only wakes up sync task. It also needs to
> > wake up sync thread. This problem will be fixed in the following
> > patch.
>
> I don't think so, unlike mddev->thread, sync_thread will only be
> executed once and must be executed each time it's registered, and caller
> must make sure to wake up registered sync_thread.
Hi Kuai
I'll modify the comments. But it should be right to
wake_up(mddev->sync_thread) in function stop_sync_thread too? You gave
the same patch yesterday too. I know the caller should wake up sync
thread too.
"However, I think the one to register sync_thread is responsible to
wake it up." I put your comments here. If I understand correctly, we
can do something like this?
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7937,6 +7937,7 @@ static int raid5_run(struct mddev *mddev)
set_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
rcu_assign_pointer(mddev->sync_thread,
md_register_thread(md_do_sync, mddev, "reshape"));
+ md_wakeup_thread(mddev->sync_thread);
if (!mddev->sync_thread)
goto abort;
}
And at first, I didn't revert
ad39c08186f8a0f221337985036ba86731d6aafe. But with my patch set, it
can cause failure in lvm2 test suit. And the patch you gave yesterday
is part of my patch01, so I revert it. Are you good if I change the
comments and with the modification (wake up sync thread after
registering reshape)?
Best Regards
Xiao
>
> Thanks,
> Kuai
> >
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> > drivers/md/md.c | 5 +----
> > drivers/md/raid10.c | 16 ++++++++++++++--
> > drivers/md/raid5.c | 29 +++++++++++++++++++++++++++--
> > 3 files changed, 42 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 9e41a9aaba8b..db4743ba7f6c 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -9376,7 +9376,6 @@ static void md_start_sync(struct work_struct *ws)
> > struct mddev *mddev = container_of(ws, struct mddev, sync_work);
> > int spares = 0;
> > bool suspend = false;
> > - char *name;
> >
> > /*
> > * If reshape is still in progress, spares won't be added or removed
> > @@ -9414,10 +9413,8 @@ static void md_start_sync(struct work_struct *ws)
> > if (spares)
> > md_bitmap_write_all(mddev->bitmap);
> >
> > - name = test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) ?
> > - "reshape" : "resync";
> > rcu_assign_pointer(mddev->sync_thread,
> > - md_register_thread(md_do_sync, mddev, name));
> > + md_register_thread(md_do_sync, mddev, "resync"));
> > if (!mddev->sync_thread) {
> > pr_warn("%s: could not start resync thread...\n",
> > mdname(mddev));
> > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> > index a5f8419e2df1..7412066ea22c 100644
> > --- a/drivers/md/raid10.c
> > +++ b/drivers/md/raid10.c
> > @@ -4175,7 +4175,11 @@ static int raid10_run(struct mddev *mddev)
> > clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> > clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> > set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
> > - set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> > + set_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
> > + rcu_assign_pointer(mddev->sync_thread,
> > + md_register_thread(md_do_sync, mddev, "reshape"));
> > + if (!mddev->sync_thread)
> > + goto out_free_conf;
> > }
> >
> > return 0;
> > @@ -4569,8 +4573,16 @@ static int raid10_start_reshape(struct mddev *mddev)
> > clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> > clear_bit(MD_RECOVERY_DONE, &mddev->recovery);
> > set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
> > - set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> > + set_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
> > +
> > + rcu_assign_pointer(mddev->sync_thread,
> > + md_register_thread(md_do_sync, mddev, "reshape"));
> > + if (!mddev->sync_thread) {
> > + ret = -EAGAIN;
> > + goto abort;
> > + }
> > conf->reshape_checkpoint = jiffies;
> > + md_wakeup_thread(mddev->sync_thread);
> > md_new_event();
> > return 0;
> >
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index 6a7a32f7fb91..8497880135ee 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -7936,7 +7936,11 @@ static int raid5_run(struct mddev *mddev)
> > clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> > clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> > set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
> > - set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> > + set_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
> > + rcu_assign_pointer(mddev->sync_thread,
> > + md_register_thread(md_do_sync, mddev, "reshape"));
> > + if (!mddev->sync_thread)
> > + goto abort;
> > }
> >
> > /* Ok, everything is just fine now */
> > @@ -8502,8 +8506,29 @@ static int raid5_start_reshape(struct mddev *mddev)
> > clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> > clear_bit(MD_RECOVERY_DONE, &mddev->recovery);
> > set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
> > - set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> > + set_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
> > + rcu_assign_pointer(mddev->sync_thread,
> > + md_register_thread(md_do_sync, mddev, "reshape"));
> > + if (!mddev->sync_thread) {
> > + mddev->recovery = 0;
> > + spin_lock_irq(&conf->device_lock);
> > + write_seqcount_begin(&conf->gen_lock);
> > + mddev->raid_disks = conf->raid_disks = conf->previous_raid_disks;
> > + mddev->new_chunk_sectors =
> > + conf->chunk_sectors = conf->prev_chunk_sectors;
> > + mddev->new_layout = conf->algorithm = conf->prev_algo;
> > + rdev_for_each(rdev, mddev)
> > + rdev->new_data_offset = rdev->data_offset;
> > + smp_wmb();
> > + conf->generation--;
> > + conf->reshape_progress = MaxSector;
> > + mddev->reshape_position = MaxSector;
> > + write_seqcount_end(&conf->gen_lock);
> > + spin_unlock_irq(&conf->device_lock);
> > + return -EAGAIN;
> > + }
> > conf->reshape_checkpoint = jiffies;
> > + md_wakeup_thread(mddev->sync_thread);
> > md_new_event();
> > return 0;
> > }
> >
>
next prev parent reply other threads:[~2024-03-01 4:41 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-29 15:49 [PATCH 0/6] Fix dmraid regression bugs Xiao Ni
2024-02-29 15:49 ` [PATCH 1/6] md: Revert "md: Don't register sync_thread for reshape directly" Xiao Ni
2024-03-01 2:38 ` Yu Kuai
2024-03-01 4:41 ` Xiao Ni [this message]
2024-02-29 15:49 ` [PATCH 2/6] md: Revert "md: Make sure md_do_sync() will set MD_RECOVERY_DONE" Xiao Ni
2024-02-29 22:53 ` Song Liu
2024-02-29 23:45 ` Song Liu
2024-03-01 0:49 ` Xiao Ni
2024-03-01 1:11 ` Song Liu
2024-02-29 15:49 ` [PATCH 3/6] md: Revert "md: Don't ignore suspended array in md_check_recovery()" Xiao Ni
2024-02-29 15:49 ` [PATCH 4/6] dm-raid/md: Clear MD_RECOVERY_WAIT when stopping dmraid Xiao Ni
2024-03-01 2:44 ` Yu Kuai
2024-03-01 4:19 ` Xiao Ni
2024-02-29 15:49 ` [PATCH 5/6] md: Set MD_RECOVERY_FROZEN before stop sync thread Xiao Ni
2024-02-29 15:49 ` [PATCH 6/6] md/raid5: Don't check crossing reshape when reshape hasn't started Xiao Ni
2024-02-29 19:39 ` [PATCH 0/6] Fix dmraid regression bugs Christoph Hellwig
2024-02-29 19:45 ` Song Liu
2024-03-01 2:12 ` Yu Kuai
2024-03-01 2:22 ` Xiao Ni
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CALTww29xwFA83m6=PjJZwr6Y=p4A10LMMmkMS6ofstBJiQzfVw@mail.gmail.com' \
--to=xni@redhat.com \
--cc=bmarzins@redhat.com \
--cc=dm-devel@lists.linux.dev \
--cc=heinzm@redhat.com \
--cc=linux-raid@vger.kernel.org \
--cc=ncroxon@redhat.com \
--cc=snitzer@kernel.org \
--cc=song@kernel.org \
--cc=yukuai1@huaweicloud.com \
--cc=yukuai3@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).