* Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error handling function
2016-01-08 0:40 [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error handling function K. Y. Srinivasan
@ 2016-01-07 23:48 ` James Bottomley
2016-01-08 18:58 ` KY Srinivasan
0 siblings, 1 reply; 7+ messages in thread
From: James Bottomley @ 2016-01-07 23:48 UTC (permalink / raw)
To: K. Y. Srinivasan, gregkh, linux-kernel, devel, ohering,
jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen, hare
Cc: stable
On Thu, 2016-01-07 at 16:40 -0800, K. Y. Srinivasan wrote:
> The macro startget_to_rport() can return NULL; handle that case
> properly.
OK, can we unwind why you think you could possibly need this? It would
mean that fc_timed_out was called for a non-FC device, which was
thought to be an impossibility when the fc transport class was
designed.
James
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Cc: <stable@vger.kernel.org>
> ---
> drivers/scsi/scsi_transport_fc.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_fc.c
> b/drivers/scsi/scsi_transport_fc.c
> index 24eaaf6..42a908f 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -2081,7 +2081,7 @@ fc_timed_out(struct scsi_cmnd *scmd)
> {
> struct fc_rport *rport = starget_to_rport(scsi_target(scmd
> ->device));
>
> - if (rport->port_state == FC_PORTSTATE_BLOCKED)
> + if ((rport == NULL) || (rport->port_state ==
> FC_PORTSTATE_BLOCKED))
> return BLK_EH_RESET_TIMER;
>
> return BLK_EH_NOT_HANDLED;
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error handling function
@ 2016-01-08 0:40 K. Y. Srinivasan
2016-01-07 23:48 ` James Bottomley
0 siblings, 1 reply; 7+ messages in thread
From: K. Y. Srinivasan @ 2016-01-08 0:40 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, ohering, jbottomley, hch, linux-scsi,
apw, vkuznets, jasowang, martin.petersen, hare
Cc: K. Y. Srinivasan, stable
The macro startget_to_rport() can return NULL; handle that case
properly.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
drivers/scsi/scsi_transport_fc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 24eaaf6..42a908f 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -2081,7 +2081,7 @@ fc_timed_out(struct scsi_cmnd *scmd)
{
struct fc_rport *rport = starget_to_rport(scsi_target(scmd->device));
- if (rport->port_state == FC_PORTSTATE_BLOCKED)
+ if ((rport == NULL) || (rport->port_state == FC_PORTSTATE_BLOCKED))
return BLK_EH_RESET_TIMER;
return BLK_EH_NOT_HANDLED;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error handling function
2016-01-07 23:48 ` James Bottomley
@ 2016-01-08 18:58 ` KY Srinivasan
2016-01-08 19:20 ` James Bottomley
0 siblings, 1 reply; 7+ messages in thread
From: KY Srinivasan @ 2016-01-08 18:58 UTC (permalink / raw)
To: James Bottomley, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
ohering@suse.com, jbottomley@parallels.com, hch@infradead.org,
linux-scsi@vger.kernel.org, apw@canonical.com,
vkuznets@redhat.com, jasowang@redhat.com,
martin.petersen@oracle.com, hare@suse.de
Cc: stable@vger.kernel.org
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Thursday, January 7, 2016 3:49 PM
> To: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; ohering@suse.com;
> jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org;
> apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com;
> martin.petersen@oracle.com; hare@suse.de
> Cc: stable@vger.kernel.org
> Subject: Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error
> handling function
>
> On Thu, 2016-01-07 at 16:40 -0800, K. Y. Srinivasan wrote:
> > The macro startget_to_rport() can return NULL; handle that case
> > properly.
>
> OK, can we unwind why you think you could possibly need this? It would
> mean that fc_timed_out was called for a non-FC device, which was
> thought to be an impossibility when the fc transport class was
> designed.
As you know, on Hyper-V, FC devices are handled exactly like normal scsi
devices and the only additional information that is provided for FC devices
is the WWN for port and node. Till recently, I was not publishing the WWN in
the guest and so I was not even using the FC transport. Recently, I implemented
support for publishing the WWN in the guest and for that I am using the FC transport
for FC hosts. When an FC LUN is dynamically removed, sometimes I see the timeout
occurring and since there is no rport associated with these devices I am hitting the issue
this patch is addressing. I could have addressed this problem by establishing a storvsc specific
time out function even for FC devices - the same timeout function that I currently use for scsi
devices - storvsc_eh_timed_out(). I chose to instead fix the fc_timed_out() function since
the code was not handling a possible condition.
Regards,
K. Y
>
> James
>
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Cc: <stable@vger.kernel.org>
> > ---
> > drivers/scsi/scsi_transport_fc.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/scsi/scsi_transport_fc.c
> > b/drivers/scsi/scsi_transport_fc.c
> > index 24eaaf6..42a908f 100644
> > --- a/drivers/scsi/scsi_transport_fc.c
> > +++ b/drivers/scsi/scsi_transport_fc.c
> > @@ -2081,7 +2081,7 @@ fc_timed_out(struct scsi_cmnd *scmd)
> > {
> > struct fc_rport *rport = starget_to_rport(scsi_target(scmd
> > ->device));
> >
> > - if (rport->port_state == FC_PORTSTATE_BLOCKED)
> > + if ((rport == NULL) || (rport->port_state ==
> > FC_PORTSTATE_BLOCKED))
> > return BLK_EH_RESET_TIMER;
> >
> > return BLK_EH_NOT_HANDLED;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error handling function
2016-01-08 18:58 ` KY Srinivasan
@ 2016-01-08 19:20 ` James Bottomley
2016-01-08 20:12 ` KY Srinivasan
0 siblings, 1 reply; 7+ messages in thread
From: James Bottomley @ 2016-01-08 19:20 UTC (permalink / raw)
To: KY Srinivasan, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
ohering@suse.com, jbottomley@parallels.com, hch@infradead.org,
linux-scsi@vger.kernel.org, apw@canonical.com,
vkuznets@redhat.com, jasowang@redhat.com,
martin.petersen@oracle.com, hare@suse.de
Cc: stable@vger.kernel.org
On Fri, 2016-01-08 at 18:58 +0000, KY Srinivasan wrote:
>
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com
> > ]
> > Sent: Thursday, January 7, 2016 3:49 PM
> > To: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org;
> > linux-
> > kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > ohering@suse.com;
> > jbottomley@parallels.com; hch@infradead.org;
> > linux-scsi@vger.kernel.org;
> > apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com;
> > martin.petersen@oracle.com; hare@suse.de
> > Cc: stable@vger.kernel.org
> > Subject: Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the
> > error
> > handling function
> >
> > On Thu, 2016-01-07 at 16:40 -0800, K. Y. Srinivasan wrote:
> > > The macro startget_to_rport() can return NULL; handle that case
> > > properly.
> >
> > OK, can we unwind why you think you could possibly need this? It
> > would
> > mean that fc_timed_out was called for a non-FC device, which was
> > thought to be an impossibility when the fc transport class was
> > designed.
>
> As you know, on Hyper-V, FC devices are handled exactly like normal
> scsi devices and the only additional information that is provided for
> FC devices is the WWN for port and node. Till recently, I was not
> publishing the WWN in the guest and so I was not even using the FC
> transport. Recently, I implemented support for publishing the WWN in
> the guest and for that I am using the FC transport for FC hosts. When
> an FC LUN is dynamically removed, sometimes I see the timeout occurri
> ng and since there is no rport associated with these devices I am
> hitting the issue this patch is addressing. I could have addressed
> this problem by establishing a storvsc specific time out function
> even for FC devices - the same timeout function that I currently use
> for scsi devices - storvsc_eh_timed_out(). I chose to instead fix
> the fc_timed_out() function since the code was not handling a
> possible condition.
OK, so the specific problem is that the device is partly torn down when
the timeout fires? I'm having a hard time seeing how we get a null
rport in that case. The starget_to_rport() can only return NULL if the
parent isn't an rport ... that shouldn't depend on the state of the FC
device because the parent is torn down after the child.
In any case, returning BLK_EH_RESET_TIMER will cause all sorts of
problems because it resets the timer to fire again for the device.
What you want is something to return BLK_EH_HANDLED which will just
complete the request ... probably at a generic level, since this
doesn't sound to be specific to FC.
Something like the below ... assuming the teardown issue is the real
problem.
James
---
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 984ddcb..3c514c6 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -273,6 +273,10 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
struct Scsi_Host *host = scmd->device->host;
+ /* timeout for an already dead device, just kill the request */
+ if (scmd->device->sdev_state == SDEV_DEL)
+ return BLK_EH_HANDLED;
+
trace_scsi_dispatch_cmd_timeout(scmd);
scsi_log_completion(scmd, TIMEOUT_ERROR);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error handling function
2016-01-08 19:20 ` James Bottomley
@ 2016-01-08 20:12 ` KY Srinivasan
2016-01-08 20:26 ` James Bottomley
0 siblings, 1 reply; 7+ messages in thread
From: KY Srinivasan @ 2016-01-08 20:12 UTC (permalink / raw)
To: James Bottomley, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
ohering@suse.com, jbottomley@parallels.com, hch@infradead.org,
linux-scsi@vger.kernel.org, apw@canonical.com,
vkuznets@redhat.com, jasowang@redhat.com,
martin.petersen@oracle.com, hare@suse.de
Cc: stable@vger.kernel.org
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Friday, January 8, 2016 11:21 AM
> To: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; ohering@suse.com;
> jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org;
> apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com;
> martin.petersen@oracle.com; hare@suse.de
> Cc: stable@vger.kernel.org
> Subject: Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error
> handling function
>
> On Fri, 2016-01-08 at 18:58 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: James Bottomley
> [mailto:James.Bottomley@HansenPartnership.com
> > > ]
> > > Sent: Thursday, January 7, 2016 3:49 PM
> > > To: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org;
> > > linux-
> > > kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > ohering@suse.com;
> > > jbottomley@parallels.com; hch@infradead.org;
> > > linux-scsi@vger.kernel.org;
> > > apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com;
> > > martin.petersen@oracle.com; hare@suse.de
> > > Cc: stable@vger.kernel.org
> > > Subject: Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the
> > > error
> > > handling function
> > >
> > > On Thu, 2016-01-07 at 16:40 -0800, K. Y. Srinivasan wrote:
> > > > The macro startget_to_rport() can return NULL; handle that case
> > > > properly.
> > >
> > > OK, can we unwind why you think you could possibly need this? It
> > > would
> > > mean that fc_timed_out was called for a non-FC device, which was
> > > thought to be an impossibility when the fc transport class was
> > > designed.
> >
> > As you know, on Hyper-V, FC devices are handled exactly like normal
> > scsi devices and the only additional information that is provided for
> > FC devices is the WWN for port and node. Till recently, I was not
> > publishing the WWN in the guest and so I was not even using the FC
> > transport. Recently, I implemented support for publishing the WWN in
> > the guest and for that I am using the FC transport for FC hosts. When
> > an FC LUN is dynamically removed, sometimes I see the timeout occurri
> > ng and since there is no rport associated with these devices I am
> > hitting the issue this patch is addressing. I could have addressed
> > this problem by establishing a storvsc specific time out function
> > even for FC devices - the same timeout function that I currently use
> > for scsi devices - storvsc_eh_timed_out(). I chose to instead fix
> > the fc_timed_out() function since the code was not handling a
> > possible condition.
>
> OK, so the specific problem is that the device is partly torn down when
> the timeout fires? I'm having a hard time seeing how we get a null
> rport in that case. The starget_to_rport() can only return NULL if the
> parent isn't an rport ... that shouldn't depend on the state of the FC
> device because the parent is torn down after the child.
In our case, the parent is not an rport since I don't invoke fc_remote_port_add() and
so I do get a NULL value from the starget_to_rport().
>
> In any case, returning BLK_EH_RESET_TIMER will cause all sorts of
> problems because it resets the timer to fire again for the device.
> What you want is something to return BLK_EH_HANDLED which will just
> complete the request ... probably at a generic level, since this
> doesn't sound to be specific to FC.
On Hyper-V, the host implements a variety of recovery strategies and for that reason,
the eh_timed_out handler for standard scsi devices will effectively have infinite timeout value:
storvsc_eh_timed_out() just resets the timer. This is the behavior I wanted for the FC devices as well.
K. Y
>
> Something like the below ... assuming the teardown issue is the real
> problem.
>
> James
>
> ---
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 984ddcb..3c514c6 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -273,6 +273,10 @@ enum blk_eh_timer_return scsi_times_out(struct
> request *req)
> enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
> struct Scsi_Host *host = scmd->device->host;
>
> + /* timeout for an already dead device, just kill the request */
> + if (scmd->device->sdev_state == SDEV_DEL)
> + return BLK_EH_HANDLED;
> +
> trace_scsi_dispatch_cmd_timeout(scmd);
> scsi_log_completion(scmd, TIMEOUT_ERROR);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error handling function
2016-01-08 20:12 ` KY Srinivasan
@ 2016-01-08 20:26 ` James Bottomley
2016-01-08 21:35 ` KY Srinivasan
0 siblings, 1 reply; 7+ messages in thread
From: James Bottomley @ 2016-01-08 20:26 UTC (permalink / raw)
To: KY Srinivasan, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
ohering@suse.com, jbottomley@parallels.com, hch@infradead.org,
linux-scsi@vger.kernel.org, apw@canonical.com,
vkuznets@redhat.com, jasowang@redhat.com,
martin.petersen@oracle.com, hare@suse.de
Cc: stable@vger.kernel.org
On Fri, 2016-01-08 at 20:12 +0000, KY Srinivasan wrote:
>
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com
> > ]
> > Sent: Friday, January 8, 2016 11:21 AM
> > To: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org;
> > linux-
> > kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > ohering@suse.com;
> > jbottomley@parallels.com; hch@infradead.org;
> > linux-scsi@vger.kernel.org;
> > apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com;
> > martin.petersen@oracle.com; hare@suse.de
> > Cc: stable@vger.kernel.org
> > Subject: Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the
> > error
> > handling function
> >
> > On Fri, 2016-01-08 at 18:58 +0000, KY Srinivasan wrote:
> > >
> > > > -----Original Message-----
> > > > From: James Bottomley
> > [mailto:James.Bottomley@HansenPartnership.com
> > > > ]
> > > > Sent: Thursday, January 7, 2016 3:49 PM
> > > > To: KY Srinivasan <kys@microsoft.com>;
> > > > gregkh@linuxfoundation.org;
> > > > linux-
> > > > kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > > ohering@suse.com;
> > > > jbottomley@parallels.com; hch@infradead.org;
> > > > linux-scsi@vger.kernel.org;
> > > > apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com;
> > > > martin.petersen@oracle.com; hare@suse.de
> > > > Cc: stable@vger.kernel.org
> > > > Subject: Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in
> > > > the
> > > > error
> > > > handling function
> > > >
> > > > On Thu, 2016-01-07 at 16:40 -0800, K. Y. Srinivasan wrote:
> > > > > The macro startget_to_rport() can return NULL; handle that
> > > > > case
> > > > > properly.
> > > >
> > > > OK, can we unwind why you think you could possibly need this?
> > > > It
> > > > would
> > > > mean that fc_timed_out was called for a non-FC device, which
> > > > was
> > > > thought to be an impossibility when the fc transport class was
> > > > designed.
> > >
> > > As you know, on Hyper-V, FC devices are handled exactly like
> > > normal
> > > scsi devices and the only additional information that is provided
> > > for
> > > FC devices is the WWN for port and node. Till recently, I was not
> > > publishing the WWN in the guest and so I was not even using the
> > > FC
> > > transport. Recently, I implemented support for publishing the WWN
> > > in
> > > the guest and for that I am using the FC transport for FC hosts.
> > > When
> > > an FC LUN is dynamically removed, sometimes I see the timeout
> > > occurri
> > > ng and since there is no rport associated with these devices I am
> > > hitting the issue this patch is addressing. I could have
> > > addressed
> > > this problem by establishing a storvsc specific time out function
> > > even for FC devices - the same timeout function that I currently
> > > use
> > > for scsi devices - storvsc_eh_timed_out(). I chose to instead
> > > fix
> > > the fc_timed_out() function since the code was not handling a
> > > possible condition.
> >
> > OK, so the specific problem is that the device is partly torn down
> > when
> > the timeout fires? I'm having a hard time seeing how we get a null
> > rport in that case. The starget_to_rport() can only return NULL if
> > the
> > parent isn't an rport ... that shouldn't depend on the state of the
> > FC
> > device because the parent is torn down after the child.
>
> In our case, the parent is not an rport since I don't invoke
> fc_remote_port_add() and so I do get a NULL value from the
> starget_to_rport().
OK, so it's nothing to do with teardown? I'm going to need the FC
people to comment on this. The transport class was apparently designed
to allow use without rports. However, there are several places where
we assume rports are present: The times out and the port block
interface ... I'm betting all current users are rport otherwise we
would have spotted this problem sooner.
> > In any case, returning BLK_EH_RESET_TIMER will cause all sorts of
> > problems because it resets the timer to fire again for the device.
> > What you want is something to return BLK_EH_HANDLED which will
> > just
> > complete the request ... probably at a generic level, since this
> > doesn't sound to be specific to FC.
>
> On Hyper-V, the host implements a variety of recovery strategies and
> for that reason, the eh_timed_out handler for standard scsi devices
> will effectively have infinite timeout value: storvsc_eh_timed_out()
> just resets the timer. This is the behavior I wanted for the FC
> devices as well.
All the world isn't hyper-v. If we change something in the generic
interface, it needs to work for everyone. To me it looks like
fc_timed_out is designed to support the port block function. If we
assume port block is not supported for non-rport devices, then
fc_timed_out should be returning BLK_EH_NOT_HANDLED for the non-rport
case.
James
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error handling function
2016-01-08 20:26 ` James Bottomley
@ 2016-01-08 21:35 ` KY Srinivasan
0 siblings, 0 replies; 7+ messages in thread
From: KY Srinivasan @ 2016-01-08 21:35 UTC (permalink / raw)
To: James Bottomley, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
ohering@suse.com, jbottomley@parallels.com, hch@infradead.org,
linux-scsi@vger.kernel.org, apw@canonical.com,
vkuznets@redhat.com, jasowang@redhat.com,
martin.petersen@oracle.com, hare@suse.de
Cc: stable@vger.kernel.org
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Friday, January 8, 2016 12:27 PM
> To: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; ohering@suse.com;
> jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org;
> apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com;
> martin.petersen@oracle.com; hare@suse.de
> Cc: stable@vger.kernel.org
> Subject: Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error
> handling function
>
> On Fri, 2016-01-08 at 20:12 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: James Bottomley
> [mailto:James.Bottomley@HansenPartnership.com
> > > ]
> > > Sent: Friday, January 8, 2016 11:21 AM
> > > To: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org;
> > > linux-
> > > kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > ohering@suse.com;
> > > jbottomley@parallels.com; hch@infradead.org;
> > > linux-scsi@vger.kernel.org;
> > > apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com;
> > > martin.petersen@oracle.com; hare@suse.de
> > > Cc: stable@vger.kernel.org
> > > Subject: Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the
> > > error
> > > handling function
> > >
> > > On Fri, 2016-01-08 at 18:58 +0000, KY Srinivasan wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: James Bottomley
> > > [mailto:James.Bottomley@HansenPartnership.com
> > > > > ]
> > > > > Sent: Thursday, January 7, 2016 3:49 PM
> > > > > To: KY Srinivasan <kys@microsoft.com>;
> > > > > gregkh@linuxfoundation.org;
> > > > > linux-
> > > > > kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > > > ohering@suse.com;
> > > > > jbottomley@parallels.com; hch@infradead.org;
> > > > > linux-scsi@vger.kernel.org;
> > > > > apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com;
> > > > > martin.petersen@oracle.com; hare@suse.de
> > > > > Cc: stable@vger.kernel.org
> > > > > Subject: Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in
> > > > > the
> > > > > error
> > > > > handling function
> > > > >
> > > > > On Thu, 2016-01-07 at 16:40 -0800, K. Y. Srinivasan wrote:
> > > > > > The macro startget_to_rport() can return NULL; handle that
> > > > > > case
> > > > > > properly.
> > > > >
> > > > > OK, can we unwind why you think you could possibly need this?
> > > > > It
> > > > > would
> > > > > mean that fc_timed_out was called for a non-FC device, which
> > > > > was
> > > > > thought to be an impossibility when the fc transport class was
> > > > > designed.
> > > >
> > > > As you know, on Hyper-V, FC devices are handled exactly like
> > > > normal
> > > > scsi devices and the only additional information that is provided
> > > > for
> > > > FC devices is the WWN for port and node. Till recently, I was not
> > > > publishing the WWN in the guest and so I was not even using the
> > > > FC
> > > > transport. Recently, I implemented support for publishing the WWN
> > > > in
> > > > the guest and for that I am using the FC transport for FC hosts.
> > > > When
> > > > an FC LUN is dynamically removed, sometimes I see the timeout
> > > > occurri
> > > > ng and since there is no rport associated with these devices I am
> > > > hitting the issue this patch is addressing. I could have
> > > > addressed
> > > > this problem by establishing a storvsc specific time out function
> > > > even for FC devices - the same timeout function that I currently
> > > > use
> > > > for scsi devices - storvsc_eh_timed_out(). I chose to instead
> > > > fix
> > > > the fc_timed_out() function since the code was not handling a
> > > > possible condition.
> > >
> > > OK, so the specific problem is that the device is partly torn down
> > > when
> > > the timeout fires? I'm having a hard time seeing how we get a null
> > > rport in that case. The starget_to_rport() can only return NULL if
> > > the
> > > parent isn't an rport ... that shouldn't depend on the state of the
> > > FC
> > > device because the parent is torn down after the child.
> >
> > In our case, the parent is not an rport since I don't invoke
> > fc_remote_port_add() and so I do get a NULL value from the
> > starget_to_rport().
>
> OK, so it's nothing to do with teardown? I'm going to need the FC
> people to comment on this. The transport class was apparently designed
> to allow use without rports. However, there are several places where
> we assume rports are present: The times out and the port block
> interface ... I'm betting all current users are rport otherwise we
> would have spotted this problem sooner.
>
> > > In any case, returning BLK_EH_RESET_TIMER will cause all sorts of
> > > problems because it resets the timer to fire again for the device.
> > > What you want is something to return BLK_EH_HANDLED which will
> > > just
> > > complete the request ... probably at a generic level, since this
> > > doesn't sound to be specific to FC.
> >
> > On Hyper-V, the host implements a variety of recovery strategies and
> > for that reason, the eh_timed_out handler for standard scsi devices
> > will effectively have infinite timeout value: storvsc_eh_timed_out()
> > just resets the timer. This is the behavior I wanted for the FC
> > devices as well.
>
> All the world isn't hyper-v. If we change something in the generic
> interface, it needs to work for everyone. To me it looks like
> fc_timed_out is designed to support the port block function. If we
> assume port block is not supported for non-rport devices, then
> fc_timed_out should be returning BLK_EH_NOT_HANDLED for the non-rport
> case.
You are right and I was not implying that either. If it is ok with you, I can submit a
patch where the change will be in the storvsc driver - I will establish the same timeout
function for both normal scsi and FC devices.
Regards,
K. Y
>
> James
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-01-08 21:35 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-08 0:40 [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error handling function K. Y. Srinivasan
2016-01-07 23:48 ` James Bottomley
2016-01-08 18:58 ` KY Srinivasan
2016-01-08 19:20 ` James Bottomley
2016-01-08 20:12 ` KY Srinivasan
2016-01-08 20:26 ` James Bottomley
2016-01-08 21:35 ` KY Srinivasan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).