* Don Slutz (dsl...@verizon.com) wrote:
> On 11/21/14 05:49, Dr. David Alan Gilbert wrote:
> >* Markus Armbruster (arm...@redhat.com) wrote:
> >>Don Slutz<dsl...@verizon.com> writes:
> >>
> >>>On 11/19/14 07:29, Markus Armbruster wrote:
> >>>>Don Slutz<dsl...@verizon.com> writes:
> >>>>
> >>>>>The other callers to blk_set_enable_write_cache() in this file
> >>>>>already check for s->blk == NULL.
> >>>>>
> >>>>>Signed-off-by: Don Slutz<dsl...@verizon.com>
> >>>>>---
> >>>>>
> >>>>>I think this is a bugfix that should be back ported to stable
> >>>>>releases.
> >>>>>
> >>>>>I also think this should be done in xen's copy of QEMU for 4.5 with
> >>>>>back port(s) to active stable releases.
> >>>>>
> >>>>>Note: In 2.1 and earlier the routine is
> >>>>>bdrv_set_enable_write_cache(); variable is s->bs.
> >>>>Got a reproducer?
> >>>yes. Migrating a guest from xen 4.2 or 4.3 to xen 4.4 (or 4.5-unstable) on
> >>>CentOS 6.3 with xen_emul_unplug=unnecessary and no cdrom defined.
> >>>
> >>>
> >>>>I'm asking because I believe s->identify_set implies s->blk.
> >>>>s->identify_set is initialized to zero, and gets set to non-zero exactly
> >>>>on the first successful IDENTIFY DEVICE or IDENTIFY PACKET DEVICE, in
> >>>>ide_identify(), ide_atapi_identify() or ide_cfata_identify(),
> >>>>respectively. Only called via cmd_identify() / cmd_identify_packet()
> >>>>via ide_exec_cmd(). The latter immediately fails when !s->blk:
> >>>>
> >>>> s = idebus_active_if(bus);
> >>>> /* ignore commands to non existent slave */
> >>>> if (s != bus->ifs && !s->blk) {
> >>>> return;
> >>>> }
> >>>I do think that you are right. I have now spent more time on why I am
> >>>seeing this.
> >>>
> >>>
> >>>>Even if I'm right, your patch is fine, because it makes this spot more
> >>>>obviously correct, and consistent with the other uses of
> >>>>blk_set_enable_write_cache(). The case for stable is weak, though.
> >>>>
> >>>I had not fully tracked down what is happening before sending the bugfix.
> >>>I have now done more debugging, and have tracked it down to xen 4.4
> >>>now using "-nodefaults" with QEMU.
> >>>
> >>>I needed to add output to QEMU to track this down because I have long
> >>>command lines...
> >>>
> >>>(all I get for ps -ef):
> >>[...]
> >>>Which is missing that option.
> >>>
> >>>The ide that was aborting in this case is the cdrom at hdc that is added
> >>>if you do not specify "-nodefaults".
> >>>
> >>>Since this is a "changed" machine config, I am no longer as sure as what
> >>>versions this needs to be in.
> >>>
> >>>If I put my QEMU hat on, it does not look like a back port is needed.
> >>>However
> >>>for xen it would be nice.
> >>>
> >>>I do not know how the QEMU community feels about migration from a config
> >>>without "-nodefaults" to one with "-nodefaults" as the only difference.
> >>So you have a CD-ROM on the source, but not on the destination?
>
> Yes, QEMU in the source has an empty CD-ROM, but not on the destination.
> xen
> does not know that QEMU added a CD-ROM drive in hdc by default.
>
> >>That can't work. I guess it broke for you in an unusual way (target
> >>crashes) rather than the usual way (target rejects migration data for a
> >>device it doesn't have) due to our convoluted IDE data structures. With
> >>your patch applied it should break the usual way. Does it?
>
> Nope. It does not break at all. The migration "works". The target accepts
> the hdc data.
> It looks like to me that all 4 IDE state data is sent.
>
>
> >>Management tools should use -nodefaults. But if it mixes default and
> >>-nodefaults in migration, recreating the stuff it got by default but
> >>doesn't get with -nodefaults is its own responsibility.
>
> Yes. Since xen did not ask for a CD-ROM, it did not expect to need to create
> it. xen was "fixed" to use -nodefaults.
>
> >Well, mostly - we wouldn't expect a migration to work if the source/dest
> >didn't match exactly; but QEMU shouldn't seg.
>
> Well, this change prevents a seg. So you are in favor of having this
> backported?
Yes.
Dave
>
>
> -Don Slutz
>
> >Dave
> >
> >--
> >Dr. David Alan Gilbert /dgilb...@redhat.com / Manchester, UK
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
