[PATCH v2 13/17] virfdstream: Allow sparse stream vol-download

[Peter Krempa]

On Thu, Aug 20, 2020 at 15:31:28 +0200, Michal Privoznik wrote:
> On 8/20/20 1:57 PM, Peter Krempa wrote:
> > On Tue, Jul 07, 2020 at 21:46:31 +0200, Michal Privoznik wrote:
> > > When handling sparse stream, a thread is executed. This thread
> > > runs a read() or write() loop (depending what API is called; in
> > > this case it's virStorageVolDownload() and  this the thread run
> > > read() loop). The read() is handled in virFDStreamThreadDoRead()
> > > which is then data/hole section aware, meaning it uses
> > > virFileInData() to detect data and hole sections and sends
> > > TYPE_DATA or TYPE_HOLE virStream messages accordingly.
> > > 
> > > However, virFileInData() does not work with block devices. Simply
> > > because block devices don't have data and hole sections. But we
> > > can use new virFileInDataDetectZeroes() which is block device
> > > friendly for that.
> > > 
> > > Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1852528
> > > 
> > > Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> > > ---
> > >   src/util/virfdstream.c | 15 ++++++++++++---
> > >   1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > IMO this goes against the semantics of the _SPARSE_STREAM flag. A block
> > device by definition is not sparse, so there are no holes to send.
> > 
> > What you've implemented is a way to sparsify a block device, but that
> > IMO should not be considered by default when a block device is used.
> > If a file is not sparse, the previous code doesn't actually transmit
> > holes either.
> > 
> > If you want to achieve sparsification on the source side of the
> > transmission, this IMO needs an explicit flag to opt-in and then we
> > should sparsify also regular files using the same algorithm.
> > 
> 
> Fair enough. So how about I'll send v3 where:
> 
> a) in the first patches I make our stream read/write functions handle block
> devices for _SPARSE_STREAM without any zero block detection. Only thing that
> will happen is that if the source is a sparse regular file and thus the
> stream receiver gets a HOLE packet and it is writing the data into a block
> device it will have to emulate the hole by writing block of zeroes. However,
> if the stream source is a block device then no HOLE shall ever be sent.

AFAIK I've R-b'd enough patches to fix this portion and provided that
there aren't any merge conflicts you can already commit those.

I'm completely fine with that portion as-is.

> b) in next patches I'll introduce _DETECT_ZEROES flag (and possibly make it
> require _SPARSE_STREAM too) which will handle the case where the stream
> source is a block device with zero blocks, at which point it will try to
> detect them and be allowed to send HOLE down the stream.

On this topic, I agree that it's a sensible approach for the rest of the
series and it at least unifies the behaviour.

I'm unsure though whether it's worth even doing _DETECT_ZEROES feature
at all though. To me it feels that the users are better off using other
tools rather than re-implementing yet another thing in libvirt.

If possible provide some additional justification here.