[PATCH RFT 0/6] scsi_debug: fake timeout handling improvements

[PATCH RFT 0/6] scsi_debug: fake timeout handling improvements

[John Garry]

This series includes improvements to fake timeout handling.

Specifically, when we try to abort a scsi_cmnd which has fake timeout,
we check the deferral type, but this would not be set properly. In
addition, we need special handling for fake timeout in the abort handler
to ensure that we don't get confused with any other deferral type.

I am setting as RTF as I want to test more. I also would like 
JiangJianJun <jiangjianjun3@huawei.com> to see if it can solve the
problem reported at https://lore.kernel.org/linux-scsi/4efb45b3-455a-44e8-83fa-e64ca8b9192a@oracle.com/T/#m2092593f3bc4b5fdde3f77376b65c10ab7bd605c

Finally some tidy-ups are included, which are not RFT.

John Garry (6):
  scsi: scsi_debug: Stop printing extra function name in debug logs
  scsi: scsi_debug: Stop using READ/WRITE_ONCE() when accessing
    sdebug_defer.defer_t
  scsi: scsi_debug: Drop NULL scsi_cmnd check in sdebug_q_cmd_complete()
  scsi: scsi_debug: Call sdebug_q_cmd_complete() from
    sdebug_blk_mq_poll_iter()
  scsi: scsi_debug: Clear sd_dp->defer_t in sdebug_q_cmd_complete()
  scsi: scsi_debug: Add special abort handling for fake timeout

 drivers/scsi/scsi_debug.c | 163 +++++++++++++++++++-------------------
 1 file changed, 83 insertions(+), 80 deletions(-)

-- 
2.43.5

[PATCH 1/6] scsi: scsi_debug: Stop printing extra function name in debug logs

[John Garry]

The driver defines as follows pr_fmt:
 #define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__

...meaning that we already get the function name added in any debug
statements.

Remove using of __func__ in debug logs to avoid the duplication.

For instances of where the function name was being printed, add some
verbose comment to avoid using "" (which would be a bit silly).

It would be nicer to stop using pr_fmt(), but that would mean rewriting
approx 100 debug statements to have a sensible and clear message.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 101 +++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 56 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 92b2af803d87e..243a440feacce 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1369,8 +1369,7 @@ static void mk_sense_invalid_fld(struct scsi_cmnd *scp,
 
 	sbuff = scp->sense_buffer;
 	if (!sbuff) {
-		sdev_printk(KERN_ERR, scp->device,
-			    "%s: sense_buffer is NULL\n", __func__);
+		sdev_printk(KERN_ERR, scp->device, "sense_buffer is NULL\n");
 		return;
 	}
 	asc = c_d ? INVALID_FIELD_IN_CDB : INVALID_FIELD_IN_PARAM_LIST;
@@ -1402,8 +1401,7 @@ static void mk_sense_invalid_fld(struct scsi_cmnd *scp,
 static void mk_sense_buffer(struct scsi_cmnd *scp, int key, int asc, int asq)
 {
 	if (!scp->sense_buffer) {
-		sdev_printk(KERN_ERR, scp->device,
-			    "%s: sense_buffer is NULL\n", __func__);
+		sdev_printk(KERN_ERR, scp->device, "sense_buffer is NULL\n");
 		return;
 	}
 	memset(scp->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
@@ -1421,8 +1419,7 @@ static void mk_sense_info_tape(struct scsi_cmnd *scp, int key, int asc, int asq,
 			unsigned int information, unsigned char tape_flags)
 {
 	if (!scp->sense_buffer) {
-		sdev_printk(KERN_ERR, scp->device,
-			    "%s: sense_buffer is NULL\n", __func__);
+		sdev_printk(KERN_ERR, scp->device, "sense_buffer is NULL\n");
 		return;
 	}
 	memset(scp->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
@@ -1450,15 +1447,12 @@ static int scsi_debug_ioctl(struct scsi_device *dev, unsigned int cmd,
 {
 	if (sdebug_verbose) {
 		if (0x1261 == cmd)
-			sdev_printk(KERN_INFO, dev,
-				    "%s: BLKFLSBUF [0x1261]\n", __func__);
+			sdev_printk(KERN_INFO, dev, "BLKFLSBUF [0x1261]\n");
 		else if (0x5331 == cmd)
 			sdev_printk(KERN_INFO, dev,
-				    "%s: CDROM_GET_CAPABILITY [0x5331]\n",
-				    __func__);
+				    "CDROM_GET_CAPABILITY [0x5331]\n");
 		else
-			sdev_printk(KERN_INFO, dev, "%s: cmd=0x%x\n",
-				    __func__, cmd);
+			sdev_printk(KERN_INFO, dev, "cmd=0x%x\n", cmd);
 	}
 	return -EINVAL;
 	/* return -ENOTTY; // correct return but upsets fdisk */
@@ -1662,8 +1656,8 @@ static int p_fill_from_dev_buffer(struct scsi_cmnd *scp, const void *arr,
 
 	act_len = sg_pcopy_from_buffer(sdb->table.sgl, sdb->table.nents,
 				       arr, arr_len, skip);
-	pr_debug("%s: off_dst=%u, scsi_bufflen=%u, act_len=%u, resid=%d\n",
-		 __func__, off_dst, scsi_bufflen(scp), act_len,
+	pr_debug("off_dst=%u, scsi_bufflen=%u, act_len=%u, resid=%d\n",
+		 off_dst, scsi_bufflen(scp), act_len,
 		 scsi_get_resid(scp));
 	n = scsi_bufflen(scp) - (off_dst + act_len);
 	scsi_set_resid(scp, min_t(u32, scsi_get_resid(scp), n));
@@ -3186,8 +3180,8 @@ static int resp_mode_select(struct scsi_cmnd *scp,
 		return DID_ERROR << 16;
 	else if (sdebug_verbose && (res < param_len))
 		sdev_printk(KERN_INFO, scp->device,
-			    "%s: cdb indicated=%d, IO sent=%d bytes\n",
-			    __func__, param_len, res);
+			    "cdb indicated=%d, IO sent=%d bytes\n",
+			    param_len, res);
 	md_len = mselect6 ? (arr[0] + 1) : (get_unaligned_be16(arr + 0) + 2);
 	bd_len = mselect6 ? arr[3] : get_unaligned_be16(arr + 6);
 	off = (mselect6 ? 4 : 8);
@@ -5123,8 +5117,7 @@ static int resp_write_scat(struct scsi_cmnd *scp,
 	if (lbdof == 0) {
 		if (sdebug_verbose)
 			sdev_printk(KERN_INFO, scp->device,
-				"%s: %s: LB Data Offset field bad\n",
-				my_name, __func__);
+				"%s: LB Data Offset field bad\n", my_name);
 		mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
 		return illegal_condition_result;
 	}
@@ -5132,8 +5125,7 @@ static int resp_write_scat(struct scsi_cmnd *scp,
 	if ((lrd_size + (num_lrd * lrd_size)) > lbdof_blen) {
 		if (sdebug_verbose)
 			sdev_printk(KERN_INFO, scp->device,
-				"%s: %s: LBA range descriptors don't fit\n",
-				my_name, __func__);
+				"%s: LBA range descriptors don't fit\n", my_name);
 		mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
 		return illegal_condition_result;
 	}
@@ -5142,8 +5134,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
 		return SCSI_MLQUEUE_HOST_BUSY;
 	if (sdebug_verbose)
 		sdev_printk(KERN_INFO, scp->device,
-			"%s: %s: Fetch header+scatter_list, lbdof_blen=%u\n",
-			my_name, __func__, lbdof_blen);
+			"%s: Fetch header+scatter_list, lbdof_blen=%u\n",
+			my_name, lbdof_blen);
 	res = fetch_to_dev_buffer(scp, lrdp, lbdof_blen);
 	if (res == -1) {
 		ret = DID_ERROR << 16;
@@ -5160,8 +5152,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
 		num = get_unaligned_be32(up + 8);
 		if (sdebug_verbose)
 			sdev_printk(KERN_INFO, scp->device,
-				"%s: %s: k=%d  LBA=0x%llx num=%u  sg_off=%u\n",
-				my_name, __func__, k, lba, num, sg_off);
+				"%s: k=%d  LBA=0x%llx num=%u  sg_off=%u\n",
+				my_name, k, lba, num, sg_off);
 		if (num == 0)
 			continue;
 		ret = check_device_access_params(scp, lba, num, true);
@@ -5173,8 +5165,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
 		if ((cum_lb + num) > bt_len) {
 			if (sdebug_verbose)
 				sdev_printk(KERN_INFO, scp->device,
-				    "%s: %s: sum of blocks > data provided\n",
-				    my_name, __func__);
+				    "%s: sum of blocks > data provided\n",
+				    my_name);
 			mk_sense_buffer(scp, ILLEGAL_REQUEST, WRITE_ERROR_ASC,
 					0);
 			ret = illegal_condition_result;
@@ -5866,8 +5858,8 @@ static int resp_verify(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 		goto cleanup;
 	} else if (sdebug_verbose && (ret < (a_num * lb_size))) {
 		sdev_printk(KERN_INFO, scp->device,
-			    "%s: %s: cdb indicated=%u, IO sent=%d bytes\n",
-			    my_name, __func__, a_num * lb_size, ret);
+			    "%s: cdb indicated=%u, IO sent=%d bytes\n",
+			    my_name, a_num * lb_size, ret);
 	}
 	if (is_bytchk3) {
 		for (j = 1, off = lb_size; j < vnum; ++j, off += lb_size)
@@ -6675,14 +6667,14 @@ static int scsi_debug_sdev_configure(struct scsi_device *sdp,
 	devip->debugfs_entry = debugfs_create_dir(dev_name(&sdp->sdev_dev),
 				sdebug_debugfs_root);
 	if (IS_ERR_OR_NULL(devip->debugfs_entry))
-		pr_info("%s: failed to create debugfs directory for device %s\n",
-			__func__, dev_name(&sdp->sdev_gendev));
+		pr_info("failed to create debugfs directory for device %s\n",
+			dev_name(&sdp->sdev_gendev));
 
 	dentry = debugfs_create_file("error", 0600, devip->debugfs_entry, sdp,
 				&sdebug_error_fops);
 	if (IS_ERR_OR_NULL(dentry))
-		pr_info("%s: failed to create error file for device %s\n",
-			__func__, dev_name(&sdp->sdev_gendev));
+		pr_info("failed to create error file for device %s\n",
+			dev_name(&sdp->sdev_gendev));
 
 	return 0;
 }
@@ -6870,7 +6862,7 @@ static int scsi_debug_abort(struct scsi_cmnd *SCpnt)
 
 	if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
 		sdev_printk(KERN_INFO, SCpnt->device,
-			    "%s: command%s found\n", __func__,
+			    "command%s found\n",
 			    aborted ? "" : " not");
 
 
@@ -6958,7 +6950,7 @@ static int scsi_debug_device_reset(struct scsi_cmnd *SCpnt)
 	++num_dev_resets;
 
 	if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
-		sdev_printk(KERN_INFO, sdp, "%s\n", __func__);
+		sdev_printk(KERN_INFO, sdp, "doing device reset");
 
 	scsi_debug_stop_all_queued(sdp);
 	if (devip) {
@@ -6998,7 +6990,7 @@ static int scsi_debug_target_reset(struct scsi_cmnd *SCpnt)
 
 	++num_target_resets;
 	if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
-		sdev_printk(KERN_INFO, sdp, "%s\n", __func__);
+		sdev_printk(KERN_INFO, sdp, "doing target reset\n");
 
 	list_for_each_entry(devip, &sdbg_host->dev_info_list, dev_list) {
 		if (devip->target == sdp->id) {
@@ -7011,7 +7003,7 @@ static int scsi_debug_target_reset(struct scsi_cmnd *SCpnt)
 
 	if (SDEBUG_OPT_RESET_NOISE & sdebug_opts)
 		sdev_printk(KERN_INFO, sdp,
-			    "%s: %d device(s) found in target\n", __func__, k);
+			    "%d device(s) found in target\n", k);
 
 	if (sdebug_fail_target_reset(SCpnt)) {
 		scmd_printk(KERN_INFO, SCpnt, "fail target reset 0x%x\n",
@@ -7032,7 +7024,7 @@ static int scsi_debug_bus_reset(struct scsi_cmnd *SCpnt)
 	++num_bus_resets;
 
 	if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
-		sdev_printk(KERN_INFO, sdp, "%s\n", __func__);
+		sdev_printk(KERN_INFO, sdp, "doing bus reset\n");
 
 	list_for_each_entry(devip, &sdbg_host->dev_info_list, dev_list) {
 		set_bit(SDEBUG_UA_BUS_RESET, devip->uas_bm);
@@ -7043,7 +7035,7 @@ static int scsi_debug_bus_reset(struct scsi_cmnd *SCpnt)
 
 	if (SDEBUG_OPT_RESET_NOISE & sdebug_opts)
 		sdev_printk(KERN_INFO, sdp,
-			    "%s: %d device(s) found in host\n", __func__, k);
+			    "%d device(s) found in host\n", k);
 	return SUCCESS;
 }
 
@@ -7055,7 +7047,7 @@ static int scsi_debug_host_reset(struct scsi_cmnd *SCpnt)
 
 	++num_host_resets;
 	if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
-		sdev_printk(KERN_INFO, SCpnt->device, "%s\n", __func__);
+		sdev_printk(KERN_INFO, SCpnt->device, "doing host reset\n");
 	mutex_lock(&sdebug_host_list_mutex);
 	list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
 		list_for_each_entry(devip, &sdbg_host->dev_info_list,
@@ -7070,7 +7062,7 @@ static int scsi_debug_host_reset(struct scsi_cmnd *SCpnt)
 	stop_all_queued();
 	if (SDEBUG_OPT_RESET_NOISE & sdebug_opts)
 		sdev_printk(KERN_INFO, SCpnt->device,
-			    "%s: %d device(s) found\n", __func__, k);
+			"%d device(s) found\n", k);
 	return SUCCESS;
 }
 
@@ -7221,8 +7213,8 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 			scsi_result = device_qfull_result;
 
 			if (unlikely(SDEBUG_OPT_Q_NOISE & sdebug_opts))
-				sdev_printk(KERN_INFO, sdp, "%s: num_in_q=%d +1, <inject> status: TASK SET FULL\n",
-					    __func__, num_in_q);
+				sdev_printk(KERN_INFO, sdp, "num_in_q=%d +1, <inject> status: TASK SET FULL\n",
+					    num_in_q);
 		}
 	}
 
@@ -7248,8 +7240,8 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 	}
 
 	if (unlikely(sdebug_verbose && cmnd->result))
-		sdev_printk(KERN_INFO, sdp, "%s: non-zero result=0x%x\n",
-			    __func__, cmnd->result);
+		sdev_printk(KERN_INFO, sdp, "non-zero result=0x%x\n",
+			    cmnd->result);
 
 	if (delta_jiff > 0 || ndelay > 0) {
 		ktime_t kt;
@@ -8687,7 +8679,7 @@ static int __init scsi_debug_init(void)
 
 	sdebug_debugfs_root = debugfs_create_dir("scsi_debug", NULL);
 	if (IS_ERR_OR_NULL(sdebug_debugfs_root))
-		pr_info("%s: failed to create initial debugfs directory\n", __func__);
+		pr_info("failed to create initial debugfs directory\n");
 
 	for (k = 0; k < hosts_to_add; k++) {
 		if (want_store && k == 0) {
@@ -8803,7 +8795,7 @@ static int sdebug_add_store(void)
 	if (unlikely(res < 0)) {
 		xa_unlock_irqrestore(per_store_ap, iflags);
 		kfree(sip);
-		pr_warn("%s: xa_alloc() errno=%d\n", __func__, -res);
+		pr_warn("xa_alloc() errno=%d\n", -res);
 		return res;
 	}
 	sdeb_most_recent_idx = n_idx;
@@ -8860,7 +8852,7 @@ static int sdebug_add_store(void)
 	return (int)n_idx;
 err:
 	sdebug_erase_store((int)n_idx, sip);
-	pr_warn("%s: failed, errno=%d\n", __func__, -res);
+	pr_warn("failed, errno=%d\n", -res);
 	return res;
 }
 
@@ -8919,7 +8911,7 @@ static int sdebug_add_host_helper(int per_host_idx)
 		put_device(&sdbg_host->dev);
 	else
 		kfree(sdbg_host);
-	pr_warn("%s: failed, errno=%d\n", __func__, -error);
+	pr_warn("failed, errno=%d\n", -error);
 	return error;
 }
 
@@ -8987,7 +8979,7 @@ static int sdebug_change_qdepth(struct scsi_device *sdev, int qdepth)
 
 	if (qdepth > SDEBUG_CANQUEUE) {
 		qdepth = SDEBUG_CANQUEUE;
-		pr_warn("%s: requested qdepth [%d] exceeds canqueue [%d], trim\n", __func__,
+		pr_warn("requested qdepth [%d] exceeds canqueue [%d], trim\n",
 			qdepth, SDEBUG_CANQUEUE);
 	}
 	if (qdepth < 1)
@@ -8999,7 +8991,7 @@ static int sdebug_change_qdepth(struct scsi_device *sdev, int qdepth)
 	mutex_unlock(&sdebug_host_list_mutex);
 
 	if (SDEBUG_OPT_Q_NOISE & sdebug_opts)
-		sdev_printk(KERN_INFO, sdev, "%s: qdepth=%d\n", __func__, qdepth);
+		sdev_printk(KERN_INFO, sdev, "qdepth=%d\n", qdepth);
 
 	return sdev->queue_depth;
 }
@@ -9272,8 +9264,7 @@ static void scsi_debug_abort_cmd(struct Scsi_Host *shost, struct scsi_cmnd *scp)
 	bool res = false;
 
 	if (!to_be_aborted_scmd) {
-		pr_err("%s: command with tag %#x not found\n", __func__,
-		       unique_tag);
+		pr_err("command with tag %#x not found\n", unique_tag);
 		return;
 	}
 
@@ -9281,11 +9272,9 @@ static void scsi_debug_abort_cmd(struct Scsi_Host *shost, struct scsi_cmnd *scp)
 		res = scsi_debug_stop_cmnd(to_be_aborted_scmd);
 
 	if (res)
-		pr_info("%s: aborted command with tag %#x\n",
-			__func__, unique_tag);
+		pr_info("aborted command with tag %#x\n", unique_tag);
 	else
-		pr_err("%s: failed to abort command with tag %#x\n",
-		       __func__, unique_tag);
+		pr_err("failed to abort command with tag %#x\n", unique_tag);
 
 	set_host_byte(scp, res ? DID_OK : DID_ERROR);
 }
-- 
2.43.5

[PATCH 2/6] scsi: scsi_debug: Stop using READ/WRITE_ONCE() when accessing sdebug_defer.defer_t

[John Garry]

Using READ/WRITE_ONCE() means that the read or write is not torn by the
compiler.

READ/WRITE_ONCE() is always used when accessing sdebug_defer.defer_t.

However, we also guard the access in a spinlock when accessing that member,
and spinlock already guarantees no tearing, so stop using
READ/WRITE_ONCE().

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 243a440feacce..e4994435ae514 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -6716,7 +6716,7 @@ static bool scsi_debug_stop_cmnd(struct scsi_cmnd *cmnd)
 {
 	struct sdebug_scsi_cmd *sdsc = scsi_cmd_priv(cmnd);
 	struct sdebug_defer *sd_dp = &sdsc->sd_dp;
-	enum sdeb_defer_type defer_t = READ_ONCE(sd_dp->defer_t);
+	enum sdeb_defer_type defer_t = sd_dp->defer_t;
 
 	lockdep_assert_held(&sdsc->lock);
 
@@ -7278,12 +7278,12 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 		if (polled) {
 			spin_lock_irqsave(&sdsc->lock, flags);
 			sd_dp->cmpl_ts = ktime_add(ns_to_ktime(ns_from_boot), kt);
-			WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_POLL);
+			sd_dp->defer_t = SDEB_DEFER_POLL;
 			spin_unlock_irqrestore(&sdsc->lock, flags);
 		} else {
 			/* schedule the invocation of scsi_done() for a later time */
 			spin_lock_irqsave(&sdsc->lock, flags);
-			WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_HRT);
+			sd_dp->defer_t = SDEB_DEFER_HRT;
 			hrtimer_start(&sd_dp->hrt, kt, HRTIMER_MODE_REL_PINNED);
 			/*
 			 * The completion handler will try to grab sqcp->lock,
@@ -7307,11 +7307,11 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 		if (polled) {
 			spin_lock_irqsave(&sdsc->lock, flags);
 			sd_dp->cmpl_ts = ns_to_ktime(ns_from_boot);
-			WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_POLL);
+			sd_dp->defer_t = SDEB_DEFER_POLL;
 			spin_unlock_irqrestore(&sdsc->lock, flags);
 		} else {
 			spin_lock_irqsave(&sdsc->lock, flags);
-			WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_WQ);
+			sd_dp->defer_t = SDEB_DEFER_WQ;
 			schedule_work(&sd_dp->ew.work);
 			spin_unlock_irqrestore(&sdsc->lock, flags);
 		}
@@ -9115,7 +9115,7 @@ static bool sdebug_blk_mq_poll_iter(struct request *rq, void *opaque)
 
 	spin_lock_irqsave(&sdsc->lock, flags);
 	sd_dp = &sdsc->sd_dp;
-	if (READ_ONCE(sd_dp->defer_t) != SDEB_DEFER_POLL) {
+	if (sd_dp->defer_t != SDEB_DEFER_POLL) {
 		spin_unlock_irqrestore(&sdsc->lock, flags);
 		return true;
 	}
-- 
2.43.5

Re: [PATCH 2/6] scsi: scsi_debug: Stop using READ/WRITE_ONCE() when accessing sdebug_defer.defer_t

[Bart Van Assche]

On 11/13/25 5:36 AM, John Garry wrote:
> Using READ/WRITE_ONCE() means that the read or write is not torn by the
> compiler.
> 
> READ/WRITE_ONCE() is always used when accessing sdebug_defer.defer_t.
> 
> However, we also guard the access in a spinlock when accessing that member,
> and spinlock already guarantees no tearing, so stop using
> READ/WRITE_ONCE().

According to the Linux kernel memory model, if a variable is accessed
outside a critical section then READ_ONCE() and WRITE_ONCE() must be
used when accessing that variable inside a critical section.

Thanks,

Bart.

Re: [PATCH 2/6] scsi: scsi_debug: Stop using READ/WRITE_ONCE() when accessing sdebug_defer.defer_t

[John Garry]

On 13/11/2025 17:45, Bart Van Assche wrote:
> 
> On 11/13/25 5:36 AM, John Garry wrote:
>> Using READ/WRITE_ONCE() means that the read or write is not torn by the
>> compiler.
>>
>> READ/WRITE_ONCE() is always used when accessing sdebug_defer.defer_t.
>>
>> However, we also guard the access in a spinlock when accessing that 
>> member,
>> and spinlock already guarantees no tearing, so stop using
>> READ/WRITE_ONCE().
> 
> According to the Linux kernel memory model, if a variable is accessed
> outside a critical section then READ_ONCE() and WRITE_ONCE() must be
> used when accessing that variable inside a critical section.

So do you see a problem with this patch? As mentioned, 
sdebug_defer.defer_t is already only accessed with a spinlock held 
(sdebug_scsi_cmd.lock) (which I consider being a critical section).

Thanks,
John

Re: [PATCH 2/6] scsi: scsi_debug: Stop using READ/WRITE_ONCE() when accessing sdebug_defer.defer_t

[Bart Van Assche]

On 11/14/25 1:02 AM, John Garry wrote:
> So do you see a problem with this patch? As mentioned, 
> sdebug_defer.defer_t is already only accessed with a spinlock held 
> (sdebug_scsi_cmd.lock) (which I consider being a critical section).

Ah, that's something I hadn't realized yet.

I agree that this patch should be fine.

Thanks,

Bart.

[PATCH 3/6] scsi: scsi_debug: Drop NULL scsi_cmnd check in sdebug_q_cmd_complete()

[John Garry]

The scp pointer cannot be NULL, as it is evaludated from container_of()
and pointer offsets (so remove the check in sdebug_q_cmd_complete()).

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index e4994435ae514..25feb0fb898cf 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -6386,11 +6386,6 @@ static void sdebug_q_cmd_complete(struct sdebug_defer *sd_dp)
 			atomic_inc(&sdebug_miss_cpus);
 	}
 
-	if (!scp) {
-		pr_err("scmd=NULL\n");
-		return;
-	}
-
 	spin_lock_irqsave(&sdsc->lock, flags);
 	aborted = sd_dp->aborted;
 	if (unlikely(aborted))
-- 
2.43.5

Re: [PATCH 3/6] scsi: scsi_debug: Drop NULL scsi_cmnd check in sdebug_q_cmd_complete()

[Bart Van Assche]

On 11/13/25 5:36 AM, John Garry wrote:
> The scp pointer cannot be NULL, as it is evaludated from container_of()
                                            ^^^^^^^^^^
                                            evaluated

Otherwise this patch looks good to me.

Thanks,

Bart.

Re: [PATCH 3/6] scsi: scsi_debug: Drop NULL scsi_cmnd check in sdebug_q_cmd_complete()

[John Garry]

On 13/11/2025 17:46, Bart Van Assche wrote:
> On 11/13/25 5:36 AM, John Garry wrote:
>> The scp pointer cannot be NULL, as it is evaludated from container_of()
>                                             ^^^^^^^^^^
>                                             evaluated
> 
> Otherwise this patch looks good to me.

Will fix, cheers

[PATCH RFT 4/6] scsi: scsi_debug: Call sdebug_q_cmd_complete() from sdebug_blk_mq_poll_iter()

[John Garry]

The poll iter calls scsi_done() directly instead of using the common
helper, sdebug_q_cmd_complete(). Change to use that helper.

Function sdebug_q_cmd_complete() also handles sd_dp->aborted being set,
which sdebug_blk_mq_poll_iter() was not doing.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 25feb0fb898cf..093124c38d28f 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -9121,13 +9121,8 @@ static bool sdebug_blk_mq_poll_iter(struct request *rq, void *opaque)
 	}
 	spin_unlock_irqrestore(&sdsc->lock, flags);
 
-	if (sdebug_statistics) {
-		atomic_inc(&sdebug_completions);
-		if (raw_smp_processor_id() != sd_dp->issuing_cpu)
-			atomic_inc(&sdebug_miss_cpus);
-	}
+	sdebug_q_cmd_complete(sd_dp);
 
-	scsi_done(cmd); /* callback to mid level */
 	(*data->num_entries)++;
 	return true;
 }
-- 
2.43.5

[PATCH RFT 5/6] scsi: scsi_debug: Clear sd_dp->defer_t in sdebug_q_cmd_complete()

[John Garry]

When a scsi_cmnd is recycled, it may check the old value, so clear it.

Such a situation is when we issue a fake timeout, as sd_dp->defer_t is
not reset in the queue_rq() path.

It would be nicer to have such code in sdebug_init_cmd_priv(), but that
is only called once per request init (and not per cycle).

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 093124c38d28f..46883c1d99a48 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -6390,6 +6390,7 @@ static void sdebug_q_cmd_complete(struct sdebug_defer *sd_dp)
 	aborted = sd_dp->aborted;
 	if (unlikely(aborted))
 		sd_dp->aborted = false;
+	sd_dp->defer_t = SDEB_DEFER_NONE;
 
 	spin_unlock_irqrestore(&sdsc->lock, flags);
 
-- 
2.43.5

[PATCH RFT 6/6] scsi: scsi_debug: Add special abort handling for fake timeout

[John Garry]

In the case of a fake timeout, the abort handler will try to "stop" the
command. Indeed, there is nothing to be done there, so just return success.

For this, add a new sdeb_defer_type, SDEB_DEFER_FAKE_TIMEOUT.

It's better not to use SDEB_DEFER_NONE for this purpose, as that could
be confused with when we want to abort a command which does not have a
fake timeout, like when we call scsi_debug_device_reset().

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 46883c1d99a48..4f383d97699bc 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -452,7 +452,8 @@ struct sdeb_store_info {
 	dev_to_sdebug_host(shost->dma_dev)
 
 enum sdeb_defer_type {SDEB_DEFER_NONE = 0, SDEB_DEFER_HRT = 1,
-		      SDEB_DEFER_WQ = 2, SDEB_DEFER_POLL = 3};
+		      SDEB_DEFER_WQ = 2, SDEB_DEFER_POLL = 3,
+		      SDEB_DEFER_FAKE_TIMEOUT = 4};
 
 struct sdebug_defer {
 	struct hrtimer hrt;
@@ -6708,14 +6709,10 @@ static void scsi_debug_sdev_destroy(struct scsi_device *sdp)
 }
 
 /* Returns true if cancelled or not running callback. */
-static bool scsi_debug_stop_cmnd(struct scsi_cmnd *cmnd)
+static bool _scsi_debug_stop_cmnd(struct sdebug_defer *sd_dp)
 {
-	struct sdebug_scsi_cmd *sdsc = scsi_cmd_priv(cmnd);
-	struct sdebug_defer *sd_dp = &sdsc->sd_dp;
 	enum sdeb_defer_type defer_t = sd_dp->defer_t;
 
-	lockdep_assert_held(&sdsc->lock);
-
 	if (defer_t == SDEB_DEFER_HRT) {
 		int res = hrtimer_try_to_cancel(&sd_dp->hrt);
 
@@ -6735,11 +6732,27 @@ static bool scsi_debug_stop_cmnd(struct scsi_cmnd *cmnd)
 		return false;
 	} else if (defer_t == SDEB_DEFER_POLL) {
 		return true;
+	} else if (defer_t == SDEB_DEFER_FAKE_TIMEOUT) {
+		return true;
 	}
 
 	return false;
 }
 
+static bool scsi_debug_stop_cmnd(struct scsi_cmnd *cmnd)
+{
+	struct sdebug_scsi_cmd *sdsc = scsi_cmd_priv(cmnd);
+	struct sdebug_defer *sd_dp = &sdsc->sd_dp;
+
+	lockdep_assert_held(&sdsc->lock);
+
+	if (_scsi_debug_stop_cmnd(sd_dp)) {
+		sd_dp->defer_t = SDEB_DEFER_NONE;
+		return true;
+	}
+	return false;
+}
+
 struct sdebug_abort_cmd {
 	u32 unique_tag;
 };
@@ -9293,10 +9306,13 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 				   struct scsi_cmnd *scp)
 {
 	u8 sdeb_i;
+	struct sdebug_scsi_cmd *sdsc = scsi_cmd_priv(scp);
+	struct sdebug_defer *sd_dp = &sdsc->sd_dp;
 	struct scsi_device *sdp = scp->device;
 	const struct opcode_info_t *oip;
 	const struct opcode_info_t *r_oip;
 	struct sdebug_dev_info *devip;
+	unsigned long spinlock_flags;
 	u8 *cmd = scp->cmnd;
 	int (*r_pfp)(struct scsi_cmnd *, struct sdebug_dev_info *);
 	int (*pfp)(struct scsi_cmnd *, struct sdebug_dev_info *) = NULL;
@@ -9353,6 +9369,9 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 
 	if (sdebug_timeout_cmd(scp)) {
 		scmd_printk(KERN_INFO, scp, "timeout command 0x%x\n", opcode);
+		spin_lock_irqsave(&sdsc->lock, spinlock_flags);
+		sd_dp->defer_t = SDEB_DEFER_FAKE_TIMEOUT;
+		spin_unlock_irqrestore(&sdsc->lock, spinlock_flags);
 		return 0;
 	}
 
@@ -9451,8 +9470,12 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 	if (sdebug_fake_rw && (F_FAKE_RW & flags))
 		goto fini;
 	if (unlikely(sdebug_every_nth)) {
-		if (fake_timeout(scp))
+		if (fake_timeout(scp)) {
+			spin_lock_irqsave(&sdsc->lock, spinlock_flags);
+			sd_dp->defer_t = SDEB_DEFER_FAKE_TIMEOUT;
+			spin_unlock_irqrestore(&sdsc->lock, spinlock_flags);
 			return 0;	/* ignore command: make trouble */
+		}
 	}
 	if (likely(oip->pfp))
 		pfp = oip->pfp;	/* calls a resp_* function */
-- 
2.43.5

Re: [PATCH RFT 0/6] scsi_debug: fake timeout handling improvements

[John Garry]

On 13/11/2025 13:36, John Garry wrote:
> This series includes improvements to fake timeout handling.
> 
> Specifically, when we try to abort a scsi_cmnd which has fake timeout,
> we check the deferral type, but this would not be set properly. In
> addition, we need special handling for fake timeout in the abort handler
> to ensure that we don't get confused with any other deferral type.
> 

Hi Martin,

Can you please consider picking up the first 3 patches in this series? I 
am awaiting feedback from JiangJianJun for the rest.

Thanks

> I am setting as RTF as I want to test more. I also would like
> JiangJianJun <jiangjianjun3@huawei.com> to see if it can solve the
> problem reported at https://lore.kernel.org/linux-scsi/4efb45b3-455a-44e8-83fa-e64ca8b9192a@oracle.com/T/#m2092593f3bc4b5fdde3f77376b65c10ab7bd605c
> 
> Finally some tidy-ups are included, which are not RFT.
> 
> John Garry (6):
>    scsi: scsi_debug: Stop printing extra function name in debug logs
>    scsi: scsi_debug: Stop using READ/WRITE_ONCE() when accessing
>      sdebug_defer.defer_t
>    scsi: scsi_debug: Drop NULL scsi_cmnd check in sdebug_q_cmd_complete()
>    scsi: scsi_debug: Call sdebug_q_cmd_complete() from
>      sdebug_blk_mq_poll_iter()
>    scsi: scsi_debug: Clear sd_dp->defer_t in sdebug_q_cmd_complete()
>    scsi: scsi_debug: Add special abort handling for fake timeout
> 
>   drivers/scsi/scsi_debug.c | 163 +++++++++++++++++++-------------------
>   1 file changed, 83 insertions(+), 80 deletions(-)
> 

Re: [PATCH RFT 0/6] scsi_debug: fake timeout handling improvements

[Martin K. Petersen]

John,

> Can you please consider picking up the first 3 patches in this series?

Patches 1-3 applied to 6.20/scsi-staging, thanks!

-- 
Martin K. Petersen

Re: [PATCH RFT 0/6] scsi_debug: fake timeout handling improvements

[John Garry]

On 17/12/2025 02:48, Martin K. Petersen wrote:
> 
> John,
> 
>> Can you please consider picking up the first 3 patches in this series?
> 
> Patches 1-3 applied to 6.20/scsi-staging, thanks!
> 

Thanks

@JiangJianJun, can you please check the other patches?

Re: [PATCH RFT 0/6] scsi_debug: fake timeout handling improvements

[JiangJianJun]

>> 
>> John,
>> 
>>> Can you please consider picking up the first 3 patches in this series?
>> 
>> Patches 1-3 applied to 6.20/scsi-staging, thanks!
>> 
>
>Thanks
>
>@JiangJianJun, can you please check the other patches?

Sorry for sending this email late.
That's great! I have checked the func, thanks!

Re: [PATCH RFT 0/6] scsi_debug: fake timeout handling improvements

[John Garry]

On 16/04/2026 03:30, JiangJianJun wrote:
>>>
>>> John,
>>>
>>>> Can you please consider picking up the first 3 patches in this series?
>>>
>>> Patches 1-3 applied to 6.20/scsi-staging, thanks!
>>>
>>
>> Thanks
>>
>> @JiangJianJun, can you please check the other patches?
> 
> Sorry for sending this email late.
> That's great! I have checked the func, thanks!
> 
> 

You seen to respond quicker than 5 months.

Martin has merged 1-3/6 in this series, as above. If you want the rest - 
which are the ones which matter to you - then you can repost them.

John

Re: (subset) [PATCH RFT 0/6] scsi_debug: fake timeout handling improvements

[Martin K. Petersen]

On Thu, 13 Nov 2025 13:36:39 +0000, John Garry wrote:

> This series includes improvements to fake timeout handling.
> 
> Specifically, when we try to abort a scsi_cmnd which has fake timeout,
> we check the deferral type, but this would not be set properly. In
> addition, we need special handling for fake timeout in the abort handler
> to ensure that we don't get confused with any other deferral type.
> 
> [...]

Applied to 6.20/scsi-queue, thanks!

[1/6] scsi: scsi_debug: Stop printing extra function name in debug logs
      https://git.kernel.org/mkp/scsi/c/a743b120227a
[2/6] scsi: scsi_debug: Stop using READ/WRITE_ONCE() when accessing sdebug_defer.defer_t
      https://git.kernel.org/mkp/scsi/c/559ae7a26b10
[3/6] scsi: scsi_debug: Drop NULL scsi_cmnd check in sdebug_q_cmd_complete()
      https://git.kernel.org/mkp/scsi/c/a8cf5c1bee0f

-- 
Martin K. Petersen