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

[Peter Krempa]

On Fri, Aug 21, 2020 at 11:19:40 +0200, Michal Privoznik wrote:
> On 8/20/20 3:42 PM, Peter Krempa wrote:
> > 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.
> 
> Almost :-)
> For instance this very patch uses virFileInDataDetectZeroes() to detect zero
> blocks on block devices. It needs to be changed to always assume data
> section and some length. The same applies to the next patch 14/17.
> But the diff is trivial:
> 
> iff --git c/src/util/virfdstream.c w/src/util/virfdstream.c
> index 9968cdc623..39514ef555 100644
> --- c/src/util/virfdstream.c
> +++ w/src/util/virfdstream.c
> @@ -440,8 +440,15 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
> 
>      if (sparse && *dataLen == 0) {
>          if (isBlock) {
> -            if (virFileInDataDetectZeroes(fdin, &inData, &sectionLen) < 0)
> -                return -1;
> +            /* Block devices are always in data section by definition. The
> +             * @sectionLen is slightly more tricky. While we could try and
> get
> +             * how much bytes is there left until EOF, we can pretend there
> is
> +             * always X bytes left and let the saferead() below hit EOF
> (which
> +             * is then handled gracefully anyway). Worst case scenario,
> this
> +             * branch is called more than once.
> +             * X was chosen to be 1MiB but it has ho special meaning. */
> +            inData = 1;
> +            sectionLen = 1 * 1024 * 1024;
> 
> And the same for virsh case. Do you want me to resend those two patches?

Well, for a substantial change like this it should be the default
approach.

You can use:

Reviewed-by: Peter Krempa <pkrempa at redhat.com>

Don't forget to send the final version to the list though.