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@xxxxxxxxxx>
>>> Cc: Christoph Hellwig <hch@xxxxxx>
>>> Cc: Hannes Reinecke <hare@xxxxxxxx>
>>> Cc: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
>>> ---
>>> 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.
>
