Re: [Qemu-devel] [PATCH 3/3] disk: don't read from disk until the guest starts

[Anthony Liguori]

On 09/11/2010 12:24 PM, Stefan Hajnoczi wrote:
> On Sat, Sep 11, 2010 at 3:04 PM, Anthony Liguori<address@hidden>  wrote:
>    
> > This fixes a couple nasty problems relating to live migration.
> >
> > 1) When dealing with shared storage with weak coherence (i.e. NFS), even if
> >    we re-read, we may end up with undesired caching.  By delaying any reads
> >    until we absolutely have to, we decrease the likelihood of any undesirable
> >    caching.
> >
> > 2) When dealing with copy-on-read, the local storage acts as a cache.  We need
> >    to make sure to avoid any reads to avoid polluting the local cache.
> >
> > Signed-off-by: Anthony Liguori<address@hidden>
> >
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 1e466d1..57d8db3 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -105,6 +132,8 @@ static void ide_identify(IDEState *s)
> >         return;
> >      }
> >
> > +    guess_geometry(s);
> > +
> >      
> Does the same change need to be made in ide_cfata_identify()?
>
> I quickly checked the VMStateDescription and don't see cylinders,
> heads, sectors being saved for migration.  I am concerned that IDE
> will break after migration if the following happens:
> 1. Guest resumes and does not issue ATA IDENTIFY so cylinders, heads,
> sectors are not initialized.
> 2. Normal I/O is performed, invoking ide_get_sector() which uses
> geometry information that has not been initialized.
>
> Did I miss something?
>    

Nope, I just missed some places that need calls.  I've renamed 
guess_geometry() to init_geometry() and added calls to 
ide_cfata_identify(), ide_get_sector(), and ide_set_sector().

That should cover it all.

> > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> > index bd6bbe6..0bf17ec 100644
> > --- a/hw/virtio-blk.c
> > +++ b/hw/virtio-blk.c
> > @@ -427,6 +427,10 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
> > uint8_t *config)
> >
> >      bdrv_get_geometry(s->bs,&capacity);
> >      bdrv_get_geometry_hint(s->bs,&cylinders,&heads,&secs);
> > +    if (cylinders == 0) {
> > +        bdrv_guess_geometry(s->bs,&cylinders,&heads,&secs);
> > +    }
> > +
> >      
> bdrv_guess_geometry() can be called unconditionally.  The call to
> bdrv_get_geometry_hint() can be eliminated.  bdrv_guess_geometry()
> updates the geometry hint and does not probe the boot sector after the
> first time.
>    

Yeah, that's a fair point.

Regards,

Anthony Liguori

> Stefan
>
>