GNOME Bugzilla – Bug 747128
sftp mount unusable when copying
Last modified: 2015-06-07 16:12:22 UTC
Since push and pull support was added, because 2 MiB of data is in flight, commands such as query_info and enumerate get queued behind the data and makes the mount unusable (e.g. to browse in Nautilus) when copying at the same time. This is especially noticeable over a slow internet connection. I think the best fix is to use two connections, one for bulk data transfer (i.e. push and pull) and one for the rest.
Created attachment 300688 [details] [review] sftp: Add support for multiple connections Multiple connections are needed since 7890d2801a7f ("sftp: Implement push support") and ed826fdf386c ("sftp: Implement pull support"). This is because up to 2 MiB of data may be in flight, so any "command" sent (e.g. refreshing the view in Nautilus) gets queued up which makes the mount unusable on a slow connection when copying at the same time. This changes the code to support multiple connections but doesn't actually make use of them.
Created attachment 300689 [details] [review] sftp: Login to both connections when mounting Login to a command connection and a data connection when mounting. Any password entered during the first connection is reused for the second connection.
Created attachment 300690 [details] [review] sftp: Use the data connection when pushing data
Created attachment 300691 [details] [review] sftp: Use the data connection when pulling data
With these patches, an sftp mount is a lot more usable when copying data at the same time.
Review of attachment 300688 [details] [review]: It would be better only refactor the code in this patch and add the data_connection with another patch, otherwise looks good... ::: daemon/gvfsbackendsftp.c @@ +281,3 @@ + + if (conn->command_stream) + g_object_unref (conn->command_stream); Good time to change it to g_clear_object?
Review of attachment 300689 [details] [review]: ::: daemon/gvfsbackendsftp.c @@ +959,1 @@ DEBUG ("handle_login #%d - prompt: \"%s\"\n", i, buffer); Would be good to print if it is initial connection in the debugs... @@ +1205,2 @@ /* Login succeed, save password in keyring */ g_vfs_keyring_save_password (op_backend->user, I think you shouldn't save the credentials for non-initial connections (again)... @@ +1852,3 @@ + g_clear_pointer (&op_backend->tmp_password, g_free); + g_cancellable_cancel (op_backend->command_connection.reply_stream_cancellable); + return; What if ssdh_config contains "MaxSessions=1"? Shouldn't this be non-fatal since we are able to work only with one connection (add fallback to use command_connection instead)?
Review of attachment 300690 [details] [review]: Looks good!
Review of attachment 300691 [details] [review]: It needs commit description... ::: daemon/gvfsbackendsftp.c @@ +1560,3 @@ + Connection *connection; + GDataOutputStream *cmd; +} Command; Wouldn't be better to split the patch - introduce this struct in a one patch and use the data_connection in another one?
(In reply to Ross Lagerwall from comment #5) > With these patches, an sftp mount is a lot more usable when copying data at > the same time. Good idea! Don't see difference on localhost, but it could be useful on slow connections...
Created attachment 303906 [details] [review] sftp: Add support for multiple connections Multiple connections are needed since 7890d2801a7f ("sftp: Implement push support") and ed826fdf386c ("sftp: Implement pull support"). This is because up to 2 MiB of data may be in flight, so any "command" sent (e.g. refreshing the view in Nautilus) gets queued up which makes the mount unusable on a slow connection when copying at the same time. This changes the code to support multiple connections but still uses a single connection.
Created attachment 303907 [details] [review] sftp: Add a data connection Add a data connection in addition to the command connection currently used.
Created attachment 303908 [details] [review] sftp: Login to both connections when mounting Login to the command connection and the data connection when mounting. Any password entered during the first connection is reused for the second connection. Because the data connection is not essential, do not fail if the connection attempt fails, and introduce a function to determine if a connection is usable.
Created attachment 303909 [details] [review] sftp: Use the data connection when pushing data Use the newly-introduced data connection when pushing data from a sftp mount. This prevents blocking other operations behind a few MBs of in-flight data and improves interactivity. If the data connection is not usable, fall back to the default copy implementation.
Created attachment 303910 [details] [review] sftp: Allow queuing and waiting for commands on different connections Update the infrastructure to allow submitting multiple commands to be sent on different connections and then waiting for the all to complete.
Created attachment 303911 [details] [review] sftp: Use the data connection when pulling data Use the newly-introduced data connection when pulling data from a sftp mount. This prevents blocking other operations behind a few MBs of in-flight data and improves interactivity. If the data connection is not usable, fall back to the default copy implementation.
Review of attachment 303908 [details] [review]: ::: daemon/gvfsbackendsftp.c @@ +908,3 @@ #endif + DEBUG ("handle_login #%d, %sinitial_connection - user: %s, host: %s, port: %d\n", Might be better to print initial_connection = %d as other booleans, but it is just detail... @@ +1830,3 @@ + &error)) + { + if (error) Would be good to add a note that error isn't returned in case of do_mount recursion when new user is specified, because it looks like we can return here without finishing the job...
Review of attachment 303906 [details] [review]: ::: daemon/gvfsbackendsftp.c @@ +1281,3 @@ { if (expected_reply->callback != NULL) + { There is no reason for adding those curly brackets, is it?
Rest of the patches looks good to me (I'm lazy to mark all of them as accepted), thanks for them. It seems sftp is working correctly. Just they are not applicable on master, so be careful...
Pushed all to master as 6809b68..ffe8bdf. Thanks for the reviews!