[PATCH v2 16/17] stream: Don't read block device twice
Michal Privoznik
mprivozn at redhat.com
Tue Jul 7 19:46:34 UTC 2020
The aim of virFileInDataDetectZeroes() is to mimic virFileInData()
so that it can be used in sparse virStream with very little change
to logic. This was implemented in previous commits. These two
functions act alike - the passed FD is rewound back to the position
it was in when either of the functions was called. For regular
files (when virFileInData()) is used, this is not a problem -
rewinding an FD is usually cheap and data is read() only from data
section and it's read only once. But this is not the case for
virFileInDataDetectZeroes(), which doesn't use cheap lseek() to
learn data/hole sections, but uses read() and zero buffer
detection. Worst case scenario, virFileInDataDetectZeroes() reads
1MiB of bytes, finds it is non-zero data, rewinds FD that 1MiB back
only so that virFDStreamThreadDoRead() (in case of daemon) or
virshStreamSource() (in case of client) can read the same data
again.
Possibly, this is not that big problem because kernel is likely to
keep the data still in cache. But let's not rely on that and store
the buffer for later use if we learned it contains actual data.
Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
---
src/util/virfdstream.c | 27 +++++++++++++---------
src/util/virfile.c | 51 ++++++++++++++----------------------------
src/util/virfile.h | 3 ++-
tools/virsh-util.c | 40 +++++++++++++++++++++++++++++----
tools/virsh-util.h | 3 +++
tools/virsh-volume.c | 3 ++-
6 files changed, 76 insertions(+), 51 deletions(-)
diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index 40825a8811..9d617c71da 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -440,7 +440,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
if (sparse && *dataLen == 0) {
if (isBlock) {
- if (virFileInDataDetectZeroes(fdin, &inData, §ionLen) < 0)
+ if (virFileInDataDetectZeroes(fdin, &inData, §ionLen, &buf) < 0)
return -1;
} else {
if (virFileInData(fdin, &inData, §ionLen) < 0)
@@ -468,7 +468,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
/* HACK: The message queue is one directional. So caller
* cannot make us skip the hole. Do that for them instead. */
- if (sectionLen &&
+ if (sectionLen && !isBlock &&
lseek(fdin, sectionLen, SEEK_CUR) == (off_t) -1) {
virReportSystemError(errno,
_("unable to seek in %s"),
@@ -476,17 +476,22 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
return -1;
}
} else {
- if (sparse &&
- buflen > *dataLen)
- buflen = *dataLen;
+ if (sparse && isBlock) {
+ /* Data already read by virFileInDataDetectZeroes() */
+ got = sectionLen;
+ } else {
+ if (sparse &&
+ buflen > *dataLen)
+ buflen = *dataLen;
- buf = g_new0(char, buflen);
+ buf = g_new0(char, buflen);
- if ((got = saferead(fdin, buf, buflen)) < 0) {
- virReportSystemError(errno,
- _("Unable to read %s"),
- fdinname);
- return -1;
+ if ((got = saferead(fdin, buf, buflen)) < 0) {
+ virReportSystemError(errno,
+ _("Unable to read %s"),
+ fdinname);
+ return -1;
+ }
}
msg->type = VIR_FDSTREAM_MSG_TYPE_DATA;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index a35d9ccb7a..e33a1c280c 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -4071,12 +4071,18 @@ virFileInData(int fd G_GNUC_UNUSED,
* @fd: file to check
* @inData: true if current position in the @fd is in data section
* @length: amount of bytes until the end of the current section
+ * @buf: data read if in data section
*
- * This behaves exactly like virFileInData() except it doesn't use SEEK_DATA
- * and SEEK_HOLE rather than a buffer into which it reads data from @fd and
- * detects if the buffer is full of zeroes. Therefore, it is safe to use on
- * special files (e.g. block devices). On the other hand it sees only
- * DETECT_ZEORES_BLOCK_SIZE ahead.
+ * For given @fd it reads up to DETECT_ZEORES_BLOCK_SIZE bytes from it.
+ * If all bytes read are zero then @inData is set to 0, otherwise @inData
+ * is set to 1 and @buf is set to the read data. In either case, the
+ * number of bytes read is stored in @length.
+ *
+ * If @fd is at EOF, then this function sets @inData = 0 and @length = 0.
+ *
+ * Note, in contrast to virFileInData(), this function is safe to use on
+ * special files (e.g. block devices), does not rewind @fd back to its
+ * starting position, and sees only DETECT_ZEORES_BLOCK_SIZE bytes ahead.
*
* Returns: 0 on success,
* -1 otherwise.
@@ -4084,50 +4090,27 @@ virFileInData(int fd G_GNUC_UNUSED,
int
virFileInDataDetectZeroes(int fd,
int *inData,
- long long *length)
+ long long *length,
+ char **buf)
{
const size_t buflen = DETECT_ZEORES_BLOCK_SIZE;
g_autofree char *bytes = NULL;
- off_t cur;
ssize_t r;
- int ret = -1;
-
- /* Get current position */
- cur = lseek(fd, 0, SEEK_CUR);
- if (cur == (off_t) -1) {
- virReportSystemError(errno, "%s",
- _("Unable to get current position in file"));
- goto cleanup;
- }
bytes = g_new0(char, buflen);
if ((r = saferead(fd, bytes, buflen)) < 0) {
virReportSystemError(errno, "%s",
_("Unable to read from file"));
- goto cleanup;
+ return -1;
}
*inData = !virStringIsNull(bytes, r);
*length = r;
- ret = 0;
+ if (*inData)
+ *buf = g_steal_pointer(&bytes);
- cleanup:
- /* At any rate, reposition back to where we started. */
- if (cur != (off_t) -1) {
- int theerrno = errno;
-
- if (lseek(fd, cur, SEEK_SET) == (off_t) -1) {
- virReportSystemError(errno, "%s",
- _("unable to restore position in file"));
- ret = -1;
- if (theerrno == 0)
- theerrno = errno;
- }
-
- errno = theerrno;
- }
- return ret;
+ return 0;
}
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 9a5eade609..06ee0bd801 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -354,7 +354,8 @@ int virFileInData(int fd,
int virFileInDataDetectZeroes(int fd,
int *inData,
- long long *length);
+ long long *length,
+ char **buf);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virFileWrapperFd, virFileWrapperFdFree);
diff --git a/tools/virsh-util.c b/tools/virsh-util.c
index 867f69577f..dd2f4fb6a3 100644
--- a/tools/virsh-util.c
+++ b/tools/virsh-util.c
@@ -160,8 +160,25 @@ virshStreamSource(virStreamPtr st G_GNUC_UNUSED,
{
virshStreamCallbackDataPtr cbData = opaque;
int fd = cbData->fd;
+ int r;
- return saferead(fd, bytes, nbytes);
+ if (cbData->isBlock) {
+ size_t bufavail = cbData->buflen - cbData->bufoff;
+
+ r = MIN(nbytes, bufavail);
+ memcpy(bytes, cbData->buf + cbData->bufoff, r);
+ cbData->bufoff += r;
+
+ if (cbData->bufoff == cbData->buflen) {
+ VIR_FREE(cbData->buf);
+ cbData->buflen = 0;
+ cbData->bufoff = 0;
+ }
+ } else {
+ r = saferead(fd, bytes, nbytes);
+ }
+
+ return r;
}
@@ -172,9 +189,11 @@ virshStreamSourceSkip(virStreamPtr st G_GNUC_UNUSED,
{
virshStreamCallbackDataPtr cbData = opaque;
int fd = cbData->fd;
- off_t cur;
- if ((cur = lseek(fd, offset, SEEK_CUR)) == (off_t) -1)
+ /* Don't seek if the @fd is a block device. It was left at
+ * the desired position by virshStreamInData(). */
+ if (!cbData->isBlock &&
+ (lseek(fd, offset, SEEK_CUR)) == (off_t) -1)
return -1;
return 0;
@@ -232,10 +251,23 @@ virshStreamInData(virStreamPtr st G_GNUC_UNUSED,
int fd = cbData->fd;
if (cbData->isBlock) {
- if (virFileInDataDetectZeroes(fd, inData, offset) < 0) {
+ g_autofree char *buf = NULL;
+
+ if (virFileInDataDetectZeroes(fd, inData, offset, &buf) < 0) {
vshError(ctl, "%s", _("Unable to get current position in stream"));
return -1;
}
+
+ if (*inData) {
+ cbData->buf = g_steal_pointer(&buf);
+ cbData->buflen = *offset;
+ cbData->bufoff = 0;
+ }
+
+ /* Intentionally leaving @fd in a different position than on entering
+ * because either we've found a hole and virshStreamSourceSkip() would
+ * want to seek forward, or we've read some data and would need to
+ * imitate seek in virshStreamSource(). Skip seeking back and forth. */
} else {
if (virFileInData(fd, inData, offset) < 0) {
vshError(ctl, "%s", _("Unable to get current position in stream"));
diff --git a/tools/virsh-util.h b/tools/virsh-util.h
index 9ef28cfe0a..b2b5d6bcd2 100644
--- a/tools/virsh-util.h
+++ b/tools/virsh-util.h
@@ -73,6 +73,9 @@ struct _virshStreamCallbackData {
vshControl *ctl;
int fd;
bool isBlock;
+ char *buf;
+ size_t buflen;
+ long long bufoff;
};
int
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index 20823e2d10..9fd1674a9f 100644
--- a/tools/virsh-volume.c
+++ b/tools/virsh-volume.c
@@ -678,7 +678,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
unsigned long long offset = 0, length = 0;
virshControlPtr priv = ctl->privData;
unsigned int flags = 0;
- virshStreamCallbackData cbData;
+ virshStreamCallbackData cbData = { 0 };
struct stat sb;
if (vshCommandOptULongLong(ctl, cmd, "offset", &offset) < 0)
@@ -752,6 +752,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
virStorageVolFree(vol);
if (st)
virStreamFree(st);
+ VIR_FREE(cbData.buf);
VIR_FORCE_CLOSE(fd);
return ret;
}
--
2.26.2
More information about the libvir-list
mailing list