Re: [PATCH 0/5]scsi:scsi_debug: Add error injection for single device

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

On 23/03/2023 13:13, haowenchao (C) wrote:
> On 2023/3/23 20:40, John Garry wrote:
>> On 23/03/2023 11:55, Wenchao Hao wrote:
>>> The original error injection mechanism was based on scsi_host which
>>> could not inject fault for a single SCSI device.
>>>
>>> This patchset provides the ability to inject errors for a single
>>> SCSI device. Now we supports inject timeout errors, queuecommand
>>> errors, and hostbyte, driverbyte, statusbyte, and sense data for
>>> specific SCSI Command.
>>
>> There is already a basic mechanism to generate errors - like timeouts 
>> - on "nth" command. Can you say why you want this new interface? What 
>> special scenarios are you trying to test/validate (which could not be 
>> achieved based on the current mechanism)?
>>
> 
> I am testing a new error handle policy which is based on single scsi_device
> without set host to RECOVERY. So I need a method to generate errors for
> single SCSI devices.
> 
> While we can not generate errors for single device with current mechanism
> because it is designed for host-wide error generation.
> 
>> With this series we would have 2x methods to inject errors, which is 
>> less than ideal, and they seem to possibly conflict as well, e.g. I 
>> set timeout for nth command via current interface and then use the new 
>> interface to set timeout for some other cadence. What behavior to 
>> expect ...?
> 
> I did not take this issue in consideration. I now assume the users would
> not use these 2 methods at same time.
> 
> What's more, I don not know where to write the usage of this newly added
> interface, maybe we can explain these in doc?

sysfs entries are described in Documentation/ABI, but please don't add 
elaborate programming interfaces in sysfs files (like in these patches) 
- a sysfs file should be just for reading or writing a single value

> 
>>
>> I'm not saying that I am a huge fan of the current inject mechanism, 
>> but at the very least you need to provide more justification for this 
>> series.
>>>>
>>> The first patch add an sysfs interface to add and inquiry single
>>> device's error injection info; the second patch defined how to remove
>>> an injection which has been added. The following 3 patches use the
>>> injection info and generate the related error type.
>>>
>>> Wenchao Hao (5):
>>>    scsi:scsi_debug: Add sysfs interface to manage scsi devices' error
>>>      injection
>>>    scsi:scsi_debug: Define grammar to remove added error injection
>>>    scsi:scsi_debug: timeout command if the error is injected
>>>    scsi:scsi_debug: Return failed value if the error is injected
>>>    scsi:scsi_debug: set command's result and sense data if the error is
>>>      injected
>>>
>>>   drivers/scsi/scsi_debug.c | 296 ++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 296 insertions(+)
>>>
>>
>>
>