| Message ID | 1365501393-5010-3-git-send-email-rjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 09, 2013 at 10:56:32AM +0100, Richard W.M. Jones wrote: These are both not worth respinning for, but just in case you send another version. > +static coroutine_fn int ssh_flush(BDRVSSHState *s) > +{ > + int r; > + static int warned = 0; > + > + DPRINTF("fsync"); > + again: > + r = libssh2_sftp_fsync(s->sftp_handle); > + if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) { > + co_yield(s); > + goto again; > + } > + if (r == LIBSSH2_ERROR_SFTP_PROTOCOL && > + libssh2_sftp_last_error(s->sftp) == LIBSSH2_FX_OP_UNSUPPORTED) { > + if (!warned) { > + error_report("warning: remote ssh server does not support fsync"); Perhaps not worth fixing but this error message has two properties which could be improved: 1. When multiple -drive file=ssh:// are used it is unclear which SSH server lacks fsync support since the error message does not say. 2. When multiple -drive file=ssh:// are used only the first server lacking support will print a warning since warned is static. > diff --git a/qemu-doc.texi b/qemu-doc.texi > index 7a0f373..9e30667 100644 > --- a/qemu-doc.texi > +++ b/qemu-doc.texi > @@ -1072,12 +1072,6 @@ the standard ssh port (22) is used. > Currently authentication must be done using ssh-agent. Other > authentication methods may be supported in future. > > -Note: The ssh driver does not obey disk flush requests (ie. to commit > -data to the backing disk when the guest requests it). This is because > -the underlying protocol (SFTP) does not support this. Thus there is a > -risk of guest disk corruption if the remote server or network goes > -down during writes. We can expect many SSH servers not to support the fsync extension. It would be best to include a note explaining that fsync is not available on older ssh servers and therefore data integrity cannot be guaranteed in those cases. Stefan
On Tue, Apr 09, 2013 at 02:47:14PM +0200, Stefan Hajnoczi wrote: > On Tue, Apr 09, 2013 at 10:56:32AM +0100, Richard W.M. Jones wrote: > > These are both not worth respinning for, but just in case you send > another version. > > > +static coroutine_fn int ssh_flush(BDRVSSHState *s) > > +{ > > + int r; > > + static int warned = 0; > > + > > + DPRINTF("fsync"); > > + again: > > + r = libssh2_sftp_fsync(s->sftp_handle); > > + if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) { > > + co_yield(s); > > + goto again; > > + } > > + if (r == LIBSSH2_ERROR_SFTP_PROTOCOL && > > + libssh2_sftp_last_error(s->sftp) == LIBSSH2_FX_OP_UNSUPPORTED) { > > + if (!warned) { > > + error_report("warning: remote ssh server does not support fsync"); > > Perhaps not worth fixing but this error message has two properties which > could be improved: > > 1. When multiple -drive file=ssh:// are used it is unclear which SSH > server lacks fsync support since the error message does not say. > > 2. When multiple -drive file=ssh:// are used only the first server > lacking support will print a warning since warned is static. > > > diff --git a/qemu-doc.texi b/qemu-doc.texi > > index 7a0f373..9e30667 100644 > > --- a/qemu-doc.texi > > +++ b/qemu-doc.texi > > @@ -1072,12 +1072,6 @@ the standard ssh port (22) is used. > > Currently authentication must be done using ssh-agent. Other > > authentication methods may be supported in future. > > > > -Note: The ssh driver does not obey disk flush requests (ie. to commit > > -data to the backing disk when the guest requests it). This is because > > -the underlying protocol (SFTP) does not support this. Thus there is a > > -risk of guest disk corruption if the remote server or network goes > > -down during writes. > > We can expect many SSH servers not to support the fsync extension. It > would be best to include a note explaining that fsync is not available > on older ssh servers and therefore data integrity cannot be guaranteed > in those cases. Thanks -- will fix both in the next version. Also I have (in my copy) implemented host_key_check parameter. Fun fact: OpenSSH uses MD5 to print SSH host key fingerprints. Rich.
diff --git a/block/ssh.c b/block/ssh.c index e1322cb..b83c239 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -826,6 +826,50 @@ static coroutine_fn int ssh_co_writev(BlockDriverState *bs, return ret; } +#ifdef HAS_LIBSSH2_SFTP_FSYNC + +static coroutine_fn int ssh_flush(BDRVSSHState *s) +{ + int r; + static int warned = 0; + + DPRINTF("fsync"); + again: + r = libssh2_sftp_fsync(s->sftp_handle); + if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) { + co_yield(s); + goto again; + } + if (r == LIBSSH2_ERROR_SFTP_PROTOCOL && + libssh2_sftp_last_error(s->sftp) == LIBSSH2_FX_OP_UNSUPPORTED) { + if (!warned) { + error_report("warning: remote ssh server does not support fsync"); + warned = 1; + } + return 0; + } + if (r < 0) { + sftp_error_report(s, "fsync failed"); + return -EIO; + } + + return 0; +} + +static coroutine_fn int ssh_co_flush(BlockDriverState *bs) +{ + BDRVSSHState *s = bs->opaque; + int ret; + + qemu_co_mutex_lock(&s->lock); + ret = ssh_flush(s); + qemu_co_mutex_unlock(&s->lock); + + return ret; +} + +#endif /* HAS_LIBSSH2_SFTP_FSYNC */ + static int64_t ssh_getlength(BlockDriverState *bs) { BDRVSSHState *s = bs->opaque; @@ -849,6 +893,9 @@ static BlockDriver bdrv_ssh = { .bdrv_co_readv = ssh_co_readv, .bdrv_co_writev = ssh_co_writev, .bdrv_getlength = ssh_getlength, +#ifdef HAS_LIBSSH2_SFTP_FSYNC + .bdrv_co_flush_to_disk = ssh_co_flush, +#endif .create_options = ssh_create_options, }; diff --git a/configure b/configure index 4dd65b0..fa79588 100755 --- a/configure +++ b/configure @@ -2357,6 +2357,31 @@ EOF fi ########################################## +# libssh2_sftp_fsync probe + +if test "$libssh2" = "yes"; then + cat > $TMPC <<EOF +#include <stdio.h> +#include <libssh2.h> +#include <libssh2_sftp.h> +int main(void) { + LIBSSH2_SESSION *session; + LIBSSH2_SFTP *sftp; + LIBSSH2_SFTP_HANDLE *sftp_handle; + session = libssh2_session_init (); + sftp = libssh2_sftp_init (session); + sftp_handle = libssh2_sftp_open (sftp, "/", 0, 0); + libssh2_sftp_fsync (sftp_handle); + return 0; +} +EOF + # libssh2_cflags/libssh2_libs defined in previous test. + if compile_prog "$libssh2_cflags" "$libssh2_libs" ; then + QEMU_CFLAGS="-DHAS_LIBSSH2_SFTP_FSYNC $QEMU_CFLAGS" + fi +fi + +########################################## # linux-aio probe if test "$linux_aio" != "no" ; then diff --git a/qemu-doc.texi b/qemu-doc.texi index 7a0f373..9e30667 100644 --- a/qemu-doc.texi +++ b/qemu-doc.texi @@ -1072,12 +1072,6 @@ the standard ssh port (22) is used. Currently authentication must be done using ssh-agent. Other authentication methods may be supported in future. -Note: The ssh driver does not obey disk flush requests (ie. to commit -data to the backing disk when the guest requests it). This is because -the underlying protocol (SFTP) does not support this. Thus there is a -risk of guest disk corruption if the remote server or network goes -down during writes. - @node pcsys_network @section Network emulation
From: "Richard W.M. Jones" <rjones@redhat.com> libssh2_sftp_fsync is an extension to libssh2 to support fsync(2) over sftp, which is itself an extension of OpenSSH. If both libssh2 and the ssh daemon support it, this will allow bdrv_flush_to_disk to commit changes through to disk on the remote server. --- block/ssh.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ configure | 25 +++++++++++++++++++++++++ qemu-doc.texi | 6 ------ 3 files changed, 72 insertions(+), 6 deletions(-)