[PATCH 0/2] Handle incomplete tap writes for pasta

[PATCH 0/2] Handle incomplete tap writes for pasta

[David Gibson]

passt can handle various cases where we're unable to write all the
data we want to to the tap side.  The same logic would make sense for
pasta, however right now we don't properly report incomplete writes
with the tuntap device of pasta, rather than the qemu socket of passt.

I come across these as possible issues with the various stall bugs
we've been investigating.  It doesn't look like they're relevant there
after all, but they should be correct changes nonetheless.

David Gibson (2):
  tap, pasta: Handle incomplete tap sends for pasta too
  tap, pasta: Handle short writes to /dev/tap

 tap.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
2.41.0

[PATCH 1/2] tap, pasta: Handle incomplete tap sends for pasta too

[David Gibson]

Since a469fc39 ("tcp, tap: Don't increase tap-side sequence counter for
dropped frames") we've handled more gracefully the case where we get data
from the socket side, but are temporarily unable to send it all to the tap
side (e.g. due to full buffers).

That code relies on tap_send_frames() returning the number of frames it
successfully sent, which in turn gets it from tap_send_frames_passt() or
tap_send_frames_pasta().

While tap_send_frames_passt() has returned that information since b62ed9ca
("tap: Don't pcap frames that didn't get sent"), tap_send_frames_pasta()
always returns as though it succesfully sent every frame.  However there
certainly are cases where it will return early without sending all frames.
Update it report that properly, so that the calling functions can handle it
properly.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tap.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tap.c b/tap.c
index 3a938f3..00622e7 100644
--- a/tap.c
+++ b/tap.c
@@ -330,8 +330,6 @@ static size_t tap_send_frames_pasta(const struct ctx *c,
 			case EWOULDBLOCK:
 #endif
 			case EINTR:
-				i--;
-				break;
 			case ENOBUFS:
 			case ENOSPC:
 				break;
@@ -341,7 +339,7 @@ static size_t tap_send_frames_pasta(const struct ctx *c,
 		}
 	}
 
-	return n;
+	return i;
 }
 
 /**
-- 
@@ -330,8 +330,6 @@ static size_t tap_send_frames_pasta(const struct ctx *c,
 			case EWOULDBLOCK:
 #endif
 			case EINTR:
-				i--;
-				break;
 			case ENOBUFS:
 			case ENOSPC:
 				break;
@@ -341,7 +339,7 @@ static size_t tap_send_frames_pasta(const struct ctx *c,
 		}
 	}
 
-	return n;
+	return i;
 }
 
 /**
-- 
2.41.0

[PATCH 2/2] tap, pasta: Handle short writes to /dev/tap

[David Gibson]

tap_send_frames_pasta() sends frames to the namespace by sending them to
our the /dev/tap device.  If that write() returns an error, we already
handle it.  However we don't handle the case where the write() returns
short, meaning we haven't successfully transmitted the whole frame.

I don't know if this can ever happen with the kernel tap device, but we
should at least report the case so we don't get a cryptic failure.  For
the purposes of the return value for tap_send_frames_pasta() we treat this
case as though it was an error (on the grounds that a partial frame is no
use to the namespace).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tap.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tap.c b/tap.c
index 00622e7..4f11000 100644
--- a/tap.c
+++ b/tap.c
@@ -321,7 +321,9 @@ static size_t tap_send_frames_pasta(const struct ctx *c,
 	size_t i;
 
 	for (i = 0; i < n; i++) {
-		if (write(c->fd_tap, iov[i].iov_base, iov[i].iov_len) < 0) {
+		ssize_t rc = write(c->fd_tap, iov[i].iov_base, iov[i].iov_len);
+
+		if (rc < 0) {
 			debug("tap write: %s", strerror(errno));
 
 			switch (errno) {
@@ -336,6 +338,10 @@ static size_t tap_send_frames_pasta(const struct ctx *c,
 			default:
 				die("Write error on tap device, exiting");
 			}
+		} else if ((size_t)rc < iov[i].iov_len) {
+			debug("short write on tuntap: %zd/%zu",
+			      rc, iov[i].iov_len);
+			break;
 		}
 	}
 
-- 
@@ -321,7 +321,9 @@ static size_t tap_send_frames_pasta(const struct ctx *c,
 	size_t i;
 
 	for (i = 0; i < n; i++) {
-		if (write(c->fd_tap, iov[i].iov_base, iov[i].iov_len) < 0) {
+		ssize_t rc = write(c->fd_tap, iov[i].iov_base, iov[i].iov_len);
+
+		if (rc < 0) {
 			debug("tap write: %s", strerror(errno));
 
 			switch (errno) {
@@ -336,6 +338,10 @@ static size_t tap_send_frames_pasta(const struct ctx *c,
 			default:
 				die("Write error on tap device, exiting");
 			}
+		} else if ((size_t)rc < iov[i].iov_len) {
+			debug("short write on tuntap: %zd/%zu",
+			      rc, iov[i].iov_len);
+			break;
 		}
 	}
 
-- 
2.41.0

Re: [PATCH 0/2] Handle incomplete tap writes for pasta

[Stefano Brivio]

On Wed,  8 Nov 2023 14:17:52 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> passt can handle various cases where we're unable to write all the
> data we want to to the tap side.  The same logic would make sense for
> pasta, however right now we don't properly report incomplete writes
> with the tuntap device of pasta, rather than the qemu socket of passt.
> 
> I come across these as possible issues with the various stall bugs
> we've been investigating.  It doesn't look like they're relevant there
> after all, but they should be correct changes nonetheless.
> 
> David Gibson (2):
>   tap, pasta: Handle incomplete tap sends for pasta too
>   tap, pasta: Handle short writes to /dev/tap

Applied.

-- 
Stefano