Re: [Qemu-devel] [PATCH v4 2/8] block: add basic backup support to block driver

[Kevin Wolf]

Am 22.05.2013 um 15:58 hat Stefan Hajnoczi geschrieben:
> On Wed, May 22, 2013 at 11:56:45AM +0200, Kevin Wolf wrote:
> > Am 22.05.2013 um 11:54 hat Paolo Bonzini geschrieben:
> > > Il 22/05/2013 11:38, Kevin Wolf ha scritto:
> > > >> +
> > > >> +        DPRINTF("brdv_co_backup_cow done C%" PRId64 "\n", start);
> > > >> +    }
> > > >> +
> > > >> +out:
> > > >> +    if (bounce_buffer) {
> > > >> +        qemu_vfree(bounce_buffer);
> > > >> +    }
> > > >> +
> > > >> +    cow_request_end(&cow_request);
> > > >> +
> > > >> +    qemu_co_rwlock_unlock(&job->flush_rwlock);
> > > >> +
> > > >> +    return ret;
> > > >> +}
> > > >> +
> > > >> +static void coroutine_fn backup_before_write_notify(Notifier 
> > > >> *notifier,
> > > >> +                                                    void *opaque)
> > > >> +{
> > > >> +    BdrvTrackedRequest *req = opaque;
> > > >> +    backup_do_cow(req->bs, req->sector_num, req->nb_sectors);
> > > >> +}
> > > > 
> > > > I don't think you can ignore errors here. Not sure if we can stop the VM
> > > > and resume later or something like that, but if we can't, the backup
> > > > will be invalid and we must fail the job.
> > > 
> > > Yes, there is rerror/werror machinery for jobs that this patch is not 
> > > using.
> > 
> > This is not enough here. The guest write can't continue before the old
> > content is saved to the backup image.
> 
> Are you saying just the vm_stop(RUN_STATE_IO_ERROR) call is missing?

No. Stopping the VM and on 'cont' assuming that the request was
successful (which it wasn't) would be wrong as well. You need the full
failed request handling, with restarting the request from the device
emulation and everything.

> I think the reason it's not there is because there's an assumption that
> block jobs are in the background and do not affect the guest.  The guest
> continues running while the block job is in an error state.

But this is _not_ a background job. This is the write request hook, it
is active on the guest write path. If you can't backup a given sector,
you can't overwrite it and must either fail the write request or the
backup job. Failing the write request is probably nicer because you get
the usual werror handling then, but it means that your assumption
doesn't hold true.

(See, this is one of the reasons why I was for a BlockDriver instead of
notifiers into block jobs. Things would be clearer that way because the
control flow would be explicit in the filter code.)

> But perhaps we can make the vm_stop() call optional so that drive-backup
> can use it.

Not sure what you're trying to do here.

Kevin