pespin submitted this change.

View Change

osmo_io: Rewrite iofd_handle_send_completion() to fix multiple issues

Previous implementation had multiple issues, which this patch addresses:
* Didn't call user cb updating completed bytes for a given msg which had
  a partial write.
* rc checks against "Incomplete write" were wrong, because rc was not
  being decremented with previous msgs idxs being processed.
* Didn't take into account the write(0, fd, buf) = 0 valid user case for
  regular files.

The rewrite also simplifies the logic by splitting the "rc < 0" (error) paths
from the "rc >= 0" (successful) paths.

Related: SYS#7842
Change-Id: Ia016e4df7be5e534a8212f7271caff9779e08eb1
---
M src/core/osmo_io.c
1 file changed, 56 insertions(+), 32 deletions(-)

diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c
index 7c59a61..2a20405 100644
--- a/src/core/osmo_io.c
+++ b/src/core/osmo_io.c
@@ -509,6 +509,26 @@
 	}
 }
 
+static void iofd_send_completion_cb(struct osmo_io_fd *iofd, int rc, const struct iofd_msghdr *msghdr, struct msgb *msg)
+{
+	switch (msghdr->action) {
+	case IOFD_ACT_WRITE:
+		if (iofd->io_ops.write_cb)
+			iofd->io_ops.write_cb(iofd, rc, msg);
+		break;
+	case IOFD_ACT_SENDTO:
+		if (iofd->io_ops.sendto_cb)
+			iofd->io_ops.sendto_cb(iofd, rc, msg, &msghdr->osa);
+		break;
+	case IOFD_ACT_SENDMSG:
+		if (iofd->io_ops.sendmsg_cb)
+			iofd->io_ops.sendmsg_cb(iofd, rc, msg);
+		break;
+	default:
+		OSMO_ASSERT(0);
+	}
+}
+
 /*! completion handler: Internal function called by osmo_io_backend after a given I/O operation has completed
  *  \param[in] iofd I/O file-descriptor on which I/O has completed
  *  \param[in] rc return value of the I/O operation
@@ -518,18 +538,45 @@
 {
 	int idx, i;
 
-	/* Re-enqueue the complete msgb. */
-	if (rc == -EAGAIN) {
-		iofd_txqueue_enqueue_front(iofd, msghdr);
+	if (OSMO_UNLIKELY(rc < 0)) {
+		/* Re-enqueue the complete msgb. */
+		if (rc == -EAGAIN) {
+			iofd_txqueue_enqueue_front(iofd, msghdr);
+			return;
+		}
+
+		/* Propagate error for all msgbs to user cb: */
+		for (idx = 0; idx < msghdr->io_len; idx++) {
+			iofd_send_completion_cb(iofd, rc, msghdr, msghdr->msg[idx]);
+			msgb_free(msghdr->msg[idx]);
+			msghdr->msg[idx] = NULL;
+
+			/* The user can unregister/close the iofd during callback above. */
+			if (!IOFD_FLAG_ISSET(iofd, IOFD_FLAG_FD_REGISTERED))
+				break;
+		}
+		iofd_msghdr_free(msghdr);
 		return;
 	}
 
 	for (idx = 0; idx < msghdr->io_len; idx++) {
 		struct msgb *msg = msghdr->msg[idx];
-		int chunk;
+		int chunk = OSMO_MIN(rc, msgb_length(msg));
+		/* chunk contains write completed bytes of msg */
 
-		/* Incomplete write */
-		if (rc > 0 && rc < msgb_length(msg)) {
+		/* If "rc == 0 and msgb_length(msg) > 0", then we had a partial
+		 * write where OS wrote up to exact start point of current msg.
+		 * In that case, we want to skip notifying the user and continue
+		 * below on the "Incomplete write" path to delay further writing.
+		 * If "rc == 0 and msgb_length(msg) == 0", the user specifically
+		 * requested a write() of 0 bytes and 0 can be returned, which
+		 * has its use cases for regular files. In that case user is
+		 * interested in receiving a cb. */
+		if (OSMO_LIKELY(!(rc == 0 && msgb_length(msg) > 0)))
+			iofd_send_completion_cb(iofd, chunk, msghdr, msg);
+
+		/* Incomplete write: */
+		if (chunk < msgb_length(msg)) {
 			/* Keep msg with unsent data only. */
 			msgb_pull(msg, rc);
 			msghdr->iov[idx].iov_len = msgb_length(msg);
@@ -551,35 +598,12 @@
 			return;
 		}
 
-		if (rc >= 0) {
-			chunk = msgb_length(msg);
-			if (rc < chunk)
-				chunk = rc;
-		} else {
-			chunk = rc;
-		}
-
-		/* All other failure and success cases are handled here */
-		switch (msghdr->action) {
-		case IOFD_ACT_WRITE:
-			if (iofd->io_ops.write_cb)
-				iofd->io_ops.write_cb(iofd, chunk, msg);
-			break;
-		case IOFD_ACT_SENDTO:
-			if (iofd->io_ops.sendto_cb)
-				iofd->io_ops.sendto_cb(iofd, chunk, msg, &msghdr->osa);
-			break;
-		case IOFD_ACT_SENDMSG:
-			if (iofd->io_ops.sendmsg_cb)
-				iofd->io_ops.sendmsg_cb(iofd, chunk, msg);
-			break;
-		default:
-			OSMO_ASSERT(0);
-		}
-
 		msgb_free(msghdr->msg[idx]);
 		msghdr->msg[idx] = NULL;
 
+		/* At least current msg (idx) was written, maybe more: rc >= chunk */
+		rc -= chunk;
+
 		/* The user can unregister/close the iofd during callback above. */
 		if (!IOFD_FLAG_ISSET(iofd, IOFD_FLAG_FD_REGISTERED))
 			break;

To view, visit change 42354. To unsubscribe, or for help writing mail filters, visit settings.