Re: [PATCH v2 13/17] virfdstream: Allow sparse stream vol-download
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
> > > > > ---
> > > > >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, , ) < 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
Don't forget to send the final version to the list though.
Re: [PATCH v2 13/17] virfdstream: Allow sparse stream vol-download
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
---
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, , ) < 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?
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.
Alright. Fair enough I guess.
If possible provide some additional justification here.
It was discussed in the bz
https://bugzilla.redhat.com/show_bug.cgi?id=1852528
VDSM is doing a thin provisioning and as a part of that they are copying
files onto block devices. But for that zero detection shouldn't be needed.
Michal
Re: [PATCH v2 13/17] virfdstream: Allow sparse stream vol-download
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 > > > --- > > > 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.
Re: [PATCH v2 13/17] virfdstream: Allow sparse stream vol-download
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 --- 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. 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. Does this sound reasonable? BTW: I've pushed the first N patches which were just switching FDStream to glib. Michal
Re: [PATCH v2 13/17] virfdstream: Allow sparse stream vol-download
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 > --- > 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.
[PATCH v2 13/17] virfdstream: Allow sparse stream vol-download
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
---
src/util/virfdstream.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index 50209a8bd4..44b4855549 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -398,6 +398,7 @@ struct _virFDStreamThreadData {
size_t length;
bool doRead;
bool sparse;
+bool isBlock;
int fdin;
char *fdinname;
int fdout;
@@ -421,6 +422,7 @@ virFDStreamThreadDataFree(virFDStreamThreadDataPtr data)
static ssize_t
virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
bool sparse,
+bool isBlock,
const int fdin,
const int fdout,
const char *fdinname,
@@ -437,8 +439,13 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
ssize_t got;
if (sparse && *dataLen == 0) {
-if (virFileInData(fdin, , ) < 0)
-return -1;
+if (isBlock) {
+if (virFileInDataDetectZeroes(fdin, , ) < 0)
+return -1;
+} else {
+if (virFileInData(fdin, , ) < 0)
+return -1;
+}
if (length &&
sectionLen > length - total)
@@ -568,6 +575,7 @@ virFDStreamThread(void *opaque)
virStreamPtr st = data->st;
size_t length = data->length;
bool sparse = data->sparse;
+bool isBlock = data->isBlock;
VIR_AUTOCLOSE fdin = data->fdin;
char *fdinname = data->fdinname;
VIR_AUTOCLOSE fdout = data->fdout;
@@ -604,7 +612,7 @@ virFDStreamThread(void *opaque)
}
if (doRead)
-got = virFDStreamThreadDoRead(fdst, sparse,
+got = virFDStreamThreadDoRead(fdst, sparse, isBlock,
fdin, fdout,
fdinname, fdoutname,
length, total,
@@ -1271,6 +1279,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
threadData->st = virObjectRef(st);
threadData->length = length;
threadData->sparse = sparse;
+threadData->isBlock = !!S_ISBLK(sb.st_mode);
if ((oflags & O_ACCMODE) == O_RDONLY) {
threadData->fdin = fd;
--
2.26.2
