[libvirt] [PATCH 0/3] storage: zfs: implement download and upload
Roman Bogorodskiy (3): fdstream: report error if virSetNonBlock fails fdstream: introduce virFDStreamOpenBlockDevice storage: zfs: implement download and upload src/fdstream.c | 30 +++++++++++++++++++++++------- src/fdstream.h | 5 +++++ src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 6 ++++-- src/storage/storage_backend_zfs.c | 2 ++ 5 files changed, 35 insertions(+), 9 deletions(-) -- 1.9.0
virFDStreamOpenInternal terminates if virSetNonBlock fails. As virSetNonBlock uses gnulib's set_nonblocking_flag that sets errno, call virReportSystemError() to let user know the reason of fail. --- src/fdstream.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/fdstream.c b/src/fdstream.c index fd576ef..d236318 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -478,8 +478,10 @@ static int virFDStreamOpenInternal(virStreamPtr st, st, fd, cmd, errfd, length); if ((st->flags & VIR_STREAM_NONBLOCK) && - virSetNonBlock(fd) < 0) + virSetNonBlock(fd) < 0) { + virReportSystemError(errno, "%s", _("Unable to set non-blocking mode")); return -1; + } if (VIR_ALLOC(fdst) < 0) return -1; -- 1.9.0
virStorageBackendVolDownloadLocal and virStorageBackendVolUploadLocal use virFDStreamOpenFile function to work with the volume fd. virFDStreamOpenFile calls virFDStreamOpenFileInternal that implements handling of the non-blocking I/O. If a file is not a character device and not a fifo, it uses libvirt_iohelper. On FreeBSD, it doesn't work as expected because disks are exposed as character devices. To overcome this, introduce a forceIOHelper flag to virFDStreamOpenFileInternal that forces using libvirt_iohelper. And introduce virFDStreamOpenBlockDevice that calls virFDStreamOpenFileInternal with the forceIOHelper set to true. --- src/fdstream.c | 26 ++++++++++++++++++++------ src/fdstream.h | 5 +++++ src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 6 ++++-- 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index d236318..4cf152a 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -577,7 +577,8 @@ virFDStreamOpenFileInternal(virStreamPtr st, unsigned long long offset, unsigned long long length, int oflags, - int mode) + int mode, + bool forceIOHelper) { int fd = -1; int childfd = -1; @@ -623,8 +624,8 @@ virFDStreamOpenFileInternal(virStreamPtr st, * the I/O so we just have a fifo. Or use AIO :-( */ if ((st->flags & VIR_STREAM_NONBLOCK) && - (!S_ISCHR(sb.st_mode) && - !S_ISFIFO(sb.st_mode))) { + ((!S_ISCHR(sb.st_mode) && + !S_ISFIFO(sb.st_mode)) || forceIOHelper)) { int fds[2] = { -1, -1 }; if ((oflags & O_ACCMODE) == O_RDWR) { @@ -703,7 +704,7 @@ int virFDStreamOpenFile(virStreamPtr st, } return virFDStreamOpenFileInternal(st, path, offset, length, - oflags, 0); + oflags, 0, false); } int virFDStreamCreateFile(virStreamPtr st, @@ -715,7 +716,8 @@ int virFDStreamCreateFile(virStreamPtr st, { return virFDStreamOpenFileInternal(st, path, offset, length, - oflags | O_CREAT, mode); + oflags | O_CREAT, mode, + false); } #ifdef HAVE_CFMAKERAW @@ -730,7 +732,8 @@ int virFDStreamOpenPTY(virStreamPtr st, if (virFDStreamOpenFileInternal(st, path, offset, length, - oflags | O_CREAT, 0) < 0) + oflags | O_CREAT, 0, + false) < 0) return -1; fdst = st->privateData; @@ -770,6 +773,17 @@ int virFDStreamOpenPTY(virStreamPtr st, } #endif /* !HAVE_CFMAKERAW */ +int virFDStreamOpenBlockDevice(virStreamPtr st, + const char *path, + unsigned long long offset, + unsigned long long length, + int oflags) +{ + return virFDStreamOpenFileInternal(st, path, + offset, length, + oflags, 0, true); +} + int virFDStreamSetInternalCloseCb(virStreamPtr st, virFDStreamInternalCloseCb cb, void *opaque, diff --git a/src/fdstream.h b/src/fdstream.h index 69d8328..2c913ea 100644 --- a/src/fdstream.h +++ b/src/fdstream.h @@ -56,6 +56,11 @@ int virFDStreamOpenPTY(virStreamPtr st, unsigned long long offset, unsigned long long length, int oflags); +int virFDStreamOpenBlockDevice(virStreamPtr st, + const char *path, + unsigned long long offset, + unsigned long long length, + int oflags); int virFDStreamSetInternalCloseCb(virStreamPtr st, virFDStreamInternalCloseCb cb, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 08111d4..ab7a39e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -823,6 +823,7 @@ virStreamClass; virFDStreamConnectUNIX; virFDStreamCreateFile; virFDStreamOpen; +virFDStreamOpenBlockDevice; virFDStreamOpenFile; virFDStreamOpenPTY; virFDStreamSetInternalCloseCb; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 9c775c9..00cfe74 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1718,7 +1718,8 @@ virStorageBackendVolUploadLocal(virConnectPtr conn ATTRIBUTE_UNUSED, /* Not using O_CREAT because the file is required to already exist at * this point */ - return virFDStreamOpenFile(stream, vol->target.path, offset, len, O_WRONLY); + return virFDStreamOpenBlockDevice(stream, vol->target.path, + offset, len, O_WRONLY); } int @@ -1732,7 +1733,8 @@ virStorageBackendVolDownloadLocal(virConnectPtr conn ATTRIBUTE_UNUSED, { virCheckFlags(0, -1); - return virFDStreamOpenFile(stream, vol->target.path, offset, len, O_RDONLY); + return virFDStreamOpenBlockDevice(stream, vol->target.path, + offset, len, O_RDONLY); } -- 1.9.0
On Fri, Aug 15, 2014 at 12:44:21PM +0400, Roman Bogorodskiy wrote:
virStorageBackendVolDownloadLocal and virStorageBackendVolUploadLocal use virFDStreamOpenFile function to work with the volume fd.
virFDStreamOpenFile calls virFDStreamOpenFileInternal that implements handling of the non-blocking I/O. If a file is not a character device and not a fifo, it uses libvirt_iohelper.
On FreeBSD, it doesn't work as expected because disks are exposed as character devices.
Why does that cause a problem ? The reason we use iohelper is because POSIX does not have O_NONBLOCK work on plain files, so we need to use the iohelper so that we have a pipe we can set O_NONBLOCK on. If FreeBSD disks are character devices though, O_NONBLOCK should work fine on them avoiding the need for iohelper. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Daniel P. Berrange wrote:
On Fri, Aug 15, 2014 at 12:44:21PM +0400, Roman Bogorodskiy wrote:
virStorageBackendVolDownloadLocal and virStorageBackendVolUploadLocal use virFDStreamOpenFile function to work with the volume fd.
virFDStreamOpenFile calls virFDStreamOpenFileInternal that implements handling of the non-blocking I/O. If a file is not a character device and not a fifo, it uses libvirt_iohelper.
On FreeBSD, it doesn't work as expected because disks are exposed as character devices.
Why does that cause a problem ? The reason we use iohelper is because POSIX does not have O_NONBLOCK work on plain files, so we need to use the iohelper so that we have a pipe we can set O_NONBLOCK on. If FreeBSD disks are character devices though, O_NONBLOCK should work fine on them avoiding the need for iohelper.
O_NONBLOCK doesn't work for some reason, at least on ZFS volumes: fcntl() fails with: fcntl: Inappropriate ioctl for device I have asked a question on an appropriate mailing list: https://lists.freebsd.org/pipermail/freebsd-fs/2014-August/019923.html I've also included a test program I use to reproduce that there.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Roman Bogorodskiy
On Fri, Aug 15, 2014 at 12:55:58PM +0400, Roman Bogorodskiy wrote:
Daniel P. Berrange wrote:
On Fri, Aug 15, 2014 at 12:44:21PM +0400, Roman Bogorodskiy wrote:
virStorageBackendVolDownloadLocal and virStorageBackendVolUploadLocal use virFDStreamOpenFile function to work with the volume fd.
virFDStreamOpenFile calls virFDStreamOpenFileInternal that implements handling of the non-blocking I/O. If a file is not a character device and not a fifo, it uses libvirt_iohelper.
On FreeBSD, it doesn't work as expected because disks are exposed as character devices.
Why does that cause a problem ? The reason we use iohelper is because POSIX does not have O_NONBLOCK work on plain files, so we need to use the iohelper so that we have a pipe we can set O_NONBLOCK on. If FreeBSD disks are character devices though, O_NONBLOCK should work fine on them avoiding the need for iohelper.
O_NONBLOCK doesn't work for some reason, at least on ZFS volumes: fcntl() fails with:
fcntl: Inappropriate ioctl for device
I have asked a question on an appropriate mailing list:
https://lists.freebsd.org/pipermail/freebsd-fs/2014-August/019923.html
I've also included a test program I use to reproduce that there.
Ah ok, so can you update the commit message to explicitly mention that ZFS character devices are unusual in not supporting O_NOBLOBKJ Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Daniel P. Berrange wrote:
On Fri, Aug 15, 2014 at 12:55:58PM +0400, Roman Bogorodskiy wrote:
Daniel P. Berrange wrote:
On Fri, Aug 15, 2014 at 12:44:21PM +0400, Roman Bogorodskiy wrote:
virStorageBackendVolDownloadLocal and virStorageBackendVolUploadLocal use virFDStreamOpenFile function to work with the volume fd.
virFDStreamOpenFile calls virFDStreamOpenFileInternal that implements handling of the non-blocking I/O. If a file is not a character device and not a fifo, it uses libvirt_iohelper.
On FreeBSD, it doesn't work as expected because disks are exposed as character devices.
Why does that cause a problem ? The reason we use iohelper is because POSIX does not have O_NONBLOCK work on plain files, so we need to use the iohelper so that we have a pipe we can set O_NONBLOCK on. If FreeBSD disks are character devices though, O_NONBLOCK should work fine on them avoiding the need for iohelper.
O_NONBLOCK doesn't work for some reason, at least on ZFS volumes: fcntl() fails with:
fcntl: Inappropriate ioctl for device
I have asked a question on an appropriate mailing list:
https://lists.freebsd.org/pipermail/freebsd-fs/2014-August/019923.html
I've also included a test program I use to reproduce that there.
Ah ok, so can you update the commit message to explicitly mention that ZFS character devices are unusual in not supporting O_NOBLOBKJ
I did some more checks and it looks like O_NONBLOCK is not supported not only on ZFS volumes, but on other disk drivers as well; I got the same result for SATA and memory disks. I'll update a commit message to say something like this:
On FreeBSD, it doesn't work as expected because disks are exposed as character devices and do not support O_NONBLOCK.
I'll wait for other comments to avoid resending the series just because of a commit message change. Roman Bogorodskiy
Add an implementation of uploadVol and downloadVol using virStorageBackendVolUploadLocal and virStorageBackendVolDownloadLocal respectively. --- src/storage/storage_backend_zfs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c index 2aeefb5..2d74055 100644 --- a/src/storage/storage_backend_zfs.c +++ b/src/storage/storage_backend_zfs.c @@ -326,4 +326,6 @@ virStorageBackend virStorageBackendZFS = { .refreshPool = virStorageBackendZFSRefreshPool, .createVol = virStorageBackendZFSCreateVol, .deleteVol = virStorageBackendZFSDeleteVol, + .uploadVol = virStorageBackendVolUploadLocal, + .downloadVol = virStorageBackendVolDownloadLocal, }; -- 1.9.0
Roman Bogorodskiy wrote:
Roman Bogorodskiy (3): fdstream: report error if virSetNonBlock fails fdstream: introduce virFDStreamOpenBlockDevice storage: zfs: implement download and upload
src/fdstream.c | 30 +++++++++++++++++++++++------- src/fdstream.h | 5 +++++ src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 6 ++++-- src/storage/storage_backend_zfs.c | 2 ++ 5 files changed, 35 insertions(+), 9 deletions(-)
Ping? Roman Bogorodskiy
On 15.08.2014 10:44, Roman Bogorodskiy wrote:
Roman Bogorodskiy (3): fdstream: report error if virSetNonBlock fails fdstream: introduce virFDStreamOpenBlockDevice storage: zfs: implement download and upload
src/fdstream.c | 30 +++++++++++++++++++++++------- src/fdstream.h | 5 +++++ src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 6 ++++-- src/storage/storage_backend_zfs.c | 2 ++ 5 files changed, 35 insertions(+), 9 deletions(-)
ACK to all the patches, if you work in Dan's comment in 2/3. Michal
Michal Privoznik wrote:
On 15.08.2014 10:44, Roman Bogorodskiy wrote:
Roman Bogorodskiy (3): fdstream: report error if virSetNonBlock fails fdstream: introduce virFDStreamOpenBlockDevice storage: zfs: implement download and upload
src/fdstream.c | 30 +++++++++++++++++++++++------- src/fdstream.h | 5 +++++ src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 6 ++++-- src/storage/storage_backend_zfs.c | 2 ++ 5 files changed, 35 insertions(+), 9 deletions(-)
ACK to all the patches, if you work in Dan's comment in 2/3.
Updated commit message in 2/3 and pushed the series; thanks! Roman Bogorodskiy
participants (3)
-
Daniel P. Berrange -
Michal Privoznik -
Roman Bogorodskiy