| Message ID | 20190714034415epcms2p25f9787cb71993a30f58524d2f355b543@epcms2p2 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] mpt3sas: support target smid for [abort|query] task | expand |
On 7/14/19 5:44 AM, Minwoo Im wrote: > We can request task management IOCTL command(MPI2_FUNCTION_SCSI_TASK_MGMT) > to /dev/mpt3ctl. If the given task_type is either abort task or query > task, it may need a field named "Initiator Port Transfer Tag to Manage" > in the IU. > > Current code does not support to check target IPTT tag from the > tm_request. This patch introduces to check TaskMID given from the > userspace as a target tag. We have a rule of relationship between > (struct request *req->tag) and smid in mpt3sas_base.c: > > 3318 u16 > 3319 mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx, > 3320 struct scsi_cmnd *scmd) > 3321 { > 3322 struct scsiio_tracker *request = scsi_cmd_priv(scmd); > 3323 unsigned int tag = scmd->request->tag; > 3324 u16 smid; > 3325 > 3326 smid = tag + 1; > > So if we want to abort a request tagged #X, then we can pass (X + 1) to > this IOCTL handler. Otherwise, user space just can pass 0 TaskMID to > abort the first outstanding smid which is legacy behaviour. > > Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com> > Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com> > Cc: Sathya Prakash <sathya.prakash@broadcom.com> > Cc: James E.J. Bottomley <jejb@linux.ibm.com> > Cc: Martin K. Petersen <martin.petersen@oracle.com> > Cc: MPT-FusionLinux.pdl@broadcom.com > Signed-off-by: Minwoo Im <minwoo.im@samsung.com> > --- > drivers/scsi/mpt3sas/mpt3sas_ctl.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > index b2bb47c14d35..f6b8fd90610a 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > @@ -596,8 +596,16 @@ _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg, > if (priv_data->sas_target->handle != handle) > continue; > st = scsi_cmd_priv(scmd); > - tm_request->TaskMID = cpu_to_le16(st->smid); > - found = 1; > + > + /* > + * If the given TaskMID from the user space is zero, then the > + * first outstanding smid will be picked up. Otherwise, > + * targeted smid will be the one. > + */ > + if (!tm_request->TaskMID || tm_request->TaskMID == st->smid) { > + tm_request->TaskMID = cpu_to_le16(st->smid); > + found = 1; > + } > } > > if (!found) { > I think this is fundamentally wrong. ABORT_TASK is used to abort a single task, which of course has to be known beforehand. If you don't know the task, what exactly do you hope to achieve here? Aborting random I/O? Or, even worse, aborting I/O the driver uses internally and corrupt the internal workflow of the driver? We should simply disallow any ABORT TASK from userspace if the TaskMID is zero. And I would even argue to disabllow ABORT TASK from userspace completely, as the smid is never relayed to userland, and as such the user cannot know which task should be aborted. Cheers, Hannes
> I think this is fundamentally wrong. > ABORT_TASK is used to abort a single task, which of course has to be > known beforehand. If you don't know the task, what exactly do you hope > to achieve here? Aborting random I/O? > Or, even worse, aborting I/O the driver uses internally and corrupt the > internal workflow of the driver? This patch is nothing but a case-addition to the existing code. I also have a doubt here why the first picked SMID should be aborted/queried, but not for this time in this patch. > > We should simply disallow any ABORT TASK from userspace if the TaskMID > is zero. And I would even argue to disabllow ABORT TASK from userspace > completely, as the smid is never relayed to userland, and as such the > user cannot know which task should be aborted. System administrator might want to query task or abort task if something happens based on the tag in block layer via debugfs or some logs. You're right that userspaces has nothing to do with the tag generation which will be held inside block layer. But some of administrator would know the relationship between smid and tag which can be found at debugfs or the logs. I'm not sure if it's okay to be picked, but if we can request TMF for the Targeted smid to the HBA firmware, it would be great to test devices or Figure out what happened in the target device. Thanks,
On 19-07-15 08:13:36, Hannes Reinecke wrote: > I think this is fundamentally wrong. > ABORT_TASK is used to abort a single task, which of course has to be > known beforehand. If you don't know the task, what exactly do you hope > to achieve here? Aborting random I/O? > Or, even worse, aborting I/O the driver uses internally and corrupt the > internal workflow of the driver? > > We should simply disallow any ABORT TASK from userspace if the TaskMID > is zero. And I would even argue to disabllow ABORT TASK from userspace > completely, as the smid is never relayed to userland, and as such the > user cannot know which task should be aborted. > > Cheers, > > Hannes Sreekanth, Could you please give some thoughts about what Hannes said?
On Mon, Jul 15, 2019 at 11:43 AM Hannes Reinecke <hare@suse.de> wrote: > > On 7/14/19 5:44 AM, Minwoo Im wrote: > > We can request task management IOCTL command(MPI2_FUNCTION_SCSI_TASK_MGMT) > > to /dev/mpt3ctl. If the given task_type is either abort task or query > > task, it may need a field named "Initiator Port Transfer Tag to Manage" > > in the IU. > > > > Current code does not support to check target IPTT tag from the > > tm_request. This patch introduces to check TaskMID given from the > > userspace as a target tag. We have a rule of relationship between > > (struct request *req->tag) and smid in mpt3sas_base.c: > > > > 3318 u16 > > 3319 mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx, > > 3320 struct scsi_cmnd *scmd) > > 3321 { > > 3322 struct scsiio_tracker *request = scsi_cmd_priv(scmd); > > 3323 unsigned int tag = scmd->request->tag; > > 3324 u16 smid; > > 3325 > > 3326 smid = tag + 1; > > > > So if we want to abort a request tagged #X, then we can pass (X + 1) to > > this IOCTL handler. Otherwise, user space just can pass 0 TaskMID to > > abort the first outstanding smid which is legacy behaviour. > > > > Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com> > > Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com> > > Cc: Sathya Prakash <sathya.prakash@broadcom.com> > > Cc: James E.J. Bottomley <jejb@linux.ibm.com> > > Cc: Martin K. Petersen <martin.petersen@oracle.com> > > Cc: MPT-FusionLinux.pdl@broadcom.com > > Signed-off-by: Minwoo Im <minwoo.im@samsung.com> > > --- > > drivers/scsi/mpt3sas/mpt3sas_ctl.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > > index b2bb47c14d35..f6b8fd90610a 100644 > > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > > @@ -596,8 +596,16 @@ _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg, > > if (priv_data->sas_target->handle != handle) > > continue; > > st = scsi_cmd_priv(scmd); > > - tm_request->TaskMID = cpu_to_le16(st->smid); > > - found = 1; > > + > > + /* > > + * If the given TaskMID from the user space is zero, then the > > + * first outstanding smid will be picked up. Otherwise, > > + * targeted smid will be the one. > > + */ > > + if (!tm_request->TaskMID || tm_request->TaskMID == st->smid) { > > + tm_request->TaskMID = cpu_to_le16(st->smid); > > + found = 1; > > + } > > } > > > > if (!found) { > > > I think this is fundamentally wrong. > ABORT_TASK is used to abort a single task, which of course has to be > known beforehand. If you don't know the task, what exactly do you hope > to achieve here? Aborting random I/O? > Or, even worse, aborting I/O the driver uses internally and corrupt the > internal workflow of the driver? > > We should simply disallow any ABORT TASK from userspace if the TaskMID > is zero. And I would even argue to disabllow ABORT TASK from userspace > completely, as the smid is never relayed to userland, and as such the > user cannot know which task should be aborted. Hannes, This interface was added long time back in mpt2sas driver and I don't have exact reason of adding this interface at that time. But I know that this interface is still used by BRCM test team & few customers only for some functionality and regression testing. Thanks, Sreekanth > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Teamlead Storage & Networking > hare@suse.de +49 911 74053 688 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah > HRB 21284 (AG Nürnberg)
On 19-07-23 16:57:49, Sreekanth Reddy wrote: > On Mon, Jul 15, 2019 at 11:43 AM Hannes Reinecke <hare@suse.de> wrote: > > > > On 7/14/19 5:44 AM, Minwoo Im wrote: > > > We can request task management IOCTL command(MPI2_FUNCTION_SCSI_TASK_MGMT) > > > to /dev/mpt3ctl. If the given task_type is either abort task or query > > > task, it may need a field named "Initiator Port Transfer Tag to Manage" > > > in the IU. > > > > > > Current code does not support to check target IPTT tag from the > > > tm_request. This patch introduces to check TaskMID given from the > > > userspace as a target tag. We have a rule of relationship between > > > (struct request *req->tag) and smid in mpt3sas_base.c: > > > > > > 3318 u16 > > > 3319 mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx, > > > 3320 struct scsi_cmnd *scmd) > > > 3321 { > > > 3322 struct scsiio_tracker *request = scsi_cmd_priv(scmd); > > > 3323 unsigned int tag = scmd->request->tag; > > > 3324 u16 smid; > > > 3325 > > > 3326 smid = tag + 1; > > > > > > So if we want to abort a request tagged #X, then we can pass (X + 1) to > > > this IOCTL handler. Otherwise, user space just can pass 0 TaskMID to > > > abort the first outstanding smid which is legacy behaviour. > > > > > > Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com> > > > Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com> > > > Cc: Sathya Prakash <sathya.prakash@broadcom.com> > > > Cc: James E.J. Bottomley <jejb@linux.ibm.com> > > > Cc: Martin K. Petersen <martin.petersen@oracle.com> > > > Cc: MPT-FusionLinux.pdl@broadcom.com > > > Signed-off-by: Minwoo Im <minwoo.im@samsung.com> > > > --- > > > drivers/scsi/mpt3sas/mpt3sas_ctl.c | 12 ++++++++++-- > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > > > index b2bb47c14d35..f6b8fd90610a 100644 > > > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > > > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > > > @@ -596,8 +596,16 @@ _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg, > > > if (priv_data->sas_target->handle != handle) > > > continue; > > > st = scsi_cmd_priv(scmd); > > > - tm_request->TaskMID = cpu_to_le16(st->smid); > > > - found = 1; > > > + > > > + /* > > > + * If the given TaskMID from the user space is zero, then the > > > + * first outstanding smid will be picked up. Otherwise, > > > + * targeted smid will be the one. > > > + */ > > > + if (!tm_request->TaskMID || tm_request->TaskMID == st->smid) { > > > + tm_request->TaskMID = cpu_to_le16(st->smid); > > > + found = 1; > > > + } > > > } > > > > > > if (!found) { > > > > > I think this is fundamentally wrong. > > ABORT_TASK is used to abort a single task, which of course has to be > > known beforehand. If you don't know the task, what exactly do you hope > > to achieve here? Aborting random I/O? > > Or, even worse, aborting I/O the driver uses internally and corrupt the > > internal workflow of the driver? > > > > We should simply disallow any ABORT TASK from userspace if the TaskMID > > is zero. And I would even argue to disabllow ABORT TASK from userspace > > completely, as the smid is never relayed to userland, and as such the > > user cannot know which task should be aborted. > > Hannes, > > This interface was added long time back in mpt2sas driver and I don't > have exact reason of adding this interface at that time. > But I know that this interface is still used by BRCM test team & few > customers only for some functionality and regression testing. Actually I have sent this patch for the testing purpose for the SAS storage that I'm working with now. Some of test platform could figure out which command has to be aborted or queried via debugfs and information from the device itself with some other methods. If the mpt3sas driver supports the targeted TMF for a single command, then it would be great for the testing. Thanks, Minwoo Im
On 19-07-23 16:57:49, Sreekanth Reddy wrote: > On Mon, Jul 15, 2019 at 11:43 AM Hannes Reinecke <hare@suse.de> wrote: > > I think this is fundamentally wrong. > > ABORT_TASK is used to abort a single task, which of course has to be > > known beforehand. If you don't know the task, what exactly do you hope > > to achieve here? Aborting random I/O? > > Or, even worse, aborting I/O the driver uses internally and corrupt the > > internal workflow of the driver? > > > > We should simply disallow any ABORT TASK from userspace if the TaskMID > > is zero. And I would even argue to disabllow ABORT TASK from userspace > > completely, as the smid is never relayed to userland, and as such the > > user cannot know which task should be aborted. > > Hannes, > > This interface was added long time back in mpt2sas driver and I don't > have exact reason of adding this interface at that time. > But I know that this interface is still used by BRCM test team & few > customers only for some functionality and regression testing. Sreekanth, and Hannes, Could you please give some review points on this? Thanks,
On Sun, Jul 14, 2019 at 9:14 AM Minwoo Im <minwoo.im@samsung.com> wrote: > > We can request task management IOCTL command(MPI2_FUNCTION_SCSI_TASK_MGMT) > to /dev/mpt3ctl. If the given task_type is either abort task or query > task, it may need a field named "Initiator Port Transfer Tag to Manage" > in the IU. > > Current code does not support to check target IPTT tag from the > tm_request. This patch introduces to check TaskMID given from the > userspace as a target tag. We have a rule of relationship between > (struct request *req->tag) and smid in mpt3sas_base.c: > > 3318 u16 > 3319 mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx, > 3320 struct scsi_cmnd *scmd) > 3321 { > 3322 struct scsiio_tracker *request = scsi_cmd_priv(scmd); > 3323 unsigned int tag = scmd->request->tag; > 3324 u16 smid; > 3325 > 3326 smid = tag + 1; > > So if we want to abort a request tagged #X, then we can pass (X + 1) to > this IOCTL handler. Otherwise, user space just can pass 0 TaskMID to > abort the first outstanding smid which is legacy behaviour. > > Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com> > Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com> > Cc: Sathya Prakash <sathya.prakash@broadcom.com> > Cc: James E.J. Bottomley <jejb@linux.ibm.com> > Cc: Martin K. Petersen <martin.petersen@oracle.com> > Cc: MPT-FusionLinux.pdl@broadcom.com > Signed-off-by: Minwoo Im <minwoo.im@samsung.com> Acked-by: Sreekanth Reddy <sreekanth.reddy@broadcom.com> > --- > drivers/scsi/mpt3sas/mpt3sas_ctl.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > index b2bb47c14d35..f6b8fd90610a 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > @@ -596,8 +596,16 @@ _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg, > if (priv_data->sas_target->handle != handle) > continue; > st = scsi_cmd_priv(scmd); > - tm_request->TaskMID = cpu_to_le16(st->smid); > - found = 1; > + > + /* > + * If the given TaskMID from the user space is zero, then the > + * first outstanding smid will be picked up. Otherwise, > + * targeted smid will be the one. > + */ > + if (!tm_request->TaskMID || tm_request->TaskMID == st->smid) { > + tm_request->TaskMID = cpu_to_le16(st->smid); > + found = 1; > + } > } > > if (!found) { > -- > 2.16.1 >
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c index b2bb47c14d35..f6b8fd90610a 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c @@ -596,8 +596,16 @@ _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg, if (priv_data->sas_target->handle != handle) continue; st = scsi_cmd_priv(scmd); - tm_request->TaskMID = cpu_to_le16(st->smid); - found = 1; + + /* + * If the given TaskMID from the user space is zero, then the + * first outstanding smid will be picked up. Otherwise, + * targeted smid will be the one. + */ + if (!tm_request->TaskMID || tm_request->TaskMID == st->smid) { + tm_request->TaskMID = cpu_to_le16(st->smid); + found = 1; + } } if (!found) {
We can request task management IOCTL command(MPI2_FUNCTION_SCSI_TASK_MGMT) to /dev/mpt3ctl. If the given task_type is either abort task or query task, it may need a field named "Initiator Port Transfer Tag to Manage" in the IU. Current code does not support to check target IPTT tag from the tm_request. This patch introduces to check TaskMID given from the userspace as a target tag. We have a rule of relationship between (struct request *req->tag) and smid in mpt3sas_base.c: 3318 u16 3319 mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx, 3320 struct scsi_cmnd *scmd) 3321 { 3322 struct scsiio_tracker *request = scsi_cmd_priv(scmd); 3323 unsigned int tag = scmd->request->tag; 3324 u16 smid; 3325 3326 smid = tag + 1; So if we want to abort a request tagged #X, then we can pass (X + 1) to this IOCTL handler. Otherwise, user space just can pass 0 TaskMID to abort the first outstanding smid which is legacy behaviour. Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com> Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com> Cc: Sathya Prakash <sathya.prakash@broadcom.com> Cc: James E.J. Bottomley <jejb@linux.ibm.com> Cc: Martin K. Petersen <martin.petersen@oracle.com> Cc: MPT-FusionLinux.pdl@broadcom.com Signed-off-by: Minwoo Im <minwoo.im@samsung.com> --- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)