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@xxxxxxxxxx> > > > > > --- > > > > > 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, §ionLen) < 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@xxxxxxxxxx> Don't forget to send the final version to the list though.
