| Message ID | 20190402195815.254796-4-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SCSI target patches for kernel v5.2 | expand |
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.
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.
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. >
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.
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.
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.
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); }
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(+)