Re: [PATCH] LIBISCSI: Alleviate NOP transmission request upon xmit failure

[Mike Christie <michaelc@cs.wisc.edu>]

On 03/14/2012 06:50 PM, Eddie Wai wrote:
> During heavy I/O transmission, it was observed that network packets
> can get dropped when no link flow control is enabled.  When this happens,
> I/O completions can exceed the default NOP transmission of 5s while the
> hw send queue resource backs up.  When the queue gets full, NOP
> transmission requests will also get blocked.  It was observed that
> the NOP transmission requests will keep repeatedly try to send out
> the NOP while holding the session lock.  This is very intrusive as the
> requests are being called on every timer execution since the last_ping
> parameter doesn't get updated upon transmission failure.  This creates a
> tremendous bottleneck especially when the connection is about to get torn down.
> 
> This patch alleviates the pounding of the NOP transmission in the
> iscsi_check_transport_timeouts routine by injecting an artifical 1s delay
> in between each NOP transmission requests due to failures upon timeout.
> 
> There is no need to keep pounding on to request this data provoking NOP
> transmission continuously when the transmit queue is full.
> 
> Please review and comment.  Thanks.
> 
> 
> Signed-off-by: Eddie Wai <eddie.wai@broadcom.com>
> ---
>  drivers/scsi/libiscsi.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 82c3fd4..f1141a8 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -940,13 +940,14 @@ static void iscsi_tmf_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
>  	wake_up(&conn->ehwait);
>  }
>  
> -static void iscsi_send_nopout(struct iscsi_conn *conn, struct iscsi_nopin *rhdr)
> +static struct iscsi_task *iscsi_send_nopout(struct iscsi_conn *conn,
> +					    struct iscsi_nopin *rhdr)
>  {
>          struct iscsi_nopout hdr;
>  	struct iscsi_task *task;
>  
>  	if (!rhdr && conn->ping_task)
> -		return;
> +		return NULL;
>  
>  	memset(&hdr, 0, sizeof(struct iscsi_nopout));
>  	hdr.opcode = ISCSI_OP_NOOP_OUT | ISCSI_OP_IMMEDIATE;
> @@ -967,6 +968,7 @@ static void iscsi_send_nopout(struct iscsi_conn *conn, struct iscsi_nopin *rhdr)
>  		conn->ping_task = task;
>  		conn->last_ping = jiffies;
>  	}
> +	return task;
>  }
>  
>  static int iscsi_nop_out_rsp(struct iscsi_task *task,
> @@ -2059,8 +2061,11 @@ static void iscsi_check_transport_timeouts(unsigned long data)
>  	if (time_before_eq(last_recv + recv_timeout, jiffies)) {
>  		/* send a ping to try to provoke some traffic */
>  		ISCSI_DBG_CONN(conn, "Sending nopout as ping\n");
> -		iscsi_send_nopout(conn, NULL);
> -		next_timeout = conn->last_ping + (conn->ping_timeout * HZ);
> +		if (iscsi_send_nopout(conn, NULL))
> +			next_timeout = conn->last_ping +
> +				       (conn->ping_timeout * HZ);

Once we send a ping, we should not run this timer again until it has
timed out Why is the ping not timing out and then why are not hitting
the check above this that just returns?

Is the timer firing early, so we keep hitting the iscsi_send_nopout path?