The included patch for openssh 3.0.2p1 implements overlapping read
requests for the sftp client. It should be able to handle weird cases
such as shriking files and reordered responses. This is only the first
shot, and I'd be happy for any comments. I plan to implement something
similar for the write path if this works out well.
The maximum number of outstanding requests is quite high at the moment
(6). I used that number since it gave the best performance on my 1 GHz
Athlon using the looback interface in Linux 2.4, and it did not degrade
perfomce over 100Mb/s ethernet or 700 kb/s ADSL which is what I have easy
access to at the moment. The drawback of many outstanding requests is
that when we detect an error, we must wait for the responses to our
outstanding requests. I should also mention that the code starts with
only one request, and increases the number by one for each successful
reply up to the maximum (6). That makes sure we do not waste packets for
short files or some obvious error cases.
There are two ideas I have not tried yet. the first is not to use the
file size from the stat call to avoid issuing more than one request past
the presumed end-of-file. The other idea is to start with COPY_SIZE
requests, but to use the size of the responses (which might be less then
COPY_SIZE) for future requests. The reason for doing this is to avoid
non-sequential requests to servers with small maximum responses sizes.
That way we could also increase COPY_SIZE to something more efficient.
Does anyone here have any comments on reading from or writing to
non-regular files using the sftp protocol? The offset argument to the
read and write requests seems to indicate that the protocol is only
intended for regular files. Should the sftp server and client verify
that, or is that too anal?
/Tobias
diff -ru openssh-3.0.2p1.orig/sftp-client.c openssh-3.0.2p1/sftp-client.c
--- openssh-3.0.2p1.orig/sftp-client.c Wed Jul 18 17:45:45 2001
+++ openssh-3.0.2p1/sftp-client.c Fri Jan 4 00:09:44 2002
@@ -45,6 +45,15 @@
/* How much data to read/write at at time during copies */
/* XXX: what should this be? */
#define COPY_SIZE 8192
+/* Maximum number of outstanding requests */
+#define REQUEST_QUEUE_SIZE 6
+
+/* A read/write request */
+struct request {
+ u_int id;
+ u_int len;
+ u_int64_t offset;
+};
/* Message ID */
static u_int msg_id = 1;
@@ -215,6 +224,44 @@
return(a);
}
+static int
+find_request(struct request *rq, int num, u_int id)
+{
+ int i;
+
+ for (i = 0; i < num; ++i) {
+ if (rq[i].id == id)
+ break;
+ }
+
+ if (i == num)
+ fatal("Request ID mismatch (%d)", id);
+
+ return i;
+}
+
+static void
+remove_request(struct request *rq, int *num, int i)
+{
+ memmove(rq + i, rq + i + 1, (*num - i - 1) * sizeof(struct request));
+ --*num;
+}
+
+static void
+send_request(int fd, const char *handle, u_int handle_len, int type,
+ const struct request *rq, Buffer *m)
+{
+ buffer_clear(m);
+ buffer_put_char(m, SSH2_FXP_READ);
+ buffer_put_int(m, rq->id);
+ buffer_put_string(m, handle, handle_len);
+ buffer_put_int64(m, rq->offset);
+ buffer_put_int(m, rq->len);
+ send_msg(fd, m);
+ debug3("Sent message SSH2_FXP_READ I:%d O:%llu S:%u",
+ rq->id, rq->offset, rq->len);
+}
+
int
do_init(int fd_in, int fd_out)
{
@@ -674,12 +721,15 @@
int pflag)
{
int local_fd;
- u_int expected_id, handle_len, mode, type, id;
+ u_int handle_len, mode, type, id;
u_int64_t offset;
char *handle;
Buffer msg;
Attrib junk, *a;
int status;
+ struct request req[REQUEST_QUEUE_SIZE];
+ int num_req = 0, max_req = 1, reply;
+ int write_error = 0, read_error = 0, write_errno;
a = do_stat(fd_in, fd_out, remote_path, 0);
if (a == NULL)
@@ -726,87 +776,103 @@
/* Read from remote and write to local */
offset = 0;
- for(;;) {
- u_int len;
+ while (num_req > 0 || max_req > 0) {
char *data;
+ u_int len;
- id = expected_id = msg_id++;
-
- buffer_clear(&msg);
- buffer_put_char(&msg, SSH2_FXP_READ);
- buffer_put_int(&msg, id);
- buffer_put_string(&msg, handle, handle_len);
- buffer_put_int64(&msg, offset);
- buffer_put_int(&msg, COPY_SIZE);
- send_msg(fd_out, &msg);
- debug3("Sent message SSH2_FXP_READ I:%d O:%llu S:%u",
- id, (u_int64_t)offset, COPY_SIZE);
+ /* Send some more requests */
+ while (num_req < max_req) {
+ req[num_req].id = msg_id++;
+ req[num_req].len = COPY_SIZE;
+ req[num_req].offset = offset;
+ offset += req[num_req].len;
+ num_req++;
+ send_request(fd_out, handle, handle_len, SSH2_FXP_READ,
+ &req[num_req - 1], &msg);
+ }
buffer_clear(&msg);
-
get_msg(fd_in, &msg);
type = buffer_get_char(&msg);
id = buffer_get_int(&msg);
debug3("Received reply T:%d I:%d", type, id);
- if (id != expected_id)
- fatal("ID mismatch (%d != %d)", id, expected_id);
- if (type == SSH2_FXP_STATUS) {
+ reply = find_request(req, num_req, id);
+
+ switch (type) {
+ case SSH2_FXP_STATUS:
status = buffer_get_int(&msg);
+ if (status != SSH2_FX_EOF)
+ read_error = 1;
+ max_req = 0;
+ remove_request(req, &num_req, reply);
+ break;
+ case SSH2_FXP_DATA:
+ data = buffer_get_string(&msg, &len);
+ if (len > req[reply].len)
+ fatal("Received more data than asked for "
+ "%d > %d", len, req[reply].len);
+
+ debug3("In read loop, got %d offset %llu",
+ len, req[reply].offset);
+ if ((lseek(local_fd, req[reply].offset, SEEK_SET) == -1 ||
+ atomicio(write, local_fd, data, len) != len) &&
+ !write_error) {
+ write_errno = errno;
+ write_error = 1;
+ max_req = 0;
+ }
+ xfree(data);
- if (status == SSH2_FX_EOF)
- break;
+ if (len == req[reply].len)
+ remove_request(req, &num_req, reply);
else {
- error("Couldn't read from remote "
- "file \"%s\" : %s", remote_path,
- fx2txt(status));
- do_close(fd_in, fd_out, handle, handle_len);
- goto done;
+ /* Resend the request for the missing data */
+ req[reply].id = msg_id++;
+ req[reply].len -= len;
+ req[reply].offset += len;
+ send_request(fd_out, handle, handle_len,
+ SSH2_FXP_READ, &req[reply], &msg);
}
- } else if (type != SSH2_FXP_DATA) {
+ if (max_req > 0 && max_req < REQUEST_QUEUE_SIZE)
+ ++max_req;
+ break;
+ default:
fatal("Expected SSH2_FXP_DATA(%d) packet, got %d",
- SSH2_FXP_DATA, type);
- }
-
- data = buffer_get_string(&msg, &len);
- if (len > COPY_SIZE)
- fatal("Received more data than asked for %d > %d",
- len, COPY_SIZE);
-
- debug3("In read loop, got %d offset %llu", len,
- (u_int64_t)offset);
- if (atomicio(write, local_fd, data, len) != len) {
- error("Couldn't write to \"%s\": %s", local_path,
- strerror(errno));
- do_close(fd_in, fd_out, handle, handle_len);
- status = -1;
- xfree(data);
- goto done;
+ SSH2_FXP_DATA, type);
}
-
- offset += len;
- xfree(data);
}
- status = do_close(fd_in, fd_out, handle, handle_len);
- /* Override umask and utimes if asked */
+ if (read_error) {
+ error("Couldn't read from remote "
+ "file \"%s\" : %s", remote_path,
+ fx2txt(status));
+ do_close(fd_in, fd_out, handle, handle_len);
+ } else if (write_error) {
+ error("Couldn't write to \"%s\": %s", local_path,
+ strerror(write_errno));
+ status = -1;
+ do_close(fd_in, fd_out, handle, handle_len);
+ } else {
+ status = do_close(fd_in, fd_out, handle, handle_len);
+
+ /* Override umask and utimes if asked */
#ifdef HAVE_FCHMOD
- if (pflag && fchmod(local_fd, mode) == -1)
+ if (pflag && fchmod(local_fd, mode) == -1)
#else
- if (pflag && chmod(local_path, mode) == -1)
+ if (pflag && chmod(local_path, mode) == -1)
#endif /* HAVE_FCHMOD */
- error("Couldn't set mode on \"%s\": %s", local_path,
- strerror(errno));
- if (pflag && (a->flags & SSH2_FILEXFER_ATTR_ACMODTIME)) {
- struct timeval tv[2];
- tv[0].tv_sec = a->atime;
- tv[1].tv_sec = a->mtime;
- tv[0].tv_usec = tv[1].tv_usec = 0;
- if (utimes(local_path, tv) == -1)
- error("Can't set times on \"%s\": %s", local_path,
- strerror(errno));
+ error("Couldn't set mode on \"%s\": %s", local_path,
+ strerror(errno));
+ if (pflag && (a->flags & SSH2_FILEXFER_ATTR_ACMODTIME)) {
+ struct timeval tv[2];
+ tv[0].tv_sec = a->atime;
+ tv[1].tv_sec = a->mtime;
+ tv[0].tv_usec = tv[1].tv_usec = 0;
+ if (utimes(local_path, tv) == -1)
+ error("Can't set times on \"%s\": %s",
+ local_path, strerror(errno));
+ }
}
-
-done:
close(local_fd);
buffer_free(&msg);
xfree(handle);