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

[John Garry <john.g.garry@oracle.com>]

On 26/09/2025 21:44, Bart Van Assche wrote:
> 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>
> ----------------------------------------------------------------------

fine, thanks

> 
>>>   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; }),

ok, but you could also introduce a new structure which defines this 
union. That's looks a little bit better.

>       .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?

but it it make a difference if we use REQ_OP_DRV_OUT or REQ_OP_DRV_IN? 
If not, then I think DMA_NONE makes more sense.

Thanks,
John