Re: [PATCH v5 07/28] scsi_debug: Abort SCSI commands via .queue_reserved_command()

[Bart Van Assche <bvanassche@acm.org>]

On 9/26/25 12:32 AM, John Garry wrote:
> On 24/09/2025 21:30, Bart Van Assche wrote:
>> Add a .queue_reserved_command() implementation and call it from the code
>> path that aborts SCSI commands. This ensures that the code for
>> allocating a pseudo SCSI device and also the code for processing
>> reserved commands gets triggered while running blktests.
>>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> 
> At least Suggested-by would be good, if you don't mind

If nobody objects I will add the following text:

----------------------------------------------------------------------
Most of the code in this patch is a modified version of code from John
Garry. See also
https://lore.kernel.org/linux-scsi/75018e17-4dea-4e1b-8c92-7a224a1e13b9@oracle.com/

Suggested-by: John Garry <john.g.garry@oracle.com>
----------------------------------------------------------------------

>>   enum sdeb_defer_type {SDEB_DEFER_NONE = 0, SDEB_DEFER_HRT = 1,
>>                 SDEB_DEFER_WQ = 2, SDEB_DEFER_POLL = 3};
>> @@ -466,6 +483,8 @@ struct sdebug_defer {
>>   struct sdebug_scsi_cmd {
>>       spinlock_t   lock;
>>       struct sdebug_defer sd_dp;
>> +
>> +    struct scsi_debug_internal_cmd internal_cmd;
> 
> you could prob make this a union with sd_dp - I mean, could they ever 
> both be simultaneously used?

Introducing a union for sd_dp and internal_cmd only would suggest to
readers of the scsi_debug code that 'lock' protects both 'sd_dp' and
'internal_cmd', which is not the case. I propose to include the
following change in this patch:

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 0d4bdd38597a..12b885a2f719 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -483,8 +483,6 @@ struct sdebug_defer {
  struct sdebug_scsi_cmd {
  	spinlock_t   lock;
  	struct sdebug_defer sd_dp;
-
-	struct scsi_debug_internal_cmd internal_cmd;
  };

  static atomic_t sdebug_cmnd_count;   /* number of incoming commands */
@@ -9518,7 +9516,8 @@ static const struct scsi_host_template 
sdebug_driver_template = {
  	.module =		THIS_MODULE,
  	.skip_settle_delay =	1,
  	.track_queue_depth =	1,
-	.cmd_size = sizeof(struct sdebug_scsi_cmd),
+	.cmd_size = sizeof(union { struct sdebug_scsi_cmd c;
+				   struct scsi_debug_internal_cmd ic; }),
  	.init_cmd_priv = sdebug_init_cmd_priv,
  	.target_alloc =		sdebug_target_alloc,
  	.target_destroy =	sdebug_target_destroy,

>> +static int scsi_debug_setup_abort_cmd(struct scsi_cmnd *cmd,
> 
> nobody checks the return value

Agreed. I will change the return type from 'int' into 'void'.

>> +    abort_cmd = scsi_get_internal_cmd(shost->pseudo_sdev, DMA_TO_DEVICE,
> 
> DMA_NONE?

DMA_TO_DEVICE is converted into REQ_OP_DRV_OUT by 
scsi_get_internal_cmd(). Isn't that more appropriate for an operation
that has side effects (aborting a SCSI command) rather than
REQ_OP_DRV_IN?

>> +                      BLK_MQ_REQ_RESERVED);
>> +    if (WARN_ON_ONCE(!abort_cmd))
>> +        return false;
> 
> I don't really think that this deserves a WARN_ON_ONCE

OK, I will remove the WARN_ON_ONCE().

>> +    scp->result = (res ? DID_OK : DID_ERROR) << 16;
> 
> personally I think that if-else is nicer, but that's just me
> 
> And please consider using set_host_byte()

I will use set_host_byte().

Thanks,

Bart.