Re: [Qemu-devel] [PATCH v8 2/3] block: ssh: Use libssh2_sftp_fsync (if supported by libssh2) to flush to disk.

[Stefan Hajnoczi <stefanha@gmail.com>]

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