Re: [Qemu-devel] [PATCH v8 4/7] block: introduce backup-top filter driver

[Kevin Wolf]

Am 18.06.2019 um 09:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 17.06.2019 19:25, Kevin Wolf wrote:
> > The first step would be making the block drivers full replacements of
> > the old things, which unfortunately isn't true today.
> > 
> > After your "deal with filters" series, we should be a lot closer for the
> > core infrastructure at least.
> > 
> > Not sure about copy-on-read, but I know that throttle still doesn't have
> > feature parity with -drive throttling. At least I'm pretty sure that
> > something about changing the group or group options at runtime (and not
> > just dropping the x-) was missing.
> 
> OK, thanks, I understand now that implicit filters are not just a
> feature but compatibility mechanism.
> 
> So, can we at some point deprecate "optionality" of filter-node-name
> parameters of jobs, in addition to deprecation of things like old
> copy-on-read option?  And actually deprecate implicit filters by this?

Hm, I'm not sure if we have ever moved an optional feature to required,
and how to communicate this to libvirt, but this would be ideal, yes.

> >>>>>>> But should really filter do that job, or it is here only to do CBW? 
> >>>>>>> Maybe target
> >>>>>>> must have another parent which will care about flushing.
> >>>>>>>
> >>>>>>> Ok, I think I'll flush target here too for safety, and leave a 
> >>>>>>> comment, that something
> >>>>>>> smarter would be better.
> >>>>>>> (or, if we decide to flush all children by default in generic code, 
> >>>>>>> I'll drop this handler)
> >>>>>>
> >>>>>> [1] Thinking more about this, for normal backups the target file does
> >>>>>> not reflect a valid disk state until the backup is done; just like for
> >>>>>> qemu-img convert.  Just like qemu-img convert, there is therefore no
> >>>>>> reason to flush the target until the job is done.
> >>>
> >>> Depends, the target could have the source as its backing file (like
> >>> fleecing, but without exporting it right now), and then you could always
> >>> have a consistent view on the target. Well, almost.
> >>>
> >>> Almost because to guarantee this, we'd have to flush between each CBW
> >>> and the corresponding write to the same block, to make sure that the old
> >>> data is backed up before it is overwritten.
> >>
> >> Yes, that’s what I meant by “enforce on the host that the target is
> >> always flushed before the source”.  Well, I meant to say there is no
> >> good way of doing that, and I kind of don’t consider this a good way.
> >>
> >>> Of course, this would perform about as badly as internal COW in qcow2...
> >>> So probably not a guarantee we should be making by default. But it might
> >>> make sense as an option.
> >>
> >> I don’t know.  “Use this option so your data isn’t lost in case of a
> >> power failure, but it makes everything pretty slow”?  Who is going to do
> >> explicitly enable that (before their first data loss)?
> > 
> > Maybe the backup job wasn't that clever after all. At least if you care
> > about keeping the point-in-time snapshot at the start of the backup job
> > instead of just retrying and getting a snapshot of a different point in
> > time that is just as good.
> > 
> > If you do care about the specific point in time, the safe way to do it
> > is to take a snapshot and copy that away, and then delete the snapshot
> > again.
> > 
> > The only problem is that merging external snapshots is slow and you end
> > up writing the new data twice. If only we had a COW image format... :-)
> 
> So, I still don't understand the reason of flushing backup target in a
> meantime.  Backup target remains invalid until the successful end of
> the job, at which point we definitely flush target, but only once.
> 
> If node crashes during backup, backup would be invalid independently
> of were there flushes after each write (or better just enable
> O_DIRECT) or not.

Say your VM is running on disk.qcow2 and you use a backup job to copy
data to backup.qcow2. At some point, the host crashes. If we made sure
that every CBW to backup.qcow2 has completed before we write new data to
disk.qcow2, backup.qcow2 contains valid data that represents the state
at the start of the backup job as long as you use disk.qcow2 as its
backing file.

The only way to ensure the right order is flushing between the two
operations. If you don't do that, then yes, your backing is invalid
before it has completed.

O_DIRECT doesn't guarantee that the data is on disk, only a flush does
that. Maybe your chances that things actually make it to the disk in
case of a host crash are higher if it sits in the disk cache rather than
in the host's RAM, but there is no guarantee without a flush.

Now flushing the target when the guest flushes its disk doesn't give you
new guarantees. Maybe it increases your chances that you're lucky and
your data is correct, but you won't be able to tell. So... not really a
reason not to do it, but it's probably kind of useless.

Kevin