[PATCH 0/9] vhost-scsi: log write descriptors for live migration (and two bugfix)

[PATCH 0/9] vhost-scsi: log write descriptors for live migration (and two bugfix)

[Dongli Zhang]

The live migration with vhost-scsi has been enabled by QEMU commit
b3e89c941a85 ("vhost-scsi: Allow user to enable migration"), which
thoroughly explains the workflow that QEMU collaborates with vhost-scsi on
the live migration.

Although it logs dirty data for the used ring, it doesn't log any write
descriptor (VRING_DESC_F_WRITE).

In comparison, vhost-net logs write descriptors via vhost_log_write(). The
SPDK (vhost-user-scsi backend) also logs write descriptors via
vhost_log_req_desc().

As a result, there is likely data mismatch between memory and vhost-scsi
disk during the live migration.

1. Suppose there is high workload and high memory usage. Suppose some
systemd userspace pages are swapped out to the swap disk.

2. Upon request from systemd, the kernel reads some pages from the swap
disk to the memory via vhost-scsi.

3. Although those userspace pages' data are updated, they are not marked as
dirty by vhost-scsi (this is the bug). They are not going to migrate to the
target host during memory transfer iterations.

4. Suppose systemd doesn't write to those pages any longer. Those pages
never get the chance to be dirty or migrated any longer.

5. Once the guest VM is resumed on the target host, because of the lack of
those dirty pages' data, the systemd may run into abnormal status, i.e.,
there may be systemd segfault.

Log all write descriptors to fix the issue.

In addition, the patchset also fixes two bugs in vhost-scsi.

Dongli Zhang (log descriptor, suggested by Joao Martins):
  vhost: modify vhost_log_write() for broader users
  vhost-scsi: adjust vhost_scsi_get_desc() to log vring descriptors
  vhost-scsi: cache log buffer in I/O queue vhost_scsi_cmd
  vhost-scsi: log I/O queue write descriptors
  vhost-scsi: log control queue write descriptors
  vhost-scsi: log event queue write descriptors
  vhost: add WARNING if log_num is more than limit

Dongli Zhang (vhost-scsi bugfix):
  vhost-scsi: protect vq->log_used with vq->mutex
  vhost-scsi: Fix vhost_scsi_send_bad_target()

 drivers/vhost/net.c   |   2 +-
 drivers/vhost/scsi.c  | 191 +++++++++++++++++++++++++++++++++++++++------
 drivers/vhost/vhost.c |  46 ++++++++---
 drivers/vhost/vhost.h |   2 +-
 4 files changed, 206 insertions(+), 35 deletions(-)


base-commit: 5c8c229261f14159b54b9a32f12e5fa89d88b905

Thank you very much!

Dongli Zhang

[PATCH 1/9] vhost: modify vhost_log_write() for broader users

[Dongli Zhang]

Currently, the only user of vhost_log_write() is vhost-net. The 'len'
argument prevents logging of pages that are not tainted by the RX path.

Adjustments are needed since more drivers (i.e. vhost-scsi) begin using
vhost_log_write(). So far vhost-net RX path may only partially use pages
shared by the last vring descriptor. Unlike vhost-net, vhost-scsi always
logs all pages shared via vring descriptors. To accommodate this, a new
argument 'partial' is introduced. This argument works alongside 'len' to
indicate whether the driver should log all pages of a vring descriptor, or
only pages that are tainted by the driver.

In addition, removes BUG().

Suggested-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/vhost/net.c   |  2 +-
 drivers/vhost/vhost.c | 28 +++++++++++++++++-----------
 drivers/vhost/vhost.h |  2 +-
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index b9b9e9d40951..0e5d82bfde76 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -1219,7 +1219,7 @@ static void handle_rx(struct vhost_net *net)
 		if (nvq->done_idx > VHOST_NET_BATCH)
 			vhost_net_signal_used(nvq);
 		if (unlikely(vq_log))
-			vhost_log_write(vq, vq_log, log, vhost_len,
+			vhost_log_write(vq, vq_log, log, vhost_len, true,
 					vq->iov, in);
 		total_len += vhost_len;
 	} while (likely(!vhost_exceeds_weight(vq, ++recv_pkts, total_len)));
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 9ac25d08f473..db3b30aba940 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2304,8 +2304,14 @@ static int log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
 	return 0;
 }
 
-int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len, struct iovec *iov, int count)
+/*
+ * 'len' is used only when 'partial' is true, to indicate whether the
+ * entire length of each descriptor is logged.
+ */
+int vhost_log_write(struct vhost_virtqueue *vq,
+		    struct vhost_log *log, unsigned int log_num,
+		    u64 len, bool partial,
+		    struct iovec *iov, int count)
 {
 	int i, r;
 
@@ -2323,19 +2329,19 @@ int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
 	}
 
 	for (i = 0; i < log_num; ++i) {
-		u64 l = min(log[i].len, len);
+		u64 l = partial ? min(log[i].len, len) : log[i].len;
+
 		r = log_write(vq->log_base, log[i].addr, l);
 		if (r < 0)
 			return r;
-		len -= l;
-		if (!len) {
-			if (vq->log_ctx)
-				eventfd_signal(vq->log_ctx);
-			return 0;
-		}
+
+		if (partial)
+			len -= l;
 	}
-	/* Length written exceeds what we have stored. This is a bug. */
-	BUG();
+
+	if (vq->log_ctx)
+		eventfd_signal(vq->log_ctx);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(vhost_log_write);
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index bb75a292d50c..5de5941988fe 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -224,7 +224,7 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
 bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
 
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len,
+		    unsigned int log_num, u64 len, bool partial,
 		    struct iovec *iov, int count);
 int vq_meta_prefetch(struct vhost_virtqueue *vq);
 
-- 
2.39.3

[PATCH 2/9] vhost-scsi: adjust vhost_scsi_get_desc() to log vring descriptors

[Dongli Zhang]

Adjust vhost_scsi_get_desc() to facilitate logging of vring descriptors.

Add new arguments to allow passing the log buffer and length to
vhost_get_vq_desc().

In addition, reset 'log_num' since vhost_get_vq_desc() may reset it only
after certain condition checks.

Suggested-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/vhost/scsi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 718fa4e0b31e..ee2310555740 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -950,13 +950,17 @@ vhost_scsi_send_bad_target(struct vhost_scsi *vs,
 
 static int
 vhost_scsi_get_desc(struct vhost_scsi *vs, struct vhost_virtqueue *vq,
-		    struct vhost_scsi_ctx *vc)
+		    struct vhost_scsi_ctx *vc,
+		    struct vhost_log *log, unsigned int *log_num)
 {
 	int ret = -ENXIO;
 
+	if (likely(log_num))
+		*log_num = 0;
+
 	vc->head = vhost_get_vq_desc(vq, vq->iov,
 				     ARRAY_SIZE(vq->iov), &vc->out, &vc->in,
-				     NULL, NULL);
+				     log, log_num);
 
 	pr_debug("vhost_get_vq_desc: head: %d, out: %u in: %u\n",
 		 vc->head, vc->out, vc->in);
@@ -1086,7 +1090,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 	vhost_disable_notify(&vs->dev, vq);
 
 	do {
-		ret = vhost_scsi_get_desc(vs, vq, &vc);
+		ret = vhost_scsi_get_desc(vs, vq, &vc, NULL, NULL);
 		if (ret)
 			goto err;
 
@@ -1411,7 +1415,7 @@ vhost_scsi_ctl_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 	vhost_disable_notify(&vs->dev, vq);
 
 	do {
-		ret = vhost_scsi_get_desc(vs, vq, &vc);
+		ret = vhost_scsi_get_desc(vs, vq, &vc, NULL, NULL);
 		if (ret)
 			goto err;
 
-- 
2.39.3

[PATCH 3/9] vhost-scsi: cache log buffer in I/O queue vhost_scsi_cmd

[Dongli Zhang]

The vhost-scsi I/O queue uses vhost_scsi_cmd. Pre-allocate the log buffer
during vhost_scsi_cmd allocation, and free it when vhost_scsi_cmd is
reclaimed.

The cached log buffer will be uses in upcoming patches to log write
descriptors for the I/O queue. The core idea is to cache the log in the
per-command log buffer in the submission path, and use them to log write
descriptors in the completion path.

Suggested-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/vhost/scsi.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index ee2310555740..5e6221cbbe9e 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -98,6 +98,11 @@ struct vhost_scsi_cmd {
 	unsigned char tvc_cdb[VHOST_SCSI_MAX_CDB_SIZE];
 	/* Sense buffer that will be mapped into outgoing status */
 	unsigned char tvc_sense_buf[TRANSPORT_SENSE_BUFFER];
+	/*
+	 * Dirty write descriptors of this command.
+	 */
+	struct vhost_log *tvc_log;
+	unsigned int tvc_log_num;
 	/* Completed commands list, serviced from vhost worker thread */
 	struct llist_node tvc_completion_list;
 	/* Used to track inflight cmd */
@@ -619,6 +624,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
 	struct vhost_scsi_nexus *tv_nexus;
 	struct scatterlist *sg, *prot_sg;
 	struct iovec *tvc_resp_iov;
+	struct vhost_log *log;
 	struct page **pages;
 	int tag;
 
@@ -639,6 +645,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
 	prot_sg = cmd->tvc_prot_sgl;
 	pages = cmd->tvc_upages;
 	tvc_resp_iov = cmd->tvc_resp_iov;
+	log = cmd->tvc_log;
 	memset(cmd, 0, sizeof(*cmd));
 	cmd->tvc_sgl = sg;
 	cmd->tvc_prot_sgl = prot_sg;
@@ -652,6 +659,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
 	cmd->tvc_nexus = tv_nexus;
 	cmd->inflight = vhost_scsi_get_inflight(vq);
 	cmd->tvc_resp_iov = tvc_resp_iov;
+	cmd->tvc_log = log;
 
 	memcpy(cmd->tvc_cdb, cdb, VHOST_SCSI_MAX_CDB_SIZE);
 
@@ -1604,6 +1612,7 @@ static void vhost_scsi_destroy_vq_cmds(struct vhost_virtqueue *vq)
 		kfree(tv_cmd->tvc_prot_sgl);
 		kfree(tv_cmd->tvc_upages);
 		kfree(tv_cmd->tvc_resp_iov);
+		kfree(tv_cmd->tvc_log);
 	}
 
 	sbitmap_free(&svq->scsi_tags);
@@ -1666,6 +1675,18 @@ static int vhost_scsi_setup_vq_cmds(struct vhost_virtqueue *vq, int max_cmds)
 			pr_err("Unable to allocate tv_cmd->tvc_prot_sgl\n");
 			goto out;
 		}
+
+		/*
+		 * tv_cmd->tvc_log and vq->log need to have the same max
+		 * length.
+		 */
+		tv_cmd->tvc_log = kcalloc(vq->dev->iov_limit,
+					  sizeof(struct vhost_log),
+					  GFP_KERNEL);
+		if (!tv_cmd->tvc_log) {
+			pr_err("Unable to allocate tv_cmd->tvc_log\n");
+			goto out;
+		}
 	}
 	return 0;
 out:
-- 
2.39.3

[PATCH 4/9] vhost-scsi: log I/O queue write descriptors

[Dongli Zhang]

Log write descriptors for the I/O queue, leveraging vhost_scsi_get_desc()
and vhost_get_vq_desc() to retrieve the array of write descriptors to
obtain the log buffer.

In addition, introduce a vhost-scsi specific function to log vring
descriptors. In this function, the 'partial' argument is set to false, and
the 'len' argument is set to 0, because vhost-scsi always logs all pages
shared by a vring descriptor. Add WARN_ON_ONCE() since vhost-scsi doesn't
support VIRTIO_F_ACCESS_PLATFORM.

Store the log buffer during the submission path and log it in the
completion path. Logging is also required in the error handling path of the
submission process.

While the submission path is already protected by vq->mutex, the completion
path also requires this lock for synchronization.

Suggested-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/vhost/scsi.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 5e6221cbbe9e..d678eaf4ca68 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -329,6 +329,24 @@ static int vhost_scsi_check_prot_fabric_only(struct se_portal_group *se_tpg)
 	return tpg->tv_fabric_prot_type;
 }
 
+static void vhost_scsi_log_write(struct vhost_virtqueue *vq,
+				 struct vhost_log *log,
+				 unsigned int log_num)
+{
+	if (likely(!log || !log_num))
+		return;
+
+	if (likely(!vhost_has_feature(vq, VHOST_F_LOG_ALL)))
+		return;
+
+	/*
+	 * vhost-scsi doesn't support VIRTIO_F_ACCESS_PLATFORM.
+	 * No requirement for vq->iotlb case.
+	 */
+	WARN_ON_ONCE(unlikely(vq->iotlb));
+	vhost_log_write(vq, log, log_num, 0, false, NULL, 0);
+}
+
 static void vhost_scsi_release_cmd_res(struct se_cmd *se_cmd)
 {
 	struct vhost_scsi_cmd *tv_cmd = container_of(se_cmd,
@@ -606,6 +624,13 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
 		} else
 			pr_err("Faulted on virtio_scsi_cmd_resp\n");
 
+		if (unlikely(cmd->tvc_log_num)) {
+			mutex_lock(&cmd->tvc_vq->mutex);
+			vhost_scsi_log_write(cmd->tvc_vq, cmd->tvc_log,
+					     cmd->tvc_log_num);
+			mutex_unlock(&cmd->tvc_vq->mutex);
+		}
+
 		vhost_scsi_release_cmd_res(se_cmd);
 	}
 
@@ -1082,6 +1107,8 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 	u8 task_attr;
 	bool t10_pi = vhost_has_feature(vq, VIRTIO_SCSI_F_T10_PI);
 	void *cdb;
+	struct vhost_log *vq_log;
+	unsigned int log_num;
 
 	mutex_lock(&vq->mutex);
 	/*
@@ -1097,8 +1124,11 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 
 	vhost_disable_notify(&vs->dev, vq);
 
+	vq_log = unlikely(vhost_has_feature(vq, VHOST_F_LOG_ALL)) ?
+		vq->log : NULL;
+
 	do {
-		ret = vhost_scsi_get_desc(vs, vq, &vc, NULL, NULL);
+		ret = vhost_scsi_get_desc(vs, vq, &vc, vq_log, &log_num);
 		if (ret)
 			goto err;
 
@@ -1238,6 +1268,11 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 			cmd->tvc_resp_iov[i] = vq->iov[vc.out + i];
 		cmd->tvc_in_iovs = vc.in;
 
+		if (unlikely(vq_log && log_num)) {
+			memcpy(cmd->tvc_log, vq->log, sizeof(*cmd->tvc_log) * log_num);
+			cmd->tvc_log_num = log_num;
+		}
+
 		pr_debug("vhost_scsi got command opcode: %#02x, lun: %d\n",
 			 cmd->tvc_cdb[0], cmd->tvc_lun);
 		pr_debug("cmd: %p exp_data_len: %d, prot_bytes: %d data_direction:"
@@ -1269,8 +1304,10 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 		 */
 		if (ret == -ENXIO)
 			break;
-		else if (ret == -EIO)
+		else if (ret == -EIO) {
 			vhost_scsi_send_bad_target(vs, vq, vc.head, vc.out);
+			vhost_scsi_log_write(vq, vq_log, log_num);
+		}
 	} while (likely(!vhost_exceeds_weight(vq, ++c, 0)));
 out:
 	mutex_unlock(&vq->mutex);
-- 
2.39.3

[PATCH 5/9] vhost-scsi: log control queue write descriptors

[Dongli Zhang]

Log write descriptors for the control queue, leveraging
vhost_scsi_get_desc() and vhost_get_vq_desc() to retrieve the array of
write descriptors to obtain the log buffer.

For Task Management Requests, similar to the I/O queue, store the log
buffer during the submission path and log it in the completion or error
handling path.

For Asynchronous Notifications, only the submission path is involved.

Suggested-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/vhost/scsi.c | 51 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index d678eaf4ca68..21c2d07b806a 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -225,6 +225,12 @@ struct vhost_scsi_tmf {
 	struct iovec resp_iov;
 	int in_iovs;
 	int vq_desc;
+
+	/*
+	 * Dirty write descriptors of this command.
+	 */
+	struct vhost_log *tmf_log;
+	unsigned int tmf_log_num;
 };
 
 /*
@@ -378,6 +384,11 @@ static void vhost_scsi_release_tmf_res(struct vhost_scsi_tmf *tmf)
 {
 	struct vhost_scsi_inflight *inflight = tmf->inflight;
 
+	if (tmf->tmf_log_num) {
+		kfree(tmf->tmf_log);
+		tmf->tmf_log_num = 0;
+	}
+
 	kfree(tmf);
 	vhost_scsi_put_inflight(inflight);
 }
@@ -1348,6 +1359,14 @@ static void vhost_scsi_tmf_resp_work(struct vhost_work *work)
 
 	vhost_scsi_send_tmf_resp(tmf->vhost, &tmf->svq->vq, tmf->in_iovs,
 				 tmf->vq_desc, &tmf->resp_iov, resp_code);
+
+	if (unlikely(tmf->tmf_log_num)) {
+		mutex_lock(&tmf->svq->vq.mutex);
+		vhost_scsi_log_write(&tmf->svq->vq, tmf->tmf_log,
+				     tmf->tmf_log_num);
+		mutex_unlock(&tmf->svq->vq.mutex);
+	}
+
 	vhost_scsi_release_tmf_res(tmf);
 }
 
@@ -1369,7 +1388,8 @@ static void
 vhost_scsi_handle_tmf(struct vhost_scsi *vs, struct vhost_scsi_tpg *tpg,
 		      struct vhost_virtqueue *vq,
 		      struct virtio_scsi_ctrl_tmf_req *vtmf,
-		      struct vhost_scsi_ctx *vc)
+		      struct vhost_scsi_ctx *vc,
+		      struct vhost_log *log, unsigned int log_num)
 {
 	struct vhost_scsi_virtqueue *svq = container_of(vq,
 					struct vhost_scsi_virtqueue, vq);
@@ -1397,6 +1417,16 @@ vhost_scsi_handle_tmf(struct vhost_scsi *vs, struct vhost_scsi_tpg *tpg,
 	tmf->in_iovs = vc->in;
 	tmf->inflight = vhost_scsi_get_inflight(vq);
 
+	if (unlikely(log && log_num)) {
+		tmf->tmf_log = kmalloc_array(log_num, sizeof(*tmf->tmf_log),
+					     GFP_KERNEL);
+		if (tmf->tmf_log) {
+			memcpy(tmf->tmf_log, log, sizeof(*tmf->tmf_log) * log_num);
+			tmf->tmf_log_num = log_num;
+		} else
+			pr_err("vhost_scsi tmf log allocation error\n");
+	}
+
 	if (target_submit_tmr(&tmf->se_cmd, tpg->tpg_nexus->tvn_se_sess, NULL,
 			      vhost_buf_to_lun(vtmf->lun), NULL,
 			      TMR_LUN_RESET, GFP_KERNEL, 0,
@@ -1410,6 +1440,7 @@ vhost_scsi_handle_tmf(struct vhost_scsi *vs, struct vhost_scsi_tpg *tpg,
 send_reject:
 	vhost_scsi_send_tmf_resp(vs, vq, vc->in, vc->head, &vq->iov[vc->out],
 				 VIRTIO_SCSI_S_FUNCTION_REJECTED);
+	vhost_scsi_log_write(vq, log, log_num);
 }
 
 static void
@@ -1446,6 +1477,8 @@ vhost_scsi_ctl_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 	struct vhost_scsi_ctx vc;
 	size_t typ_size;
 	int ret, c = 0;
+	struct vhost_log *vq_log;
+	unsigned int log_num;
 
 	mutex_lock(&vq->mutex);
 	/*
@@ -1459,8 +1492,11 @@ vhost_scsi_ctl_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 
 	vhost_disable_notify(&vs->dev, vq);
 
+	vq_log = unlikely(vhost_has_feature(vq, VHOST_F_LOG_ALL)) ?
+		vq->log : NULL;
+
 	do {
-		ret = vhost_scsi_get_desc(vs, vq, &vc, NULL, NULL);
+		ret = vhost_scsi_get_desc(vs, vq, &vc, vq_log, &log_num);
 		if (ret)
 			goto err;
 
@@ -1524,9 +1560,12 @@ vhost_scsi_ctl_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 			goto err;
 
 		if (v_req.type == VIRTIO_SCSI_T_TMF)
-			vhost_scsi_handle_tmf(vs, tpg, vq, &v_req.tmf, &vc);
-		else
+			vhost_scsi_handle_tmf(vs, tpg, vq, &v_req.tmf, &vc,
+					      vq_log, log_num);
+		else {
 			vhost_scsi_send_an_resp(vs, vq, &vc);
+			vhost_scsi_log_write(vq, vq_log, log_num);
+		}
 err:
 		/*
 		 * ENXIO:  No more requests, or read error, wait for next kick
@@ -1536,8 +1575,10 @@ vhost_scsi_ctl_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 		 */
 		if (ret == -ENXIO)
 			break;
-		else if (ret == -EIO)
+		else if (ret == -EIO) {
 			vhost_scsi_send_bad_target(vs, vq, vc.head, vc.out);
+			vhost_scsi_log_write(vq, vq_log, log_num);
+		}
 	} while (likely(!vhost_exceeds_weight(vq, ++c, 0)));
 out:
 	mutex_unlock(&vq->mutex);
-- 
2.39.3

[PATCH 6/9] vhost-scsi: log event queue write descriptors

[Dongli Zhang]

Log write descriptors for the event queue, leveraging vhost_get_vq_desc()
to retrieve the array of write descriptors to obtain the log buffer.

There is only one path for event queue.

Suggested-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/vhost/scsi.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 21c2d07b806a..40268b88f470 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -498,6 +498,8 @@ vhost_scsi_do_evt_work(struct vhost_scsi *vs, struct vhost_scsi_evt *evt)
 	struct vhost_virtqueue *vq = &vs->vqs[VHOST_SCSI_VQ_EVT].vq;
 	struct virtio_scsi_event *event = &evt->event;
 	struct virtio_scsi_event __user *eventp;
+	struct vhost_log *vq_log;
+	unsigned int log_num;
 	unsigned out, in;
 	int head, ret;
 
@@ -508,9 +510,19 @@ vhost_scsi_do_evt_work(struct vhost_scsi *vs, struct vhost_scsi_evt *evt)
 
 again:
 	vhost_disable_notify(&vs->dev, vq);
+
+	vq_log = unlikely(vhost_has_feature(vq, VHOST_F_LOG_ALL)) ?
+		vq->log : NULL;
+
+	/*
+	 * Reset 'log_num' since vhost_get_vq_desc() may reset it only
+	 * after certain condition checks.
+	 */
+	log_num = 0;
+
 	head = vhost_get_vq_desc(vq, vq->iov,
 			ARRAY_SIZE(vq->iov), &out, &in,
-			NULL, NULL);
+			vq_log, &log_num);
 	if (head < 0) {
 		vs->vs_events_missed = true;
 		return;
@@ -540,6 +552,8 @@ vhost_scsi_do_evt_work(struct vhost_scsi *vs, struct vhost_scsi_evt *evt)
 		vhost_add_used_and_signal(&vs->dev, vq, head, 0);
 	else
 		vq_err(vq, "Faulted on vhost_scsi_send_event\n");
+
+	vhost_scsi_log_write(vq, vq_log, log_num);
 }
 
 static void vhost_scsi_complete_events(struct vhost_scsi *vs, bool drop)
-- 
2.39.3

[PATCH 7/9] vhost: add WARNING if log_num is more than limit

[Dongli Zhang]

Since long time ago, the only user of vq->log is vhost-net. The concern is
to add support for more devices (i.e. vhost-scsi or vsock) may reveals
unknown issue in the vhost API. Add a WARNING.

Suggested-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/vhost/vhost.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index db3b30aba940..8368370b40f7 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2553,6 +2553,15 @@ static int get_indirect(struct vhost_virtqueue *vq,
 		if (access == VHOST_ACCESS_WO) {
 			*in_num += ret;
 			if (unlikely(log && ret)) {
+				/*
+				 * Since long time ago, the only user of
+				 * vq->log is vhost-net. The concern is to
+				 * add support for more devices (i.e.
+				 * vhost-scsi or vsock) may reveals unknown
+				 * issue in the vhost API. Add a WARNING.
+				 */
+				WARN_ON_ONCE(*log_num >= vq->dev->iov_limit);
+
 				log[*log_num].addr = vhost64_to_cpu(vq, desc.addr);
 				log[*log_num].len = vhost32_to_cpu(vq, desc.len);
 				++*log_num;
@@ -2673,6 +2682,15 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
 			 * increment that count. */
 			*in_num += ret;
 			if (unlikely(log && ret)) {
+				/*
+				 * Since long time ago, the only user of
+				 * vq->log is vhost-net. The concern is to
+				 * add support for more devices (i.e.
+				 * vhost-scsi or vsock) may reveals unknown
+				 * issue in the vhost API. Add a WARNING.
+				 */
+				WARN_ON_ONCE(*log_num >= vq->dev->iov_limit);
+
 				log[*log_num].addr = vhost64_to_cpu(vq, desc.addr);
 				log[*log_num].len = vhost32_to_cpu(vq, desc.len);
 				++*log_num;
-- 
2.39.3

[PATCH 8/9] vhost-scsi: protect vq->log_used with vq->mutex

[Dongli Zhang]

The vhost-scsi completion path may access vq->log_base when vq->log_used is
already set to false.

    vhost-thread                       QEMU-thread

vhost_scsi_complete_cmd_work()
-> vhost_add_used()
   -> vhost_add_used_n()
      if (unlikely(vq->log_used))
                                      QEMU disables vq->log_used
                                      via VHOST_SET_VRING_ADDR.
                                      mutex_lock(&vq->mutex);
                                      vq->log_used = false now!
                                      mutex_unlock(&vq->mutex);

				      QEMU gfree(vq->log_base)
        log_used()
        -> log_write(vq->log_base)

Assuming the VMM is QEMU. The vq->log_base is from QEMU userpace and can be
reclaimed via gfree(). As a result, this causes invalid memory writes to
QEMU userspace.

The control queue path has the same issue.

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/vhost/scsi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 40268b88f470..3b87d698adaf 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -645,7 +645,9 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
 		if (likely(ret == sizeof(v_rsp))) {
 			signal = true;
 
+			mutex_lock(&cmd->tvc_vq->mutex);
 			vhost_add_used(cmd->tvc_vq, cmd->tvc_vq_desc, 0);
+			mutex_unlock(&cmd->tvc_vq->mutex);
 		} else
 			pr_err("Faulted on virtio_scsi_cmd_resp\n");
 
@@ -1371,8 +1373,10 @@ static void vhost_scsi_tmf_resp_work(struct vhost_work *work)
 	else
 		resp_code = VIRTIO_SCSI_S_FUNCTION_REJECTED;
 
+	mutex_lock(&tmf->svq->vq.mutex);
 	vhost_scsi_send_tmf_resp(tmf->vhost, &tmf->svq->vq, tmf->in_iovs,
 				 tmf->vq_desc, &tmf->resp_iov, resp_code);
+	mutex_unlock(&tmf->svq->vq.mutex);
 
 	if (unlikely(tmf->tmf_log_num)) {
 		mutex_lock(&tmf->svq->vq.mutex);
-- 
2.39.3

[PATCH 9/9] vhost-scsi: Fix vhost_scsi_send_bad_target()

[Dongli Zhang]

Although the support of VIRTIO_F_ANY_LAYOUT + VIRTIO_F_VERSION_1 was
signaled by the commit 664ed90e621c ("vhost/scsi: Set
VIRTIO_F_ANY_LAYOUT + VIRTIO_F_VERSION_1 feature bits"),
vhost_scsi_send_bad_target() still assumes the response in a single
descriptor.

In addition, although vhost_scsi_send_bad_target() is used by both I/O
queue and control queue, the response header is always
virtio_scsi_cmd_resp. It is required to use virtio_scsi_ctrl_tmf_resp or
virtio_scsi_ctrl_an_resp for control queue.

Fixes: 664ed90e621c ("vhost/scsi: Set VIRTIO_F_ANY_LAYOUT + VIRTIO_F_VERSION_1 feature bits")
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/vhost/scsi.c | 50 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 3b87d698adaf..6aa3f3ad2695 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -989,23 +989,46 @@ static void vhost_scsi_target_queue_cmd(struct vhost_scsi_cmd *cmd)
 	target_submit(se_cmd);
 }
 
+#define TYPE_IO_CMD	0
+#define TYPE_CTRL_TMF	1
+#define TYPE_CTRL_AN	2
+
 static void
 vhost_scsi_send_bad_target(struct vhost_scsi *vs,
 			   struct vhost_virtqueue *vq,
-			   int head, unsigned out)
+			   struct vhost_scsi_ctx *vc, int type)
 {
-	struct virtio_scsi_cmd_resp __user *resp;
-	struct virtio_scsi_cmd_resp rsp;
+	union {
+		struct virtio_scsi_cmd_resp cmd;
+		struct virtio_scsi_ctrl_tmf_resp tmf;
+		struct virtio_scsi_ctrl_an_resp an;
+	} resp;
+	struct iov_iter iov_iter;
+	size_t resp_size;
 	int ret;
 
-	memset(&rsp, 0, sizeof(rsp));
-	rsp.response = VIRTIO_SCSI_S_BAD_TARGET;
-	resp = vq->iov[out].iov_base;
-	ret = __copy_to_user(resp, &rsp, sizeof(rsp));
-	if (!ret)
-		vhost_add_used_and_signal(&vs->dev, vq, head, 0);
+	memset(&resp, 0, sizeof(resp));
+
+	if (type == TYPE_IO_CMD) {
+		resp_size = sizeof(struct virtio_scsi_cmd_resp);
+		resp.cmd.response = VIRTIO_SCSI_S_BAD_TARGET;
+	} else if (type == TYPE_CTRL_TMF) {
+		resp_size = sizeof(struct virtio_scsi_ctrl_tmf_resp);
+		resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
+	} else {
+		resp_size = sizeof(struct virtio_scsi_ctrl_an_resp);
+		resp.an.response = VIRTIO_SCSI_S_BAD_TARGET;
+	}
+
+	iov_iter_init(&iov_iter, ITER_DEST, &vq->iov[vc->out], vc->in,
+		      resp_size);
+
+	ret = copy_to_iter(&resp, resp_size, &iov_iter);
+
+	if (likely(ret == resp_size))
+		vhost_add_used_and_signal(&vs->dev, vq, vc->head, 0);
 	else
-		pr_err("Faulted on virtio_scsi_cmd_resp\n");
+		pr_err("Faulted on virtio scsi type=%d\n", type);
 }
 
 static int
@@ -1332,7 +1355,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 		if (ret == -ENXIO)
 			break;
 		else if (ret == -EIO) {
-			vhost_scsi_send_bad_target(vs, vq, vc.head, vc.out);
+			vhost_scsi_send_bad_target(vs, vq, &vc, TYPE_IO_CMD);
 			vhost_scsi_log_write(vq, vq_log, log_num);
 		}
 	} while (likely(!vhost_exceeds_weight(vq, ++c, 0)));
@@ -1594,7 +1617,10 @@ vhost_scsi_ctl_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 		if (ret == -ENXIO)
 			break;
 		else if (ret == -EIO) {
-			vhost_scsi_send_bad_target(vs, vq, vc.head, vc.out);
+			vhost_scsi_send_bad_target(vs, vq, &vc,
+						   v_req.type == VIRTIO_SCSI_T_TMF ?
+						   TYPE_CTRL_TMF :
+						   TYPE_CTRL_AN);
 			vhost_scsi_log_write(vq, vq_log, log_num);
 		}
 	} while (likely(!vhost_exceeds_weight(vq, ++c, 0)));
-- 
2.39.3

Re: [PATCH 3/9] vhost-scsi: cache log buffer in I/O queue vhost_scsi_cmd

[Mike Christie]

On 2/7/25 12:41 PM, Dongli Zhang wrote:
> The vhost-scsi I/O queue uses vhost_scsi_cmd. Pre-allocate the log buffer
> during vhost_scsi_cmd allocation, and free it when vhost_scsi_cmd is
> reclaimed.
> 
> The cached log buffer will be uses in upcoming patches to log write
> descriptors for the I/O queue. The core idea is to cache the log in the
> per-command log buffer in the submission path, and use them to log write
> descriptors in the completion path.
> 
> Suggested-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
>  drivers/vhost/scsi.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index ee2310555740..5e6221cbbe9e 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -98,6 +98,11 @@ struct vhost_scsi_cmd {
>  	unsigned char tvc_cdb[VHOST_SCSI_MAX_CDB_SIZE];
>  	/* Sense buffer that will be mapped into outgoing status */
>  	unsigned char tvc_sense_buf[TRANSPORT_SENSE_BUFFER];
> +	/*
> +	 * Dirty write descriptors of this command.
> +	 */
> +	struct vhost_log *tvc_log;
> +	unsigned int tvc_log_num;
>  	/* Completed commands list, serviced from vhost worker thread */
>  	struct llist_node tvc_completion_list;
>  	/* Used to track inflight cmd */
> @@ -619,6 +624,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
>  	struct vhost_scsi_nexus *tv_nexus;
>  	struct scatterlist *sg, *prot_sg;
>  	struct iovec *tvc_resp_iov;
> +	struct vhost_log *log;
>  	struct page **pages;
>  	int tag;
>  
> @@ -639,6 +645,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
>  	prot_sg = cmd->tvc_prot_sgl;
>  	pages = cmd->tvc_upages;
>  	tvc_resp_iov = cmd->tvc_resp_iov;
> +	log = cmd->tvc_log;
>  	memset(cmd, 0, sizeof(*cmd));
>  	cmd->tvc_sgl = sg;
>  	cmd->tvc_prot_sgl = prot_sg;
> @@ -652,6 +659,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
>  	cmd->tvc_nexus = tv_nexus;
>  	cmd->inflight = vhost_scsi_get_inflight(vq);
>  	cmd->tvc_resp_iov = tvc_resp_iov;
> +	cmd->tvc_log = log;
>  
>  	memcpy(cmd->tvc_cdb, cdb, VHOST_SCSI_MAX_CDB_SIZE);
>  
> @@ -1604,6 +1612,7 @@ static void vhost_scsi_destroy_vq_cmds(struct vhost_virtqueue *vq)
>  		kfree(tv_cmd->tvc_prot_sgl);
>  		kfree(tv_cmd->tvc_upages);
>  		kfree(tv_cmd->tvc_resp_iov);
> +		kfree(tv_cmd->tvc_log);
>  	}
>  
>  	sbitmap_free(&svq->scsi_tags);
> @@ -1666,6 +1675,18 @@ static int vhost_scsi_setup_vq_cmds(struct vhost_virtqueue *vq, int max_cmds)
>  			pr_err("Unable to allocate tv_cmd->tvc_prot_sgl\n");
>  			goto out;
>  		}
> +
> +		/*
> +		 * tv_cmd->tvc_log and vq->log need to have the same max
> +		 * length.
> +		 */
> +		tv_cmd->tvc_log = kcalloc(vq->dev->iov_limit,
> +					  sizeof(struct vhost_log),
> +					  GFP_KERNEL);

VHOST_F_LOG_ALL is normally set when the migration starts right?

I mean it's done before the initial setup when the above code is run so
is it possible to do the allocation when vhost_scsi_set_features is passed
VHOST_F_LOG_ALL? We then don't allocate a bunch of mem for a feature that
may never be used.

Re: [PATCH 5/9] vhost-scsi: log control queue write descriptors

[Mike Christie]

On 2/7/25 12:41 PM, Dongli Zhang wrote:
> @@ -378,6 +384,11 @@ static void vhost_scsi_release_tmf_res(struct vhost_scsi_tmf *tmf)
>  {
>  	struct vhost_scsi_inflight *inflight = tmf->inflight;
>  
> +	if (tmf->tmf_log_num) {
> +		kfree(tmf->tmf_log);
> +		tmf->tmf_log_num = 0;

Just a small nit. We can drop this line above. We free
the struct on the next line so it's not useful.

> +	}
> +
>  	kfree(tmf);

Re: [PATCH 0/9] vhost-scsi: log write descriptors for live migration (and two bugfix)

[dongli.zhang]

Thanks to the suggestion from Mike, I am going re-send v2 with:

1. Re-base on top of the below patchset.

[PATCH v2 0/8] vhost-scsi: Memory reduction patches
https://yhbt.net/lore/target-devel/20241203191705.19431-1-michael.christie@oracle.com/

The patchset can clean apply/build on top of the commit 87a132e73910
("Merge tag 'mm-hotfixes-stable-2025-02-19-17-49' of
git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm").


2. Don't allocate all per-cmd log buffer until VHOST_F_LOG_ALL is set.

Either to take advantage of vhost_scsi_set_features(), or follow the idea
of below patch.

[PATCH v2 5/8] vhost-scsi: Dynamically allocate scatterlists
https://yhbt.net/lore/target-devel/20241203191705.19431-6-michael.christie@oracle.com/

Thank you very much!

Dongli Zhang

On 2/7/25 10:41 AM, Dongli Zhang wrote:
> The live migration with vhost-scsi has been enabled by QEMU commit
> b3e89c941a85 ("vhost-scsi: Allow user to enable migration"), which
> thoroughly explains the workflow that QEMU collaborates with vhost-scsi on
> the live migration.
> 
> Although it logs dirty data for the used ring, it doesn't log any write
> descriptor (VRING_DESC_F_WRITE).
> 
> In comparison, vhost-net logs write descriptors via vhost_log_write(). The
> SPDK (vhost-user-scsi backend) also logs write descriptors via
> vhost_log_req_desc().
> 
> As a result, there is likely data mismatch between memory and vhost-scsi
> disk during the live migration.
> 
> 1. Suppose there is high workload and high memory usage. Suppose some
> systemd userspace pages are swapped out to the swap disk.
> 
> 2. Upon request from systemd, the kernel reads some pages from the swap
> disk to the memory via vhost-scsi.
> 
> 3. Although those userspace pages' data are updated, they are not marked as
> dirty by vhost-scsi (this is the bug). They are not going to migrate to the
> target host during memory transfer iterations.
> 
> 4. Suppose systemd doesn't write to those pages any longer. Those pages
> never get the chance to be dirty or migrated any longer.
> 
> 5. Once the guest VM is resumed on the target host, because of the lack of
> those dirty pages' data, the systemd may run into abnormal status, i.e.,
> there may be systemd segfault.
> 
> Log all write descriptors to fix the issue.
> 
> In addition, the patchset also fixes two bugs in vhost-scsi.
> 
> Dongli Zhang (log descriptor, suggested by Joao Martins):
>   vhost: modify vhost_log_write() for broader users
>   vhost-scsi: adjust vhost_scsi_get_desc() to log vring descriptors
>   vhost-scsi: cache log buffer in I/O queue vhost_scsi_cmd
>   vhost-scsi: log I/O queue write descriptors
>   vhost-scsi: log control queue write descriptors
>   vhost-scsi: log event queue write descriptors
>   vhost: add WARNING if log_num is more than limit
> 
> Dongli Zhang (vhost-scsi bugfix):
>   vhost-scsi: protect vq->log_used with vq->mutex
>   vhost-scsi: Fix vhost_scsi_send_bad_target()
> 
>  drivers/vhost/net.c   |   2 +-
>  drivers/vhost/scsi.c  | 191 +++++++++++++++++++++++++++++++++++++++------
>  drivers/vhost/vhost.c |  46 ++++++++---
>  drivers/vhost/vhost.h |   2 +-
>  4 files changed, 206 insertions(+), 35 deletions(-)
> 
> 
> base-commit: 5c8c229261f14159b54b9a32f12e5fa89d88b905
> 
> Thank you very much!
> 
> Dongli Zhang
> 
>