Re: 3.4.0-02580-g72c04af regression on sparc64 - partitions not recognized

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Thu, 2012-05-24 at 00:54 -0700, Dan Williams wrote:
> On Thu, May 24, 2012 at 12:42 AM, James Bottomley
> <jejbbe@hansenpartnership.com> wrote:
> > On Wed, 2012-05-23 at 19:22 -0700, Dan Williams wrote:
> >> On Wed, 2012-05-23 at 23:56 +0100, James Bottomley wrote:
> >> > On Wed, 2012-05-23 at 14:04 -0400, David Miller wrote:
> >> > > From: Meelis Roos <mroos@linux.ee>
> >> > > Date: Wed, 23 May 2012 19:46:46 +0300 (EEST)
> >> > >
> >> > > CC:'ing interested parties.
> >> > >
> >> > > >> > Just tested 3.4.0-02580-g72c04af on about 10 machines. While most of
> >> > > >> > them work (including 3 different sparc64 machines with real scsi disks),
> >> > > >> > Sun Netra X1 with pata_ali and IDE disk consistently fails to boot. sda
> >> > > >> > is recognized but no partitions. 3.3.0 works fine, as did something
> >> > > >> > around 3.4-rc7 (plain 3.4 not tested yet). No other IDE machines tested
> >> > > >> > yet since I have none with remote console at the moment.
> >> > > >>
> >> > > >> If 3.4.0-final is OK, start bisecting from v3.4.0 until 72c04af.  One
> >> > > >> possibility could be the sparc64 NOBOOTMEM conversion that went into
> >> > > >> the merge window.
> >> > > >
> >> > > > Bisecting leads to this commit:
> >> > > >
> >> > > > a7a20d103994fd760766e6c9d494daa569cbfe06 is the first bad commit
> >> > > > commit a7a20d103994fd760766e6c9d494daa569cbfe06
> >> > > > Author: Dan Williams <dan.j.williams@intel.com>
> >> > > > Date:   Thu Mar 22 17:05:11 2012 -0700
> >> > > >
> >> > > >     [SCSI] sd: limit the scope of the async probe domain
> >> >
> >> > My theory is that this is an init problem: The assumption in a lot of
> >> > our code is that async_synchronize_full() waits for everything ... even
> >> > the domain specific async schedules, which isn't true.
> >> >
> >> > The code in init that makes this assumption is wait_for_device_probe().
> >> > There's also a fun async_synchronize_full() in init_post() that assumes
> >> > it can free the init memory after, which would fail badly if anything in
> >> > init used an async domain.
> >> >
> >> > So either we fix the assumptions or we can't use domain specific async
> >> > schedules.
> >> >
> >>
> >> Hm, we already have cases of code not trusting the semantics of
> >> wait_for_device_probe(), especially as it relates to async scanning like
> >> in kernel/power/hibernate.c:
> >>
> >>                 /*
> >>                  * Some device discovery might still be in progress; we need
> >>                  * to wait for this to finish.
> >>                  */
> >>                 wait_for_device_probe();
> >>
> >>                 if (resume_wait) {
> >>                         while ((swsusp_resume_device = name_to_dev_t(resume_file)) == 0)
> >>                                 msleep(10);
> >>                         async_synchronize_full();
> >>                 }
> >>
> >>                 /*
> >>                  * We can't depend on SCSI devices being available after loading
> >>                  * one of their modules until scsi_complete_async_scans() is
> >>                  * called and the resume device usually is a SCSI one.
> >>                  */
> >>                 scsi_complete_async_scans();
> >
> > This is actually looks wrong: it works if SCSI is built in, but it's a
> > nop if SCSI is a module (the nop function is gated by the else clause of
> > #ifdef CONFIG_SCSI)
> >
> > Rafael, you added this not via the SCSI tree, is that the intention?
> >
> >> ...so it seems scsi_complete_async_scans() should take care to flush sd
> >> probe actions as well... here is a test patch:
> >>
> >> --- snip ---
> >>
> >> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> >> index 8906557..05a92d3 100644
> >> --- a/drivers/scsi/scsi_scan.c
> >> +++ b/drivers/scsi/scsi_scan.c
> >> @@ -141,13 +141,13 @@ struct async_scan_data {
> >>   * started scanning after this function was called may or may not have
> >>   * finished.
> >>   */
> >> -int scsi_complete_async_scans(void)
> >> +static void __scsi_complete_async_scans(void)
> >>  {
> >>         struct async_scan_data *data;
> >>
> >>         do {
> >>                 if (list_empty(&scanning_hosts))
> >> -                       return 0;
> >> +                       return;
> >>                 /* If we can't get memory immediately, that's OK.  Just
> >>                  * sleep a little.  Even if we never get memory, the async
> >>                  * scans will finish eventually.
> >> @@ -181,6 +181,13 @@ int scsi_complete_async_scans(void)
> >>         spin_unlock(&async_scan_lock);
> >>
> >>         kfree(data);
> >> +}
> >> +
> >> +int scsi_complete_async_scans(void)
> >> +{
> >> +       __scsi_complete_async_scans();
> >> +       async_synchronize_full_domain(&scsi_sd_probe_domain);
> >> +
> >>         return 0;
> >>  }
> >
> > But this still doesn't fix the boot problem, does it? ... unless we want
> > to add a scsi_complete_async_scans() into init/do_mounts.c, which looks
> > like piling one hack on top of another.
> 
> I managed to convince myself that one in prepare_namespace() was
> probably not needed because of the late_initcall of
> scsi_complete_async_scans() in the built-in case and in the module
> case the initramfs should be taking care of it.

Ah, now I remember, that was the hack we put in to prevent a boot panic
caused by a similar thing ... mainly because async SCSI scanning uses a
custom infrastructure.

> > I really think the correct fix is to have wait_for_device_probe()
> > actually wait until all probes have completed and everything is
> > discovered, that way we get the semantics the name implies and boot
> > should just work.
> >
> 
> ...but wouldn't it need to go something like:
> 
> wait_for_device_probe(); /* all pci drivers probed */
> scsi_complete_async_scans(): /* flush host scans */
> wait_for_device_probe(); /* all recently attached sd devices probed */
> 
> ?

No .. the point is if we wait for all outstanding scans instead of just
those in the async_running domain, it will "just work", with no special
SCSI magic.

James