On Thu, 2021-04-29 at 09:32 -0700, Bart Van Assche wrote:
> On 4/29/21 8:50 AM, mwilck@xxxxxxxx wrote:
> > + if (hdr.dxfer_len > (queue_max_hw_sectors(bdev->bd_disk-
> > >queue) << 9))
> > + return -EIO;
>
> How about using SECTOR_SHIFT instead of the number 9?
no problem. That line was just copied from the scsi_ioctl code.
>
> > + /*
> > + * Errors resulting from invalid parameters
> > shouldn't be retried
> > + * on another path.
> > + */
> > + switch (rc) {
> > + case -ENOIOCTLCMD:
> > + case -EFAULT:
> > + case -EINVAL:
> > + case -EPERM:
> > + goto out;
> > + default:
> > + break;
> > + }
>
> Will -ENOMEM result in an immediate retry? Is that what's desired?
No, I overlooked that case. Thanks for pointing this out.
>
> > + if (rhdr.info & SG_INFO_CHECK) {
> > + int result;
> > + blk_status_t sts;
> > +
> > + __set_status_byte(&result, rhdr.status);
> > + __set_msg_byte(&result, rhdr.msg_status);
> > + __set_host_byte(&result, rhdr.host_status);
> > + __set_driver_byte(&result,
> > rhdr.driver_status);
> > +
> > + sts = __scsi_result_to_blk_status(&result,
> > result);
> > + rhdr.host_status = host_byte(result);
> > +
> > + /* See if this is a target or path error.
> > */
> > + if (sts == BLK_STS_OK)
> > + rc = 0;
> > + else if (blk_path_error(sts))
> > + rc = -EIO;
> > + else {
> > + rc = blk_status_to_errno(sts);
> > + goto out;
> > + }
> > + }
>
> Will SAM_STAT_CHECK_CONDITION be treated as an I/O error? Is that
> what's
> desired? If not, does that mean that scsi_result_to_blk_status()
> shouldn't be used but instead that a custom SCSI result conversion is
> needed?
This mimics the logic for regular SCSI block I/O. By default, CHECK
CONDITION indeed maps to a BLK_STS_IO_ERR, and will be treated as a
path error. As you probably know, there are some exceptions that are
handled in the SCSI mid-layer beforehand. For example, check_sense()
sets DID_TARGET_FAILURE or DID_MEDIUM_ERROR for certain sense codes,
which map to target errors. So yes, I think this is correct.
> If __scsi_result_to_blk_status() is the right function to call, how
> about making that function accept the driver status, host status, msg
> and SAM status as four separate arguments such that the
> __set_*_byte()
> calls can be left out?
>
> > + char *argv[2] = { "fail_path", bdbuf };
>
> Can the above array be declared static?
How would that work? The function needs to be reentrant, and bdbuf is
on the stack. I don't see an issue here, it's really just two pointers
on the stack.
Regards,
Martin
