Re: [PATCH] md: Don't clear MD_CLOSING when the raid is about to stop

[Yu Kuai]

Hi,

在 2023/12/11 17:56, Mariusz Tkaczyk 写道:
> On Mon, 11 Dec 2023 16:17:14 +0800
> linan666@xxxxxxxxxxxxxxx wrote:
>
> > From: Li Nan <linan122@xxxxxxxxxx>
> >
> > The raid should not be opened anymore when it is about to be stopped.
> > However, other processes can open it again if the flag MD_CLOSING is
> > cleared before exiting. From now on, this flag will not be cleared when
> > the raid will be stopped.
> >
> > Fixes: 065e519e71b2 ("md: MD_CLOSING needs to be cleared after called
> > md_set_readonly or do_md_stop") Signed-off-by: Li Nan <linan122@xxxxxxxxxx>
>
> Hello Li Nan,
> I was there when I needed to fix this:
> https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git/commit/?h=md-next&id=c8870379a21fbd9ad14ca36204ccfbe9d25def43
>
> For sure, you have to consider applying same solution for array_store "clear".
> Minor nit below.
>
> Thanks,
> Mariusz
>
> > ---
> >   drivers/md/md.c | 8 +++-----
> >   1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 4e9fe5cbeedc..ebdfc9068a60 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -6238,7 +6238,6 @@ static void md_clean(struct mddev *mddev)
> >   	mddev->persistent = 0;
> >   	mddev->level = LEVEL_NONE;
> >   	mddev->clevel[0] = 0;
> > -	mddev->flags = 0;
>
> I recommend (safety recommendation):
> 	mddev->flags = MD_CLOSING;

Taking a look I think both MD_CLOSING and MD_DELETED should not be
cleared, however, there is no guarantee that MD_CLOSING will be set
before md_clean, because mdadm can be removed without running. Hence I
think just set MD_CLOSING is werid.

I think the proper way is to keep MD_CLOSING and MD_DELETED if they are
set. However, there is no such api to clear other bits at once. Since
we're not expecting anyone else to write flags, following maybe
acceptable:

mddev->flags &= BIT_ULL_MASK(MD_CLOSING) | BIT_ULL_MASK(MD_DELETED);

Or after making sure other flags cannot race, this patch is ok.

Thanks,
Kuai

> Unless you can prove that other flags cannot race.
>
> >   	mddev->sb_flags = 0;
> >   	mddev->ro = MD_RDWR;
> >   	mddev->metadata_type[0] = 0;
>
> .