sftp performance problem, cured by TCP_NODELAY

[Miklos Szeredi]

> I ran a version (likely somewhat old) of what ships with HP-UX under
> a system call trace tool.  I'm not exactly sure what I was seeing,
> but I did see several instances of call flow like:
> 
> write four bytes to the socket
> write lots of bytes to the socket  (<= 8KB IIRC)
> 
> which smells (an awful lot) like something writing a message length
> to a socket and then writing the rest of the message.

Yup.

> I also saw a pattern of
> 
> read four bytes from a socket (a different one)
> read lots of bytes from a socket
> 
> Which seemed to support the header, data hypothesis
> 
> Now, the latter (read four read lots) is really just a question of
> CPU overheads, it does not affect what goes-out onto the network.
> 
> However, the former looks like a classic case of an "unfortuneate"
> (if being dimplomatic, "buggy" if not) coding practice that quite
> easily runs afoul of Nagle - writing "logically associated data" to
> the connection in separate calls.

Yes, this is unfortunate.  But I fixed it (see patch below) and it
didn't help.  I think this would have actually triggered multiple
writes only very rarely on UP, since ssh would need to be scheduled in
between the two writes to actually see the split message, otherwise
the pipe would merge the two writes before it gets to ssh.

On SMP this may be worse, and applying the patch is a good idea anyway.

It seems that sftp-server is already doing the right thing.

> Assuming my brief syscall trace analysis is correct and if that
> behaviour is the same in the contemporary versions of sftp, it might
> be better to fix that before asking to do TCP_NODELAY.

Done.  Can I now have my NODELAY please ;)

Miklos

--- openssh-4.2p1/sftp-client.c	2005-08-02 09:07:08.000000000 +0200
+++ openssh-4.2p1.new/sftp-client.c	2006-01-26 19:22:39.000000000 +0100
@@ -57,16 +57,11 @@
 static void
 send_msg(int fd, Buffer *m)
 {
-	u_char mlen[4];
-
-	if (buffer_len(m) > MAX_MSG_LENGTH)
-		fatal("Outbound message too long %u", buffer_len(m));
+	if (buffer_len(m) - 4 > MAX_MSG_LENGTH)
+		fatal("Outbound message too long %u", buffer_len(m) - 4);
 
 	/* Send length first */
-	PUT_32BIT(mlen, buffer_len(m));
-	if (atomicio(vwrite, fd, mlen, sizeof(mlen)) != sizeof(mlen))
-		fatal("Couldn't send packet: %s", strerror(errno));
-
+	PUT_32BIT((u_char *) buffer_ptr(m), buffer_len(m) - 4);
 	if (atomicio(vwrite, fd, buffer_ptr(m), buffer_len(m)) != buffer_len(m))
 		fatal("Couldn't send packet: %s", strerror(errno));
 
@@ -106,6 +101,7 @@
 	Buffer msg;
 
 	buffer_init(&msg);
+	buffer_put_int(&msg, 0);
 	buffer_put_char(&msg, code);
 	buffer_put_int(&msg, id);
 	buffer_put_string(&msg, s, len);
@@ -121,6 +117,7 @@
 	Buffer msg;
 
 	buffer_init(&msg);
+	buffer_put_int(&msg, 0);
 	buffer_put_char(&msg, code);
 	buffer_put_int(&msg, id);
 	buffer_put_string(&msg, s, len);
@@ -229,6 +226,7 @@
 	struct sftp_conn *ret;
 
 	buffer_init(&msg);
+	buffer_put_int(&msg, 0);
 	buffer_put_char(&msg, SSH2_FXP_INIT);
 	buffer_put_int(&msg, SSH2_FILEXFER_VERSION);
 	send_msg(fd_out, &msg);
@@ -288,6 +286,7 @@
 	Buffer msg;
 
 	buffer_init(&msg);
+	buffer_put_int(&msg, 0);
 
 	id = conn->msg_id++;
 	buffer_put_char(&msg, SSH2_FXP_CLOSE);
@@ -317,6 +316,7 @@
 	id = conn->msg_id++;
 
 	buffer_init(&msg);
+	buffer_put_int(&msg, 0);
 	buffer_put_char(&msg, SSH2_FXP_OPENDIR);
 	buffer_put_int(&msg, id);
 	buffer_put_cstring(&msg, path);
@@ -340,6 +340,7 @@
 		debug3("Sending SSH2_FXP_READDIR I:%u", id);
 
 		buffer_clear(&msg);
+		buffer_put_int(&msg, 0);
 		buffer_put_char(&msg, SSH2_FXP_READDIR);
 		buffer_put_int(&msg, id);
 		buffer_put_string(&msg, handle, handle_len);
@@ -619,6 +620,7 @@
 	u_int status, id;
 
 	buffer_init(&msg);
+	buffer_put_int(&msg, 0);
 
 	/* Send rename request */
 	id = conn->msg_id++;
@@ -651,6 +653,7 @@
 	}
 
 	buffer_init(&msg);
+	buffer_put_int(&msg, 0);
 
 	/* Send symlink request */
 	id = conn->msg_id++;
@@ -726,6 +729,7 @@
 
 	buffer_init(&msg);
 	buffer_clear(&msg);
+	buffer_put_int(&msg, 0);
 	buffer_put_char(&msg, SSH2_FXP_READ);
 	buffer_put_int(&msg, id);
 	buffer_put_string(&msg, handle, handle_len);
@@ -781,6 +785,7 @@
 
 	buflen = conn->transfer_buflen;
 	buffer_init(&msg);
+	buffer_put_int(&msg, 0);
 
 	/* Send open request */
 	id = conn->msg_id++;
@@ -1022,6 +1027,7 @@
 		a.flags &= ~SSH2_FILEXFER_ATTR_ACMODTIME;
 
 	buffer_init(&msg);
+	buffer_put_int(&msg, 0);
 
 	/* Send open request */
 	id = conn->msg_id++;
@@ -1077,6 +1083,7 @@
 			TAILQ_INSERT_TAIL(&acks, ack, tq);
 
 			buffer_clear(&msg);
+			buffer_put_int(&msg, 0);
 			buffer_put_char(&msg, SSH2_FXP_WRITE);
 			buffer_put_int(&msg, ack->id);
 			buffer_put_string(&msg, handle, handle_len);