Re: [PATCH 03/11] target/core: Release SPC-2 reservation upon initiator logout

[Mike Christie <mchristi@redhat.com>]

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.
>