From: Hannes Reinecke <hare@suse.de>
To: Laurence Oberman <loberman@redhat.com>, Christoph Hellwig <hch@lst.de>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>,
James Bottomley <james.bottomley@hansenpartnership.com>,
Bart van Assche <bvanassche@acm.org>,
linux-scsi@vger.kernel.org
Subject: Re: [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences
Date: Wed, 29 Jul 2020 08:09:41 +0200 [thread overview]
Message-ID: <cd1037d9-cbca-0e6d-69e3-f188b34b9d20@suse.de> (raw)
In-Reply-To: <da14033928b356c5691187f819f8e6101901dafd.camel@redhat.com>
On 7/28/20 9:50 PM, Laurence Oberman wrote:
> On Tue, 2020-07-28 at 14:43 -0400, Laurence Oberman wrote:
>> On Tue, 2020-07-28 at 15:48 +0200, Hannes Reinecke wrote:
>>> The SCSI midlayer does not allow state transitions from SDEV_BLOCK
>>> to SDEV_BLOCK, so calling scsi_target_block() from
>>> __rport_fast_io_fail()
>>> is wrong as the port is already blocked.
>>> Similarly we don't need to call scsi_target_unblock() afterwards as
>>> the
>>> function has already done this.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>> drivers/scsi/scsi_transport_srp.c | 12 +++++-------
>>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_transport_srp.c
>>> b/drivers/scsi/scsi_transport_srp.c
>>> index d4d1104fac99..cba1cf6a1c12 100644
>>> --- a/drivers/scsi/scsi_transport_srp.c
>>> +++ b/drivers/scsi/scsi_transport_srp.c
>>> @@ -395,6 +395,10 @@ static void srp_reconnect_work(struct
>>> work_struct *work)
>>> }
>>> }
>>>
>>> +/*
>>> + * scsi_target_block() must have been called before this function
>>> is
>>> + * called to guarantee that no .queuecommand() calls are in
>>> progress.
>>> + */
>>> static void __rport_fail_io_fast(struct srp_rport *rport)
>>> {
>>> struct Scsi_Host *shost = rport_to_shost(rport);
>>> @@ -404,11 +408,7 @@ static void __rport_fail_io_fast(struct
>>> srp_rport *rport)
>>>
>>> if (srp_rport_set_state(rport, SRP_RPORT_FAIL_FAST))
>>> return;
>>> - /*
>>> - * Call scsi_target_block() to wait for ongoing shost-
>>>> queuecommand()
>>>
>>> - * calls before invoking i->f->terminate_rport_io().
>>> - */
>>> - scsi_target_block(rport->dev.parent);
>>> +
>>> scsi_target_unblock(rport->dev.parent, SDEV_TRANSPORT_OFFLINE);
>>>
>>> /* Involve the LLD if possible to terminate all I/O on the
>>> rport. */
>>> @@ -570,8 +570,6 @@ int srp_reconnect_rport(struct srp_rport
>>> *rport)
>>> * failure timers if these had not yet been started.
>>> */
>>> __rport_fail_io_fast(rport);
>>> - scsi_target_unblock(&shost->shost_gendev,
>>> - SDEV_TRANSPORT_OFFLINE);
>>> __srp_start_tl_fail_timers(rport);
>>> } else if (rport->state != SRP_RPORT_BLOCKED) {
>>> scsi_target_unblock(&shost->shost_gendev,
>>
>> This looks OK to me but I guess its alwasy worked by just ignoring it
>> being called or IU would have seenm issues.
>> I etest that stuff pretty heavily.
>> Reviewed-by: Laurence Oberman <loberman@redhat.com>
>>
>
> Ouch, my last message saw my bad touch typing creep in. Let me try
> again.
>
> This looks OK to me but I guess its always worked by just ignoring it
> being called or I would have seen issues already.
> I test that stuff pretty heavily and regularly.
>
> Reviewed-by: Laurence Oberman <loberman@redhat.com>
>
>
Well, we've had a customer running into it.
And they do _heavy_ tests, too.
I haven't said it's a regular occurrence :-)
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
next prev parent reply other threads:[~2020-07-29 6:09 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-28 13:48 [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences Hannes Reinecke
2020-07-28 18:43 ` Laurence Oberman
2020-07-28 19:50 ` Laurence Oberman
2020-07-29 6:09 ` Hannes Reinecke [this message]
2020-07-30 3:27 ` Bart Van Assche
2020-07-31 2:14 ` Martin K. Petersen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cd1037d9-cbca-0e6d-69e3-f188b34b9d20@suse.de \
--to=hare@suse.de \
--cc=bvanassche@acm.org \
--cc=hch@lst.de \
--cc=james.bottomley@hansenpartnership.com \
--cc=linux-scsi@vger.kernel.org \
--cc=loberman@redhat.com \
--cc=martin.petersen@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).