Re: [PATCH 2/2] imsm: FIX: Be more patient during loading matadata

[NeilBrown <neilb@suse.de>]

On Wed, 13 Apr 2011 11:56:29 -0700 Dan Williams <dan.j.williams@intel.com>
wrote:

> 2011/4/12 Kwolek, Adam <adam.kwolek@intel.com>:
> >> -----Original Message-----
> >> From: dan.j.williams@gmail.com [mailto:dan.j.williams@gmail.com] On
> >> Behalf Of Dan Williams
> >> Sent: Wednesday, April 13, 2011 2:45 AM
> >> To: Kwolek, Adam
> >> Cc: neilb@suse.de; linux-raid@vger.kernel.org; Ciechanowski, Ed;
> >> Neubauer, Wojciech
> >> Subject: Re: [PATCH 2/2] imsm: FIX: Be more patient during loading
> >> matadata
> >>
> >> On Tue, Apr 12, 2011 at 5:51 AM, Adam Kwolek <adam.kwolek@intel.com>
> >> wrote:
> >> > Sometimes occurs that metadata cannot be loaded e.g. wrong check sum
> >> > It can happen due to metadata update racing with mdmon condition.
> >> > If mpb loading is tried again, it is loaded successfully.
> >> > Try to load metadata again before really giving up.
> >> >
> >> > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> >> > ---
> >> >
> >> >  super-intel.c |   10 ++++++++--
> >> >  1 files changed, 8 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/super-intel.c b/super-intel.c
> >> > index dc5e34e..d23267a 100644
> >> > --- a/super-intel.c
> >> > +++ b/super-intel.c
> >> > @@ -2773,8 +2773,14 @@ load_and_parse_mpb(int fd, struct intel_super
> >> *super, char *devname, int keep_fd
> >> >        int err;
> >> >
> >> >        err = load_imsm_mpb(fd, super, devname);
> >> > -       if (err)
> >> > -               return err;
> >> > +       if (err) {
> >> > +               /* try to load mpb again,
> >> > +                * in case of mdmon race we could have more luck...
> >> > +                */
> >> > +               err = load_imsm_mpb(fd, super, devname);
> >> > +               if (err)
> >> > +                       return err;
> >> > +       }
> >> >        err = load_imsm_disk(fd, super, devname, keep_fd);
> >> >        if (err)
> >> >                return err;
> >>
> >> This is semi-duplicates the check we already do after returning from
> >> load_and_parse_mpb in load_super_imsm_all.  I'm curious, are you
> >> hitting this path from load_super_imsm?  If the container is assembled
> >> we should be loading from the container, if the container is not
> >> available then mdmon can't be running and checksum errors are real.
> >>
> >
> > My test scenario is that after boot I'm disassembling read only array and immediately new array is assembled for grow continuation.
> > Sometimes occurs that mdadm throws exception and core file is generated. It shows that anchor pointer has NULL value due to CRC error.
> > Second reading try helps, and anchor is always read correctly.
> > This behavior and fact that if I put more time between array disassembling and assembling it again helps also suggest, that we have some race condition here.
> 
> Right, so we need to fix that disassemble-to-assemble race, this patch
> only "helps" :-).
> 

Yes.  We need to fix it properly.
The race should be avoided by proper use of O_EXCL.
i.e.
 1/ mdmon should only write metadata to devices while they are actually part
    of the container and so the container has exclusive access (which it
    shares with member arrays).
 2/ mdadm should only try to use a device in an array if it has O_EXCL access.

Which of these rules is broken - and where?


> > Problem is not in currently monitored in mdmon container but rather in interaction with previous mdmon session that is about to close.
> >
> > This patch makes that error condition never occurs in this scenario.
> 
> That "never" needs to be qualified.  This patch makes the race harder
> to lose, but as far as I can see not "impossible" to lose.
> 
> > Grow.c is fixed for correct error condition behavior also.
> > I can agree that both patches in this series can help for this problem separately, but I think both should be placed in code.
> 
> Why does this existing check:
> 
>                /* retry the load if we might have raced against mdmon */
>                 if (err == 3 && mdmon_running(devnum))
>                         for (retry = 0; retry < 3; retry++) {
>                                 usleep(3000);
>                                 err = load_and_parse_mpb(dfd, s, NULL, 1);
>                                 if (err != 3)
>                                         break;
>                         }

It's pretty horrible that we need to do this, isn't it?
Maybe we need some way to synchronise with mdmon so we *know* if we have
raced or not.
i.e. mdmon keeps a count of the number of times it has updated the metadata.
We send a message to get the count, read, then get the count again.
The request blocks while mdmon is actually writing.
If the counts are different, we read again.
??

NeilBrown


> 
> ...not help in this case.  I suspect due to that mdmon_running()
> qualification and the fact that the test is only seeing this in a
> disassemble-to-assemble window.  So that seems to be further evidence
> that a higher level fix is needed.
> 
> --
> Dan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html