[03/11] target/core: Release SPC-2 reservation upon initiator logout
diff mbox series

Message ID 20190402195815.254796-4-bvanassche@acm.org
State New
Headers show
Series
  • SCSI target patches for kernel v5.2
Related show

Commit Message

Bart Van Assche April 2, 2019, 7:58 p.m. UTC
While testing with the libiscsi tool I noticed after the tool had stopped
and hence after it had logged out that an SPC-2 reservation was still active:

$ (cd /sys/kernel/config/target/core && find -name res_holder|xargs grep -aH .)
./pscsi_0/vdev3/pr/res_holder:Passthrough
./iblock_0/vdev2/pr/res_holder:No SPC-3 Reservation holder
./fileio_1/vdev1/pr/res_holder:SPC-2 Reservation: iSCSI Initiator: iqn.2007-10.com.github:sahlberg:libiscsi:iscsi-test-2
./fileio_0/vdev0/pr/res_holder:No SPC-3 Reservation holder

This is a bug. SPC-2 reservations must be cleared when an initiator
logs out. This patch fixes that bug. A quote from SPC-2 illustrates this:
"Reservations managed using the reserve/release method do not persist
across some recovery actions (e.g., hard resets). When a target performs
one of these recovery actions, the application client(s) have to rediscover
the configuration and re-establish the required reservations."

Cc: Mike Christie <mchristi@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/target/target_core_transport.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Mike Christie April 8, 2019, 8:04 p.m. UTC | #1
On 04/02/2019 02:58 PM, Bart Van Assche wrote:
> While testing with the libiscsi tool I noticed after the tool had stopped
> and hence after it had logged out that an SPC-2 reservation was still active:
> 
> $ (cd /sys/kernel/config/target/core && find -name res_holder|xargs grep -aH .)
> ./pscsi_0/vdev3/pr/res_holder:Passthrough
> ./iblock_0/vdev2/pr/res_holder:No SPC-3 Reservation holder
> ./fileio_1/vdev1/pr/res_holder:SPC-2 Reservation: iSCSI Initiator: iqn.2007-10.com.github:sahlberg:libiscsi:iscsi-test-2
> ./fileio_0/vdev0/pr/res_holder:No SPC-3 Reservation holder
> 
> This is a bug. SPC-2 reservations must be cleared when an initiator
> logs out. This patch fixes that bug. A quote from SPC-2 illustrates this:
> "Reservations managed using the reserve/release method do not persist
> across some recovery actions (e.g., hard resets). When a target performs
> one of these recovery actions, the application client(s) have to rediscover
> the configuration and re-establish the required reservations."
> 
> Cc: Mike Christie <mchristi@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/target/target_core_transport.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index e3f7e21e6614..93ef5c6362d6 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -550,6 +550,15 @@ void transport_deregister_session_configfs(struct se_session *se_sess)
>  }
>  EXPORT_SYMBOL(transport_deregister_session_configfs);
>  
> +static int target_sess_release_reservation(struct se_device *dev, void *data)
> +{
> +	struct se_session *sess = data;
> +
> +	if (dev->reservation_holder == sess)
> +		target_release_reservation(dev);
> +	return 0;
> +}
> +
>  void transport_free_session(struct se_session *se_sess)
>  {
>  	struct se_node_acl *se_nacl = se_sess->se_node_acl;
> @@ -592,6 +601,7 @@ void transport_free_session(struct se_session *se_sess)
>  		sbitmap_queue_free(&se_sess->sess_tag_pool);
>  		kvfree(se_sess->sess_cmd_map);
>  	}
> +	target_for_each_device(target_sess_release_reservation, se_sess);
>  	percpu_ref_exit(&se_sess->cmd_count);
>  	kmem_cache_free(se_sess_cache, se_sess);
>  }
> 

Was the original code done for iscsi? We have this in
https://tools.ietf.org/html/rfc7143:

4.4.3.2. Reservations

....

In contrast, [SPC2] does not specify detailed persistence
requirements for reserve/release reservation state after an I_T nexus
failure. Nonetheless, when reserve/release reservations are
supported by an iSCSI target, the preferred implementation approach
is to preserve reserve/release reservation state for iSCSI session
reinstatement (see Section 6.3.5) or session continuation (see
Section 6.3.6).


So for example for session reinstatement if you pull a network cable
then plug it back in the iscsi target can do a
iscsit_close_session->transport_deregister_session ->
transport_free_session. Above we will now release the reservation.
Bart Van Assche April 8, 2019, 8:17 p.m. UTC | #2
On Mon, 2019-04-08 at 15:04 -0500, Mike Christie wrote:
> On 04/02/2019 02:58 PM, Bart Van Assche wrote:
> > While testing with the libiscsi tool I noticed after the tool had stopped
> > and hence after it had logged out that an SPC-2 reservation was still active:
> > 
> > $ (cd /sys/kernel/config/target/core && find -name res_holder|xargs grep -aH .)
> > ./pscsi_0/vdev3/pr/res_holder:Passthrough
> > ./iblock_0/vdev2/pr/res_holder:No SPC-3 Reservation holder
> > ./fileio_1/vdev1/pr/res_holder:SPC-2 Reservation: iSCSI Initiator: iqn.2007-10.com.github:sahlberg:libiscsi:iscsi-test-2
> > ./fileio_0/vdev0/pr/res_holder:No SPC-3 Reservation holder
> > 
> > This is a bug. SPC-2 reservations must be cleared when an initiator
> > logs out. This patch fixes that bug. A quote from SPC-2 illustrates this:
> > "Reservations managed using the reserve/release method do not persist
> > across some recovery actions (e.g., hard resets). When a target performs
> > one of these recovery actions, the application client(s) have to rediscover
> > the configuration and re-establish the required reservations."
> > 
> > Cc: Mike Christie <mchristi@redhat.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Hannes Reinecke <hare@suse.com>
> > Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > ---
> >  drivers/target/target_core_transport.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index e3f7e21e6614..93ef5c6362d6 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -550,6 +550,15 @@ void transport_deregister_session_configfs(struct se_session *se_sess)
> >  }
> >  EXPORT_SYMBOL(transport_deregister_session_configfs);
> >  
> > +static int target_sess_release_reservation(struct se_device *dev, void *data)
> > +{
> > +	struct se_session *sess = data;
> > +
> > +	if (dev->reservation_holder == sess)
> > +		target_release_reservation(dev);
> > +	return 0;
> > +}
> > +
> >  void transport_free_session(struct se_session *se_sess)
> >  {
> >  	struct se_node_acl *se_nacl = se_sess->se_node_acl;
> > @@ -592,6 +601,7 @@ void transport_free_session(struct se_session *se_sess)
> >  		sbitmap_queue_free(&se_sess->sess_tag_pool);
> >  		kvfree(se_sess->sess_cmd_map);
> >  	}
> > +	target_for_each_device(target_sess_release_reservation, se_sess);
> >  	percpu_ref_exit(&se_sess->cmd_count);
> >  	kmem_cache_free(se_sess_cache, se_sess);
> >  }
> > 
> 
> Was the original code done for iscsi? We have this in
> https://tools.ietf.org/html/rfc7143:
> 
> 4.4.3.2. Reservations
> 
> ....
> 
> In contrast, [SPC2] does not specify detailed persistence
> requirements for reserve/release reservation state after an I_T nexus
> failure. Nonetheless, when reserve/release reservations are
> supported by an iSCSI target, the preferred implementation approach
> is to preserve reserve/release reservation state for iSCSI session
> reinstatement (see Section 6.3.5) or session continuation (see
> Section 6.3.6).
> 
> 
> So for example for session reinstatement if you pull a network cable
> then plug it back in the iscsi target can do a
> iscsit_close_session->transport_deregister_session ->
> transport_free_session. Above we will now release the reservation.

Hello Mike,

Was that paragraph from the iSCSI RFC perhaps written before it was made clear
in SPC-2 that reserve/release reservations do not persist? I think the name of
the successor concept, "persistent reservations", makes it clear that the
majority of the implementations of the old reserve/release mechanism did not
persist these reservations across sessions.

Bart.
Mike Christie April 8, 2019, 10:41 p.m. UTC | #3
On 04/08/2019 03:17 PM, Bart Van Assche wrote:
> On Mon, 2019-04-08 at 15:04 -0500, Mike Christie wrote:
>> On 04/02/2019 02:58 PM, Bart Van Assche wrote:
>>> While testing with the libiscsi tool I noticed after the tool had stopped
>>> and hence after it had logged out that an SPC-2 reservation was still active:
>>>
>>> $ (cd /sys/kernel/config/target/core && find -name res_holder|xargs grep -aH .)
>>> ./pscsi_0/vdev3/pr/res_holder:Passthrough
>>> ./iblock_0/vdev2/pr/res_holder:No SPC-3 Reservation holder
>>> ./fileio_1/vdev1/pr/res_holder:SPC-2 Reservation: iSCSI Initiator: iqn.2007-10.com.github:sahlberg:libiscsi:iscsi-test-2
>>> ./fileio_0/vdev0/pr/res_holder:No SPC-3 Reservation holder
>>>
>>> This is a bug. SPC-2 reservations must be cleared when an initiator
>>> logs out. This patch fixes that bug. A quote from SPC-2 illustrates this:
>>> "Reservations managed using the reserve/release method do not persist
>>> across some recovery actions (e.g., hard resets). When a target performs
>>> one of these recovery actions, the application client(s) have to rediscover
>>> the configuration and re-establish the required reservations."
>>>
>>> Cc: Mike Christie <mchristi@redhat.com>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Cc: Hannes Reinecke <hare@suse.com>
>>> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>>  drivers/target/target_core_transport.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
>>> index e3f7e21e6614..93ef5c6362d6 100644
>>> --- a/drivers/target/target_core_transport.c
>>> +++ b/drivers/target/target_core_transport.c
>>> @@ -550,6 +550,15 @@ void transport_deregister_session_configfs(struct se_session *se_sess)
>>>  }
>>>  EXPORT_SYMBOL(transport_deregister_session_configfs);
>>>  
>>> +static int target_sess_release_reservation(struct se_device *dev, void *data)
>>> +{
>>> +	struct se_session *sess = data;
>>> +
>>> +	if (dev->reservation_holder == sess)
>>> +		target_release_reservation(dev);
>>> +	return 0;
>>> +}
>>> +
>>>  void transport_free_session(struct se_session *se_sess)
>>>  {
>>>  	struct se_node_acl *se_nacl = se_sess->se_node_acl;
>>> @@ -592,6 +601,7 @@ void transport_free_session(struct se_session *se_sess)
>>>  		sbitmap_queue_free(&se_sess->sess_tag_pool);
>>>  		kvfree(se_sess->sess_cmd_map);
>>>  	}
>>> +	target_for_each_device(target_sess_release_reservation, se_sess);
>>>  	percpu_ref_exit(&se_sess->cmd_count);
>>>  	kmem_cache_free(se_sess_cache, se_sess);
>>>  }
>>>
>>
>> Was the original code done for iscsi? We have this in
>> https://tools.ietf.org/html/rfc7143:
>>
>> 4.4.3.2. Reservations
>>
>> ....
>>
>> In contrast, [SPC2] does not specify detailed persistence
>> requirements for reserve/release reservation state after an I_T nexus
>> failure. Nonetheless, when reserve/release reservations are
>> supported by an iSCSI target, the preferred implementation approach
>> is to preserve reserve/release reservation state for iSCSI session
>> reinstatement (see Section 6.3.5) or session continuation (see
>> Section 6.3.6).
>>
>>
>> So for example for session reinstatement if you pull a network cable
>> then plug it back in the iscsi target can do a
>> iscsit_close_session->transport_deregister_session ->
>> transport_free_session. Above we will now release the reservation.
> 
> Hello Mike,
> 
> Was that paragraph from the iSCSI RFC perhaps written before it was made clear
> in SPC-2 that reserve/release reservations do not persist? I think the name of

Do you know when that was added to SPC 2?

The iscsi text above is from RFC 7143 which I think was ratified in
2014. I think it was added in that version to clarify the issue. I am
not sure though.

The original 3720 ratified in 2004 did not have that chunk and only had
limited references/info on reservations.


> the successor concept, "persistent reservations", makes it clear that the
> majority of the implementations of the old reserve/release mechanism did not
> persist these reservations across sessions.
> 
> Bart.
>
Bart Van Assche April 8, 2019, 11:17 p.m. UTC | #4
On Mon, 2019-04-08 at 17:41 -0500, Mike Christie wrote:
> On 04/08/2019 03:17 PM, Bart Van Assche wrote:
> > On Mon, 2019-04-08 at 15:04 -0500, Mike Christie wrote:
> > > Was the original code done for iscsi? We have this in
> > > https://tools.ietf.org/html/rfc7143:
> > > 
> > > 4.4.3.2. Reservations
> > > 
> > > ....
> > > 
> > > In contrast, [SPC2] does not specify detailed persistence
> > > requirements for reserve/release reservation state after an I_T nexus
> > > failure. Nonetheless, when reserve/release reservations are
> > > supported by an iSCSI target, the preferred implementation approach
> > > is to preserve reserve/release reservation state for iSCSI session
> > > reinstatement (see Section 6.3.5) or session continuation (see
> > > Section 6.3.6).
> > > 
> > > 
> > > So for example for session reinstatement if you pull a network cable
> > > then plug it back in the iscsi target can do a
> > > iscsit_close_session->transport_deregister_session ->
> > > transport_free_session. Above we will now release the reservation.
> > 
> > Hello Mike,
> > 
> > Was that paragraph from the iSCSI RFC perhaps written before it was made clear
> > in SPC-2 that reserve/release reservations do not persist? I think the name of
> 
> Do you know when that was added to SPC 2?
> 
> The iscsi text above is from RFC 7143 which I think was ratified in
> 2014. I think it was added in that version to clarify the issue. I am
> not sure though.
> 
> The original 3720 ratified in 2004 did not have that chunk and only had
> limited references/info on reservations.

Hello Mike,

From spc2r01 (1997-11-13): "Reservations managed using the Reserve/Release
method do not persist across some recovery actions (e.g., hard resets), so
most systems require significant reinitialization after a failure that
results in a hard reset. Reserve/Release managed reservations are retained
by the device server until released or until reset by mechanisms specified
in this standard."

Although the words differ from later versions of SPC2, the intention has not
changed.

Other vendors seem to agree with what has been defined in SPC2. From
https://kb.netapp.com/app/answers/answer_view/a_id/1001463/~/what-are-scsi-reservations-and-scsi-persistent-reservations%3F-:
"Thus, a SCSI bus reset performed due to an error recovery would cause the
reservation to be released."

Does this mean that the authors of the iSCSI RFC chose behavior that
contradicts established behavior for the SCSI reserve and release commands?

Thanks,

Bart.
Mike Christie April 9, 2019, 5:47 a.m. UTC | #5
On 04/08/2019 06:17 PM, Bart Van Assche wrote:
> On Mon, 2019-04-08 at 17:41 -0500, Mike Christie wrote:
>> On 04/08/2019 03:17 PM, Bart Van Assche wrote:
>>> On Mon, 2019-04-08 at 15:04 -0500, Mike Christie wrote:
>>>> Was the original code done for iscsi? We have this in
>>>> https://tools.ietf.org/html/rfc7143:
>>>>
>>>> 4.4.3.2. Reservations
>>>>
>>>> ....
>>>>
>>>> In contrast, [SPC2] does not specify detailed persistence
>>>> requirements for reserve/release reservation state after an I_T nexus
>>>> failure. Nonetheless, when reserve/release reservations are
>>>> supported by an iSCSI target, the preferred implementation approach
>>>> is to preserve reserve/release reservation state for iSCSI session
>>>> reinstatement (see Section 6.3.5) or session continuation (see
>>>> Section 6.3.6).
>>>>
>>>>
>>>> So for example for session reinstatement if you pull a network cable
>>>> then plug it back in the iscsi target can do a
>>>> iscsit_close_session->transport_deregister_session ->
>>>> transport_free_session. Above we will now release the reservation.
>>>
>>> Hello Mike,
>>>
>>> Was that paragraph from the iSCSI RFC perhaps written before it was made clear
>>> in SPC-2 that reserve/release reservations do not persist? I think the name of
>>
>> Do you know when that was added to SPC 2?
>>
>> The iscsi text above is from RFC 7143 which I think was ratified in
>> 2014. I think it was added in that version to clarify the issue. I am
>> not sure though.
>>
>> The original 3720 ratified in 2004 did not have that chunk and only had
>> limited references/info on reservations.
> 
> Hello Mike,
> 
> From spc2r01 (1997-11-13): "Reservations managed using the Reserve/Release
> method do not persist across some recovery actions (e.g., hard resets), so
> most systems require significant reinitialization after a failure that
> results in a hard reset. Reserve/Release managed reservations are retained
> by the device server until released or until reset by mechanisms specified
> in this standard."
> 
> Although the words differ from later versions of SPC2, the intention has not
> changed.
> 
> Other vendors seem to agree with what has been defined in SPC2. From
> https://kb.netapp.com/app/answers/answer_view/a_id/1001463/~/what-are-scsi-reservations-and-scsi-persistent-reservations%3F-:
> "Thus, a SCSI bus reset performed due to an error recovery would cause the
> reservation to be released."
> 
> Does this mean that the authors of the iSCSI RFC chose behavior that
> contradicts established behavior for the SCSI reserve and release commands?
> 

I don't know.

For the hard reset/reset event case, if we go by SAM 2 which iscsi RFC
7143 and 3720 say they are based on at the beginning of their
definitions then it sounds like it depends on the transport spec for
what is a hard reset. We have this from SAM 2:

5.9.6 Hard reset:

A hard reset is a SCSI target port action in response to a reset event
within the service delivery subsystem. A wakeup event (see 3.1.132) is a
reset event. The definition of additional reset events is SCSI transport
protocol specific. Each SCSI transport protocol standard that defines
reset events shall specify the SCSI target port’s action
in response to reset events.

--------

That might sound like a wake event is a hard reset defined by SAM 2, and
the transport defines others. In the iscsi RFC the only mention of reset
events is when handling the iSCSI TARGET COLD RESET TMF. However, the
RFC does define the session reinstatement case as a I_T nexus loss
event, so if that is considered a hard reset by the SAM 2 definition
then I think your patch is correct. I have no idea if other software
writers read the specs/rfcs like maybe Nick did though.
Bart Van Assche April 9, 2019, 5:25 p.m. UTC | #6
On Tue, 2019-04-09 at 00:47 -0500, Mike Christie wrote:
> I don't know.
> 
> For the hard reset/reset event case, if we go by SAM 2 which iscsi RFC
> 7143 and 3720 say they are based on at the beginning of their
> definitions then it sounds like it depends on the transport spec for
> what is a hard reset. We have this from SAM 2:
> 
> 5.9.6 Hard reset:
> 
> A hard reset is a SCSI target port action in response to a reset event
> within the service delivery subsystem. A wakeup event (see 3.1.132) is a
> reset event. The definition of additional reset events is SCSI transport
> protocol specific. Each SCSI transport protocol standard that defines
> reset events shall specify the SCSI target port’s action
> in response to reset events.
> 
> --------
> 
> That might sound like a wake event is a hard reset defined by SAM 2, and
> the transport defines others. In the iscsi RFC the only mention of reset
> events is when handling the iSCSI TARGET COLD RESET TMF. However, the
> RFC does define the session reinstatement case as a I_T nexus loss
> event, so if that is considered a hard reset by the SAM 2 definition
> then I think your patch is correct. I have no idea if other software
> writers read the specs/rfcs like maybe Nick did though.

Hi Mike,

I'm fine with leaving out patch 3/11. But even if this patch is left out
that still leaves the question open what the behavior of RESERVE/RELEASE
should be ...

Bart.

Patch
diff mbox series

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index e3f7e21e6614..93ef5c6362d6 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -550,6 +550,15 @@  void transport_deregister_session_configfs(struct se_session *se_sess)
 }
 EXPORT_SYMBOL(transport_deregister_session_configfs);
 
+static int target_sess_release_reservation(struct se_device *dev, void *data)
+{
+	struct se_session *sess = data;
+
+	if (dev->reservation_holder == sess)
+		target_release_reservation(dev);
+	return 0;
+}
+
 void transport_free_session(struct se_session *se_sess)
 {
 	struct se_node_acl *se_nacl = se_sess->se_node_acl;
@@ -592,6 +601,7 @@  void transport_free_session(struct se_session *se_sess)
 		sbitmap_queue_free(&se_sess->sess_tag_pool);
 		kvfree(se_sess->sess_cmd_map);
 	}
+	target_for_each_device(target_sess_release_reservation, se_sess);
 	percpu_ref_exit(&se_sess->cmd_count);
 	kmem_cache_free(se_sess_cache, se_sess);
 }