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

[Mariusz Tkaczyk]

On Tue, 12 Dec 2023 11:21:28 +0800
Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:

> 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);

Yes, MD_CLOSING is a bit number to not a bit value I can assign directly.
Thanks for clarifying!

Mariusz
> 
> 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;  
> > 
> > .
> >   
>