On Thu, 2023-11-16 at 11:15 -0600, Mike Christie wrote: > On 11/16/23 5:39 AM, John Garry wrote: > > On 14/11/2023 01:37, Mike Christie wrote: > > > > Feel free to pick up: > > Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx> > > > > > + the_result = scsi_execute_cmd(sdp, cmd, REQ_OP_DRV_IN, > > > buffer, > > > + RC16_LEN, SD_TIMEOUT, sdkp->max_retries, > > > > BTW, some might find it a little confusing that we have > > scsi_execute_cmd() retries arg as well as being able to pass > > exec_args.failure, and exec_args.failure may allow us to retry. > > Could this be improved, even in terms of arg or struct member > > naming/comments? > > Will add some comments. > > Martin W, if you are going to ask about this again, the answer is > that > in a future patchset, we can kill the reties passed directly into > scsi_execute_cmd. > Did I ask about this? ... I dimly remember ;-) > We could make scsi_decide_disposition's failures an array of > scsi_failure structs that gets checked in scsi_decide_disposition > and scsi_check_passthrough. We would need to add a function callout > to the scsi_failure struct for some of the more complicated failure > handling. That could then be used for some of the other > scsi_execute_cmd > cases as well. For the normal/fast path case though we would need > something to avoid function pointers. I am not sure if I like this idea. scsi_decide_disposition() is challenging to read already. I am not convinced that converting it into a loop over "struct scsi_failure" would make it easier to understand. I guess we'd need to see how it would look like. To my understanding, the two retry counts are currently orthogonal, the one in scsi_execute_cmd() representing the standard mid-layer "maybe_retry" case in scsi_decide_disposition, and the the other one representing the additional "high level" passthrough retry. We need to be careful when merging them. Wrt the callout, I'd actually thought about that when looking at your previous set, but I didn't want to interfere too much. I thought that using callouts might simplify the scsi_failure handling, and might actually make the new code easier to read. I can't judge possible effects on performance. Cheers, Martin
