Attachment 237838 Details for Bug 267539 – Replace the virtio_scsi tags with an atomic 32 bit counter.

[patch] Replace the virtio_scsi tags with an atomic 32 bit counter.

virtio_scsi_counter.patch (text/plain), 2.97 KB, created by crest on 2022-11-03 12:57:14 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: crest

Created: 2022-11-03 12:57:14 UTC

Size: 2.97 KB

patch

obsolete

>diff --git a/sys/dev/virtio/scsi/virtio_scsi.c b/sys/dev/virtio/scsi/virtio_scsi.c
>index 51d9e5f532f..acba30e6075 100644
>--- a/sys/dev/virtio/scsi/virtio_scsi.c
>+++ b/sys/dev/virtio/scsi/virtio_scsi.c
>@@ -66,6 +66,9 @@ __FBSDID("$FreeBSD$");
> #include <dev/virtio/scsi/virtio_scsi.h>
> #include <dev/virtio/scsi/virtio_scsivar.h>
> 
>+#include <sys/types.h>
>+#include <machine/atomic.h>
>+
> #include "virtio_if.h"
> 
> static int	vtscsi_modevent(module_t, int, void *);
>@@ -1629,6 +1632,8 @@ vtscsi_set_request_lun(struct ccb_hdr *ccbh, uint8_t lun[])
> 	lun[3] = ccbh->target_lun & 0xFF;
> }
> 
>+static uint32_t counter = 0;
>+
> static void
> vtscsi_init_scsi_cmd_req(struct vtscsi_softc *sc, struct ccb_scsiio *csio,
>     struct virtio_scsi_cmd_req *cmd_req)
>@@ -1650,8 +1655,44 @@ vtscsi_init_scsi_cmd_req(struct vtscsi_softc *sc, struct ccb_scsiio *csio,
> 		break;
> 	}
> 
>+	// This code used to use the csio pointer as tag, but
>+	// CTL truncates the tag to 32 bit which resulted in
>+	// overlapping tags under load. Cargo cult the iSCSI
>+	// tag handling.
>+	// The iSCSI code assumes that operations won't be pending
>+	// long enough to still to witness the 32 bit counter
>+	// wrapping. The maximum value was reserved.
>+	// The reservation could be an iSCSI only special case,
>+	// but it shouldn't hurt performance to increment twice
>+	// once every 2**32 commands.
>+	//
>+	// As I'm just hacking on this code without a sufficient
>+	// understanding of its threading and locking I used an
>+	// attomic fetchadd to increment the tag counter.
>+	// The fetchadd operation is an atomic *post*-increment
>+	// aka it returns the old tag value.
>+	//
>+	// TODO: Have this reviewed by someone with an understand of the
>+	// SCSI specifications, VirtIO, and CAM.
>+	// 
>+	// Questions:
>+	//   * The old code used the csio pointer as tag. Are there places
>+	//     expecting to map a tag back to the csio pointer?
>+	//     (Shouldn't have worked on 64 bit systems anyway).
>+	//
>+	//   * Does the tag have to be stable for csio's lifetime e.g.
>+	//     during retries?
>+	//
>+	// Observations:
>+	//   * This change alone didn't prevent the (0x4D, 0xNN)
>+	//     overlapping tag.
>+	uintptr_t tag;
>+	do {
>+		tag = atomic_fetchadd_32(&counter, 1);
>+	} while ( __builtin_expect(tag == UINT32_MAX, false) );
>+
> 	vtscsi_set_request_lun(&csio->ccb_h, cmd_req->lun);
>-	cmd_req->tag = vtscsi_gtoh64(sc, (uintptr_t) csio);
>+	cmd_req->tag = vtscsi_gtoh64(sc, tag);
> 	cmd_req->task_attr = attr;
> 
> 	memcpy(cmd_req->cdb,
>@@ -1664,6 +1705,15 @@ static void
> vtscsi_init_ctrl_tmf_req(struct vtscsi_softc *sc, struct ccb_hdr *ccbh,
>     uint32_t subtype, uintptr_t tag, struct virtio_scsi_ctrl_tmf_req *tmf_req)
> {
>+	// I don't know what if anything I'm breaking by silently replacing
>+	// the tag value with a second atomic 32 bit counter.
>+	//
>+	// Silence compiler warnings about unused arguments.
>+	(void)tag;
>+
>+	do {
>+		tag = atomic_fetchadd_32(&counter, 1);
>+	} while ( __builtin_expect(tag == UINT32_MAX, false) );
> 
> 	vtscsi_set_request_lun(ccbh, tmf_req->lun);
>

Actions: View | Diff

Attachments on bug 267539: 237838