* [PATCH 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + response failures
@ 2014-06-05 23:30 Nicholas A. Bellinger
2014-06-05 23:30 ` [PATCH 1/2] tcm_fc: Generate TASK_SET_FULL status for DataIN failures Nicholas A. Bellinger
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Nicholas A. Bellinger @ 2014-06-05 23:30 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi, fcoe-devel, Nicholas Bellinger
From: Nicholas Bellinger <nab@linux-iscsi.org>
Hi Vasu,
This series generates SAM_STAT_TASK_SET_FULL status for lport->tt.seq_send()
failures in DataIN + response status codepaths, which is done in order to get
the initiator to reduce it's current queue_depth thus reducing the number of
outstanding I/Os permitted in flight on the wire.
For the DataIN case, once a lport->tt.seq_send() failure occurs it will stop
attempting to send subsequent DataIN payload data, and immediately attempt to
send a response packet with SAM_STAT_TASK_SET_FULL status.
For the response case, once a lport->tt.seq_send() failure occurs it will
set SAM_STAT_TASK_SET_FULL status and return -ENOMEM to the target, forcing
the response to be requeued by target-core
Note this series has been compile tested only, so please review + test.
Thank you,
--nab
Nicholas Bellinger (2):
tcm_fc: Generate TASK_SET_FULL status for DataIN failures
tcm_fc: Generate TASK_SET_FULL status for response failures
drivers/target/tcm_fc/tfc_cmd.c | 19 ++++++++++++++++---
drivers/target/tcm_fc/tfc_io.c | 14 +++++++++++++-
2 files changed, 29 insertions(+), 4 deletions(-)
--
1.7.10.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] tcm_fc: Generate TASK_SET_FULL status for DataIN failures
2014-06-05 23:30 [PATCH 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + response failures Nicholas A. Bellinger
@ 2014-06-05 23:30 ` Nicholas A. Bellinger
2014-06-06 20:51 ` Vasu Dev
2014-06-05 23:30 ` [PATCH 2/2] tcm_fc: Generate TASK_SET_FULL status for response failures Nicholas A. Bellinger
2014-06-06 20:38 ` [PATCH 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + " Vasu Dev
2 siblings, 1 reply; 12+ messages in thread
From: Nicholas A. Bellinger @ 2014-06-05 23:30 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi, fcoe-devel, Nicholas Bellinger, Vasu Dev, Jun Wu
From: Nicholas Bellinger <nab@linux-iscsi.org>
This patch changes ft_queue_data_in() to set SAM_STAT_TASK_SET_FULL
status upon a lport->tt.seq_send() failure, where it will now stop
sending subsequent DataIN, and immediately attempt to send the
response with exception status.
Sending a response with SAM_STAT_TASK_SET_FULL status is useful in
order to signal the initiator that it should try to reduce it's
current queue_depth, to lower the number of outstanding I/Os on
the wire.
Also, add a check to skip sending DataIN if TASK_SET_FULL status
has already been set due to a response lport->tt.seq_send()
failure, that has asked target-core to requeue a response.
Reported-by: Vasu Dev <vasu.dev@linux.intel.com>
Cc: Vasu Dev <vasu.dev@linux.intel.com>
Cc: Jun Wu <jwu@stormojo.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
drivers/target/tcm_fc/tfc_io.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
index e415af3..140659f 100644
--- a/drivers/target/tcm_fc/tfc_io.c
+++ b/drivers/target/tcm_fc/tfc_io.c
@@ -82,6 +82,10 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
if (cmd->aborted)
return 0;
+
+ if (se_cmd->scsi_status == SAM_STAT_TASK_SET_FULL)
+ goto queue_status;
+
ep = fc_seq_exch(cmd->seq);
lport = ep->lp;
cmd->seq = lport->tt.seq_start_next(cmd->seq);
@@ -178,14 +182,22 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
FC_TYPE_FCP, f_ctl, fh_off);
error = lport->tt.seq_send(lport, seq, fp);
if (error) {
- /* XXX For now, initiator will retry */
pr_err_ratelimited("%s: Failed to send frame %p, "
"xid <0x%x>, remaining %zu, "
"lso_max <0x%x>\n",
__func__, fp, ep->xid,
remaining, lport->lso_max);
+ /*
+ * Generate a TASK_SET_FULL status to notify the
+ * initiator to reduce it's queue_depth, ignoring
+ * the rest of the data-in and immediately attempt
+ * to send the response.
+ */
+ se_cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
+ break;
}
}
+queue_status:
return ft_queue_status(se_cmd);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] tcm_fc: Generate TASK_SET_FULL status for response failures
2014-06-05 23:30 [PATCH 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + response failures Nicholas A. Bellinger
2014-06-05 23:30 ` [PATCH 1/2] tcm_fc: Generate TASK_SET_FULL status for DataIN failures Nicholas A. Bellinger
@ 2014-06-05 23:30 ` Nicholas A. Bellinger
[not found] ` <1402011002-20771-3-git-send-email-nab-PEzghdH756F8UrSeD/g0lQ@public.gmane.org>
2014-06-06 20:38 ` [PATCH 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + " Vasu Dev
2 siblings, 1 reply; 12+ messages in thread
From: Nicholas A. Bellinger @ 2014-06-05 23:30 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi, fcoe-devel, Nicholas Bellinger, Vasu Dev, Jun Wu
From: Nicholas Bellinger <nab@linux-iscsi.org>
This patch changes ft_queue_status() to set SAM_STAT_TASK_SET_FULL
status upon lport->tt.seq_send( failure, and return -EAGAIN to notify
target-core to attempt to requeue the response.
It also does the same for a fc_frame_alloc() failures, in order to
signal the initiator that it should try to reduce it's current
queue_depth, to lower the number of outstanding I/Os on the wire.
Reported-by: Vasu Dev <vasu.dev@linux.intel.com>
Cc: Vasu Dev <vasu.dev@linux.intel.com>
Cc: Jun Wu <jwu@stormojo.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
drivers/target/tcm_fc/tfc_cmd.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index f5fd515..5585038 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -128,6 +128,7 @@ int ft_queue_status(struct se_cmd *se_cmd)
struct fc_lport *lport;
struct fc_exch *ep;
size_t len;
+ int rc;
if (cmd->aborted)
return 0;
@@ -137,9 +138,10 @@ int ft_queue_status(struct se_cmd *se_cmd)
len = sizeof(*fcp) + se_cmd->scsi_sense_length;
fp = fc_frame_alloc(lport, len);
if (!fp) {
- /* XXX shouldn't just drop it - requeue and retry? */
- return 0;
+ se_cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
+ return -ENOMEM;
}
+
fcp = fc_frame_payload_get(fp, len);
memset(fcp, 0, len);
fcp->resp.fr_status = se_cmd->scsi_status;
@@ -170,7 +172,18 @@ int ft_queue_status(struct se_cmd *se_cmd)
fc_fill_fc_hdr(fp, FC_RCTL_DD_CMD_STATUS, ep->did, ep->sid, FC_TYPE_FCP,
FC_FC_EX_CTX | FC_FC_LAST_SEQ | FC_FC_END_SEQ, 0);
- lport->tt.seq_send(lport, cmd->seq, fp);
+ rc = lport->tt.seq_send(lport, cmd->seq, fp);
+ if (rc) {
+ pr_err_ratelimited("%s: Failed to send response frame %p, "
+ "xid <0x%x>\n", __func__, fp, ep->xid);
+ /*
+ * Generate a TASK_SET_FULL status to notify the initiator
+ * to reduce it's queue_depth after the se_cmd response has
+ * been re-queued by target-core.
+ */
+ se_cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
+ return -ENOMEM;
+ }
lport->tt.exch_done(cmd->seq);
return 0;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + response failures
2014-06-05 23:30 [PATCH 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + response failures Nicholas A. Bellinger
2014-06-05 23:30 ` [PATCH 1/2] tcm_fc: Generate TASK_SET_FULL status for DataIN failures Nicholas A. Bellinger
2014-06-05 23:30 ` [PATCH 2/2] tcm_fc: Generate TASK_SET_FULL status for response failures Nicholas A. Bellinger
@ 2014-06-06 20:38 ` Vasu Dev
2014-06-06 21:09 ` Nicholas A. Bellinger
2 siblings, 1 reply; 12+ messages in thread
From: Vasu Dev @ 2014-06-06 20:38 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: target-devel, linux-scsi, fcoe-devel, Nicholas Bellinger
On Thu, 2014-06-05 at 23:30 +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> Hi Vasu,
>
> This series generates SAM_STAT_TASK_SET_FULL status for lport->tt.seq_send()
> failures in DataIN + response status codepaths, which is done in order to get
> the initiator to reduce it's current queue_depth thus reducing the number of
> outstanding I/Os permitted in flight on the wire.
>
> For the DataIN case, once a lport->tt.seq_send() failure occurs it will stop
> attempting to send subsequent DataIN payload data, and immediately attempt to
> send a response packet with SAM_STAT_TASK_SET_FULL status.
>
> For the response case, once a lport->tt.seq_send() failure occurs it will
> set SAM_STAT_TASK_SET_FULL status and return -ENOMEM to the target, forcing
> the response to be requeued by target-core
>
Good change and it was missing in tcm_fc.
> Note this series has been compile tested only, so please review + test.
Just did review and basic test but should be stressed to really ensure
it does work & fixes issues resulting these patches.
Thanks,
Vasu
> Thank you,
>
> --nab
>
> Nicholas Bellinger (2):
> tcm_fc: Generate TASK_SET_FULL status for DataIN failures
> tcm_fc: Generate TASK_SET_FULL status for response failures
>
> drivers/target/tcm_fc/tfc_cmd.c | 19 ++++++++++++++++---
> drivers/target/tcm_fc/tfc_io.c | 14 +++++++++++++-
> 2 files changed, 29 insertions(+), 4 deletions(-)
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] tcm_fc: Generate TASK_SET_FULL status for DataIN failures
2014-06-05 23:30 ` [PATCH 1/2] tcm_fc: Generate TASK_SET_FULL status for DataIN failures Nicholas A. Bellinger
@ 2014-06-06 20:51 ` Vasu Dev
2014-06-06 21:02 ` Nicholas A. Bellinger
0 siblings, 1 reply; 12+ messages in thread
From: Vasu Dev @ 2014-06-06 20:51 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: target-devel, linux-scsi, fcoe-devel, Nicholas Bellinger, Jun Wu
On Thu, 2014-06-05 at 23:30 +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch changes ft_queue_data_in() to set SAM_STAT_TASK_SET_FULL
> status upon a lport->tt.seq_send() failure, where it will now stop
> sending subsequent DataIN, and immediately attempt to send the
> response with exception status.
>
> Sending a response with SAM_STAT_TASK_SET_FULL status is useful in
> order to signal the initiator that it should try to reduce it's
> current queue_depth, to lower the number of outstanding I/Os on
> the wire.
>
> Also, add a check to skip sending DataIN if TASK_SET_FULL status
> has already been set due to a response lport->tt.seq_send()
> failure, that has asked target-core to requeue a response.
>
> Reported-by: Vasu Dev <vasu.dev@linux.intel.com>
> Cc: Vasu Dev <vasu.dev@linux.intel.com>
> Cc: Jun Wu <jwu@stormojo.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
> drivers/target/tcm_fc/tfc_io.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
> index e415af3..140659f 100644
> --- a/drivers/target/tcm_fc/tfc_io.c
> +++ b/drivers/target/tcm_fc/tfc_io.c
> @@ -82,6 +82,10 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
>
> if (cmd->aborted)
> return 0;
> +
> + if (se_cmd->scsi_status == SAM_STAT_TASK_SET_FULL)
> + goto queue_status;
> +
> ep = fc_seq_exch(cmd->seq);
> lport = ep->lp;
> cmd->seq = lport->tt.seq_start_next(cmd->seq);
> @@ -178,14 +182,22 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
> FC_TYPE_FCP, f_ctl, fh_off);
> error = lport->tt.seq_send(lport, seq, fp);
> if (error) {
> - /* XXX For now, initiator will retry */
> pr_err_ratelimited("%s: Failed to send frame %p, "
> "xid <0x%x>, remaining %zu, "
> "lso_max <0x%x>\n",
> __func__, fp, ep->xid,
> remaining, lport->lso_max);
> + /*
> + * Generate a TASK_SET_FULL status to notify the
> + * initiator to reduce it's queue_depth, ignoring
> + * the rest of the data-in and immediately attempt
> + * to send the response.
> + */
I see above added check will drop rest of the frames but cannot find
notifying to initiator with the TASK_SET_FULL status as in comments
above, may be just the comment needs update otherwise code changes are
fine.
> + se_cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
> + break;
> }
> }
> +queue_status:
> return ft_queue_status(se_cmd);
> }
>
Reviewed-by: Vasu Dev <vasu.dev@intel.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] tcm_fc: Generate TASK_SET_FULL status for DataIN failures
2014-06-06 20:51 ` Vasu Dev
@ 2014-06-06 21:02 ` Nicholas A. Bellinger
2014-06-09 17:19 ` Vasu Dev
0 siblings, 1 reply; 12+ messages in thread
From: Nicholas A. Bellinger @ 2014-06-06 21:02 UTC (permalink / raw)
To: Vasu Dev; +Cc: fcoe-devel, Nicholas A. Bellinger, target-devel, linux-scsi
On Fri, 2014-06-06 at 13:51 -0700, Vasu Dev wrote:
> On Thu, 2014-06-05 at 23:30 +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
> >
> > This patch changes ft_queue_data_in() to set SAM_STAT_TASK_SET_FULL
> > status upon a lport->tt.seq_send() failure, where it will now stop
> > sending subsequent DataIN, and immediately attempt to send the
> > response with exception status.
> >
> > Sending a response with SAM_STAT_TASK_SET_FULL status is useful in
> > order to signal the initiator that it should try to reduce it's
> > current queue_depth, to lower the number of outstanding I/Os on
> > the wire.
> >
> > Also, add a check to skip sending DataIN if TASK_SET_FULL status
> > has already been set due to a response lport->tt.seq_send()
> > failure, that has asked target-core to requeue a response.
> >
> > Reported-by: Vasu Dev <vasu.dev-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > Cc: Vasu Dev <vasu.dev-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > Cc: Jun Wu <jwu-kEM2MQtWpIJWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
> > ---
> > drivers/target/tcm_fc/tfc_io.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
> > index e415af3..140659f 100644
> > --- a/drivers/target/tcm_fc/tfc_io.c
> > +++ b/drivers/target/tcm_fc/tfc_io.c
> > @@ -82,6 +82,10 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
> >
> > if (cmd->aborted)
> > return 0;
> > +
> > + if (se_cmd->scsi_status == SAM_STAT_TASK_SET_FULL)
> > + goto queue_status;
> > +
> > ep = fc_seq_exch(cmd->seq);
> > lport = ep->lp;
> > cmd->seq = lport->tt.seq_start_next(cmd->seq);
> > @@ -178,14 +182,22 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
> > FC_TYPE_FCP, f_ctl, fh_off);
> > error = lport->tt.seq_send(lport, seq, fp);
> > if (error) {
> > - /* XXX For now, initiator will retry */
> > pr_err_ratelimited("%s: Failed to send frame %p, "
> > "xid <0x%x>, remaining %zu, "
> > "lso_max <0x%x>\n",
> > __func__, fp, ep->xid,
> > remaining, lport->lso_max);
> > + /*
> > + * Generate a TASK_SET_FULL status to notify the
> > + * initiator to reduce it's queue_depth, ignoring
> > + * the rest of the data-in and immediately attempt
> > + * to send the response.
> > + */
>
> I see above added check will drop rest of the frames but cannot find
> notifying to initiator with the TASK_SET_FULL status as in comments
> above, may be just the comment needs update otherwise code changes are
> fine.
>
The break aborts the DataIN send loop and invokes ft_queue_status()
below in an attempt to send TASK_SET_FULL status.
If the ft_queue_status() -> lport->tt.seq_send() also fails, then
-ENOMEM will be returned to the target and a delayed re-queue attempt
will be made.
In any event, updating the comment above to be more precise..
> > + se_cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
> > + break;
> > }
> > }
> > +queue_status:
> > return ft_queue_status(se_cmd);
> > }
> >
>
> Reviewed-by: Vasu Dev <vasu.dev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
Thanks Vasu!
--nab
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tcm_fc: Generate TASK_SET_FULL status for response failures
[not found] ` <1402011002-20771-3-git-send-email-nab-PEzghdH756F8UrSeD/g0lQ@public.gmane.org>
@ 2014-06-06 21:03 ` Vasu Dev
2014-06-06 21:11 ` Nicholas A. Bellinger
0 siblings, 1 reply; 12+ messages in thread
From: Vasu Dev @ 2014-06-06 21:03 UTC (permalink / raw)
To: Nicholas A. Bellinger; +Cc: fcoe-devel, target-devel, linux-scsi
On Thu, 2014-06-05 at 23:30 +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
>
> This patch changes ft_queue_status() to set SAM_STAT_TASK_SET_FULL
> status upon lport->tt.seq_send( failure, and return -EAGAIN to notify
> target-core to attempt to requeue the response.
>
> It also does the same for a fc_frame_alloc() failures, in order to
> signal the initiator that it should try to reduce it's current
> queue_depth, to lower the number of outstanding I/Os on the wire.
>
> Reported-by: Vasu Dev <vasu.dev-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: Vasu Dev <vasu.dev-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: Jun Wu <jwu-kEM2MQtWpIJWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
> ---
> drivers/target/tcm_fc/tfc_cmd.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
> index f5fd515..5585038 100644
> --- a/drivers/target/tcm_fc/tfc_cmd.c
> +++ b/drivers/target/tcm_fc/tfc_cmd.c
> @@ -128,6 +128,7 @@ int ft_queue_status(struct se_cmd *se_cmd)
> struct fc_lport *lport;
> struct fc_exch *ep;
> size_t len;
> + int rc;
>
> if (cmd->aborted)
> return 0;
> @@ -137,9 +138,10 @@ int ft_queue_status(struct se_cmd *se_cmd)
> len = sizeof(*fcp) + se_cmd->scsi_sense_length;
> fp = fc_frame_alloc(lport, len);
> if (!fp) {
> - /* XXX shouldn't just drop it - requeue and retry? */
> - return 0;
> + se_cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
> + return -ENOMEM;
> }
> +
> fcp = fc_frame_payload_get(fp, len);
> memset(fcp, 0, len);
> fcp->resp.fr_status = se_cmd->scsi_status;
> @@ -170,7 +172,18 @@ int ft_queue_status(struct se_cmd *se_cmd)
> fc_fill_fc_hdr(fp, FC_RCTL_DD_CMD_STATUS, ep->did, ep->sid, FC_TYPE_FCP,
> FC_FC_EX_CTX | FC_FC_LAST_SEQ | FC_FC_END_SEQ, 0);
>
> - lport->tt.seq_send(lport, cmd->seq, fp);
> + rc = lport->tt.seq_send(lport, cmd->seq, fp);
> + if (rc) {
> + pr_err_ratelimited("%s: Failed to send response frame %p, "
> + "xid <0x%x>\n", __func__, fp, ep->xid);
May be should be pr_info_ratelimited since this is not really error and
IO could get aborted due to timeouts in any setups or stressed
workloads.
> + /*
> + * Generate a TASK_SET_FULL status to notify the initiator
> + * to reduce it's queue_depth after the se_cmd response has
> + * been re-queued by target-core.
> + */
> + se_cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
> + return -ENOMEM;
> + }
> lport->tt.exch_done(cmd->seq);
> return 0;
> }
Reviewed-by: Vasu Dev <vasu.dev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + response failures
2014-06-06 20:38 ` [PATCH 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + " Vasu Dev
@ 2014-06-06 21:09 ` Nicholas A. Bellinger
2014-06-09 17:13 ` Vasu Dev
0 siblings, 1 reply; 12+ messages in thread
From: Nicholas A. Bellinger @ 2014-06-06 21:09 UTC (permalink / raw)
To: Vasu Dev
Cc: Nicholas A. Bellinger, target-devel, linux-scsi, fcoe-devel, Jun Wu
On Fri, 2014-06-06 at 13:38 -0700, Vasu Dev wrote:
> On Thu, 2014-06-05 at 23:30 +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > Hi Vasu,
> >
> > This series generates SAM_STAT_TASK_SET_FULL status for lport->tt.seq_send()
> > failures in DataIN + response status codepaths, which is done in order to get
> > the initiator to reduce it's current queue_depth thus reducing the number of
> > outstanding I/Os permitted in flight on the wire.
> >
> > For the DataIN case, once a lport->tt.seq_send() failure occurs it will stop
> > attempting to send subsequent DataIN payload data, and immediately attempt to
> > send a response packet with SAM_STAT_TASK_SET_FULL status.
> >
> > For the response case, once a lport->tt.seq_send() failure occurs it will
> > set SAM_STAT_TASK_SET_FULL status and return -ENOMEM to the target, forcing
> > the response to be requeued by target-core
> >
>
> Good change and it was missing in tcm_fc.
>
> > Note this series has been compile tested only, so please review + test.
>
> Just did review and basic test but should be stressed to really ensure
> it does work & fixes issues resulting these patches.
So if you don't mind I'll go ahead and queue these up for now in
target-pending/for-next, given they are pretty straight-forward fixes.
If they end up being problematic, they can be dropped before the v3.16
PULL request goes out next week.
Jun can you please test both cases..? Eg: the first with an explicitly
reduced fcoe initiator queue_depth setting mentioned earlier, and the
second with these two patches to return TASK_SET_FULL when the sequence
failures start occurring.
Thanks,
--nab
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tcm_fc: Generate TASK_SET_FULL status for response failures
2014-06-06 21:03 ` Vasu Dev
@ 2014-06-06 21:11 ` Nicholas A. Bellinger
2014-06-09 17:16 ` Vasu Dev
0 siblings, 1 reply; 12+ messages in thread
From: Nicholas A. Bellinger @ 2014-06-06 21:11 UTC (permalink / raw)
To: Vasu Dev
Cc: Nicholas A. Bellinger, target-devel, linux-scsi, fcoe-devel, Jun Wu
On Fri, 2014-06-06 at 14:03 -0700, Vasu Dev wrote:
> On Thu, 2014-06-05 at 23:30 +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch changes ft_queue_status() to set SAM_STAT_TASK_SET_FULL
> > status upon lport->tt.seq_send( failure, and return -EAGAIN to notify
> > target-core to attempt to requeue the response.
> >
> > It also does the same for a fc_frame_alloc() failures, in order to
> > signal the initiator that it should try to reduce it's current
> > queue_depth, to lower the number of outstanding I/Os on the wire.
> >
> > Reported-by: Vasu Dev <vasu.dev@linux.intel.com>
> > Cc: Vasu Dev <vasu.dev@linux.intel.com>
> > Cc: Jun Wu <jwu@stormojo.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> > drivers/target/tcm_fc/tfc_cmd.c | 19 ++++++++++++++++---
> > 1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
> > index f5fd515..5585038 100644
> > --- a/drivers/target/tcm_fc/tfc_cmd.c
> > +++ b/drivers/target/tcm_fc/tfc_cmd.c
> > @@ -128,6 +128,7 @@ int ft_queue_status(struct se_cmd *se_cmd)
> > struct fc_lport *lport;
> > struct fc_exch *ep;
> > size_t len;
> > + int rc;
> >
> > if (cmd->aborted)
> > return 0;
> > @@ -137,9 +138,10 @@ int ft_queue_status(struct se_cmd *se_cmd)
> > len = sizeof(*fcp) + se_cmd->scsi_sense_length;
> > fp = fc_frame_alloc(lport, len);
> > if (!fp) {
> > - /* XXX shouldn't just drop it - requeue and retry? */
> > - return 0;
> > + se_cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
> > + return -ENOMEM;
> > }
> > +
> > fcp = fc_frame_payload_get(fp, len);
> > memset(fcp, 0, len);
> > fcp->resp.fr_status = se_cmd->scsi_status;
> > @@ -170,7 +172,18 @@ int ft_queue_status(struct se_cmd *se_cmd)
> > fc_fill_fc_hdr(fp, FC_RCTL_DD_CMD_STATUS, ep->did, ep->sid, FC_TYPE_FCP,
> > FC_FC_EX_CTX | FC_FC_LAST_SEQ | FC_FC_END_SEQ, 0);
> >
> > - lport->tt.seq_send(lport, cmd->seq, fp);
> > + rc = lport->tt.seq_send(lport, cmd->seq, fp);
> > + if (rc) {
> > + pr_err_ratelimited("%s: Failed to send response frame %p, "
> > + "xid <0x%x>\n", __func__, fp, ep->xid);
>
> May be should be pr_info_ratelimited since this is not really error and
> IO could get aborted due to timeouts in any setups or stressed
> workloads.
<nod>, changing this to pr_info_ratelimited and doing the same in
ft_queue_data_in() well.
>
> > + /*
> > + * Generate a TASK_SET_FULL status to notify the initiator
> > + * to reduce it's queue_depth after the se_cmd response has
> > + * been re-queued by target-core.
> > + */
> > + se_cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
> > + return -ENOMEM;
> > + }
> > lport->tt.exch_done(cmd->seq);
> > return 0;
> > }
>
> Reviewed-by: Vasu Dev <vasu.dev@intel.com>
>
Thanks Vasu!
--nab
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + response failures
2014-06-06 21:09 ` Nicholas A. Bellinger
@ 2014-06-09 17:13 ` Vasu Dev
0 siblings, 0 replies; 12+ messages in thread
From: Vasu Dev @ 2014-06-09 17:13 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Nicholas A. Bellinger, target-devel, linux-scsi, fcoe-devel, Jun Wu
On Fri, 2014-06-06 at 14:09 -0700, Nicholas A. Bellinger wrote:
>
> So if you don't mind I'll go ahead and queue these up for now in
> target-pending/for-next, given they are pretty straight-forward fixes.
>
> If they end up being problematic, they can be dropped before the v3.16
> PULL request goes out next week.
All make sense, so go head with the push and I already added reviewed by
to both patches.
//Vasu
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tcm_fc: Generate TASK_SET_FULL status for response failures
2014-06-06 21:11 ` Nicholas A. Bellinger
@ 2014-06-09 17:16 ` Vasu Dev
0 siblings, 0 replies; 12+ messages in thread
From: Vasu Dev @ 2014-06-09 17:16 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Nicholas A. Bellinger, target-devel, linux-scsi, fcoe-devel, Jun Wu
On Fri, 2014-06-06 at 14:11 -0700, Nicholas A. Bellinger wrote:
> <nod>, changing this to pr_info_ratelimited and doing the same in
> ft_queue_data_in() well.
Since doing more places and therefore can be done in separate patch.
//Vasu
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] tcm_fc: Generate TASK_SET_FULL status for DataIN failures
2014-06-06 21:02 ` Nicholas A. Bellinger
@ 2014-06-09 17:19 ` Vasu Dev
0 siblings, 0 replies; 12+ messages in thread
From: Vasu Dev @ 2014-06-09 17:19 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Nicholas A. Bellinger, target-devel, linux-scsi, fcoe-devel, Jun Wu
On Fri, 2014-06-06 at 14:02 -0700, Nicholas A. Bellinger wrote:
> The break aborts the DataIN send loop and invokes ft_queue_status()
> below in an attempt to send TASK_SET_FULL status.
>
> If the ft_queue_status() -> lport->tt.seq_send() also fails, then
> -ENOMEM will be returned to the target and a delayed re-queue attempt
> will be made.
I see.
> In any event, updating the comment above to be more precise
I'm ok with leaving comment as is for now, just to avoid patch re-spin
for this after above clarification. Thanks for detailed clarification
//Vasu
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-06-09 17:19 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-05 23:30 [PATCH 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + response failures Nicholas A. Bellinger
2014-06-05 23:30 ` [PATCH 1/2] tcm_fc: Generate TASK_SET_FULL status for DataIN failures Nicholas A. Bellinger
2014-06-06 20:51 ` Vasu Dev
2014-06-06 21:02 ` Nicholas A. Bellinger
2014-06-09 17:19 ` Vasu Dev
2014-06-05 23:30 ` [PATCH 2/2] tcm_fc: Generate TASK_SET_FULL status for response failures Nicholas A. Bellinger
[not found] ` <1402011002-20771-3-git-send-email-nab-PEzghdH756F8UrSeD/g0lQ@public.gmane.org>
2014-06-06 21:03 ` Vasu Dev
2014-06-06 21:11 ` Nicholas A. Bellinger
2014-06-09 17:16 ` Vasu Dev
2014-06-06 20:38 ` [PATCH 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + " Vasu Dev
2014-06-06 21:09 ` Nicholas A. Bellinger
2014-06-09 17:13 ` Vasu Dev
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.