Patchwork [v8,2/3] block: ssh: Use libssh2_sftp_fsync (if supported by libssh2) to flush to disk.

login
register
mail settings
Submitter Richard W.M. Jones
Date April 9, 2013, 9:56 a.m.
Message ID <1365501393-5010-3-git-send-email-rjones@redhat.com>
Download mbox | patch
Permalink /patch/235022/
State New
Headers show

Comments

Richard W.M. Jones - April 9, 2013, 9:56 a.m.
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(-)
Stefan Hajnoczi - April 9, 2013, 12:47 p.m.
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
Richard W.M. Jones - April 9, 2013, 1:04 p.m.
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.

Patch

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