From: Vlastimil Babka <vbabka@suse.cz>
To: Bart Van Assche <bvanassche@acm.org>,
"Martin K . Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org,
Damien Le Moal <damien.lemoal@opensource.wdc.com>,
Hannes Reinecke <hare@suse.de>,
Geert Uytterhoeven <geert@linux-m68k.org>,
gzhqyz@gmail.com, "James E.J. Bottomley" <jejb@linux.ibm.com>
Subject: Re: [PATCH] scsi: sd: Revert "Rework asynchronous resume support"
Date: Wed, 17 Aug 2022 22:06:35 +0200 [thread overview]
Message-ID: <06afc774-063d-1aba-f75c-9a4c80e2f836@suse.cz> (raw)
In-Reply-To: <20220816172638.538734-1-bvanassche@acm.org>
On 8/16/22 19:26, Bart Van Assche wrote:
> Although patch "Rework asynchronous resume support" eliminates the delay
> for some ATA disks after resume, it causes resume of ATA disks to fail
> on other setups. See also:
> * "Resume process hangs for 5-6 seconds starting sometime in 5.16"
> (https://bugzilla.kernel.org/show_bug.cgi?id=215880).
> * Geert's regression report
> (https://lore.kernel.org/linux-scsi/alpine.DEB.2.22.394.2207191125130.1006766@ramsan.of.borg/).
>
> This is what I understand about this issue:
> * During resume, ata_port_pm_resume() starts the SCSI error handler.
> This changes the SCSI host state into SHOST_RECOVERY and causes
> scsi_queue_rq() to return BLK_STS_RESOURCE.
> * sd_resume() calls sd_start_stop_device() for ATA devices. That
> function in turn calls sd_submit_start() which tries to submit a START
> STOP UNIT command. That command can only be submitted after the SCSI
> error handler has changed the SCSI host state back to SHOST_RUNNING.
> * The SCSI error handler runs on its own thread and calls
> schedule_work(&(ap->scsi_rescan_task)). That causes
> ata_scsi_dev_rescan() to be called from the context of a kernel
> workqueue. That call hangs in blk_mq_get_tag(). I'm not sure why -
> maybe because all available tags have been allocated by
> sd_submit_start() calls (this is a guess).
>
> Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: gzhqyz@gmail.com
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Reported-by: gzhqyz@gmail.com
> Fixes: 88f1669019bd ("scsi: sd: Rework asynchronous resume support"; v6.0-rc1~114^2~68)
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reported-and-tested-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> drivers/scsi/sd.c | 84 ++++++++++-------------------------------------
> drivers/scsi/sd.h | 5 ---
> 2 files changed, 18 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 8f79fa6318fe..eb76ba055021 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -103,7 +103,6 @@ static void sd_config_discard(struct scsi_disk *, unsigned int);
> static void sd_config_write_same(struct scsi_disk *);
> static int sd_revalidate_disk(struct gendisk *);
> static void sd_unlock_native_capacity(struct gendisk *disk);
> -static void sd_start_done_work(struct work_struct *work);
> static int sd_probe(struct device *);
> static int sd_remove(struct device *);
> static void sd_shutdown(struct device *);
> @@ -3471,7 +3470,6 @@ static int sd_probe(struct device *dev)
> sdkp->max_retries = SD_MAX_RETRIES;
> atomic_set(&sdkp->openers, 0);
> atomic_set(&sdkp->device->ioerr_cnt, 0);
> - INIT_WORK(&sdkp->start_done_work, sd_start_done_work);
>
> if (!sdp->request_queue->rq_timeout) {
> if (sdp->type != TYPE_MOD)
> @@ -3594,69 +3592,12 @@ static void scsi_disk_release(struct device *dev)
> kfree(sdkp);
> }
>
> -/* Process sense data after a START command finished. */
> -static void sd_start_done_work(struct work_struct *work)
> -{
> - struct scsi_disk *sdkp = container_of(work, typeof(*sdkp),
> - start_done_work);
> - struct scsi_sense_hdr sshdr;
> - int res = sdkp->start_result;
> -
> - if (res == 0)
> - return;
> -
> - sd_print_result(sdkp, "Start/Stop Unit failed", res);
> -
> - if (res < 0)
> - return;
> -
> - if (scsi_normalize_sense(sdkp->start_sense_buffer,
> - sdkp->start_sense_len, &sshdr))
> - sd_print_sense_hdr(sdkp, &sshdr);
> -}
> -
> -/* A START command finished. May be called from interrupt context. */
> -static void sd_start_done(struct request *req, blk_status_t status)
> -{
> - const struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(req);
> - struct scsi_disk *sdkp = scsi_disk(req->q->disk);
> -
> - sdkp->start_result = scmd->result;
> - WARN_ON_ONCE(scmd->sense_len > SCSI_SENSE_BUFFERSIZE);
> - sdkp->start_sense_len = scmd->sense_len;
> - memcpy(sdkp->start_sense_buffer, scmd->sense_buffer,
> - ARRAY_SIZE(sdkp->start_sense_buffer));
> - WARN_ON_ONCE(!schedule_work(&sdkp->start_done_work));
> -}
> -
> -/* Submit a START command asynchronously. */
> -static int sd_submit_start(struct scsi_disk *sdkp, u8 cmd[], u8 cmd_len)
> -{
> - struct scsi_device *sdev = sdkp->device;
> - struct request_queue *q = sdev->request_queue;
> - struct request *req;
> - struct scsi_cmnd *scmd;
> -
> - req = scsi_alloc_request(q, REQ_OP_DRV_IN, BLK_MQ_REQ_PM);
> - if (IS_ERR(req))
> - return PTR_ERR(req);
> -
> - scmd = blk_mq_rq_to_pdu(req);
> - scmd->cmd_len = cmd_len;
> - memcpy(scmd->cmnd, cmd, cmd_len);
> - scmd->allowed = sdkp->max_retries;
> - req->timeout = SD_TIMEOUT;
> - req->rq_flags |= RQF_PM | RQF_QUIET;
> - req->end_io = sd_start_done;
> - blk_execute_rq_nowait(req, /*at_head=*/true);
> -
> - return 0;
> -}
> -
> static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
> {
> unsigned char cmd[6] = { START_STOP }; /* START_VALID */
> + struct scsi_sense_hdr sshdr;
> struct scsi_device *sdp = sdkp->device;
> + int res;
>
> if (start)
> cmd[4] |= 1; /* START */
> @@ -3667,10 +3608,23 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
> if (!scsi_device_online(sdp))
> return -ENODEV;
>
> - /* Wait until processing of sense data has finished. */
> - flush_work(&sdkp->start_done_work);
> + res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
> + SD_TIMEOUT, sdkp->max_retries, 0, RQF_PM, NULL);
> + if (res) {
> + sd_print_result(sdkp, "Start/Stop Unit failed", res);
> + if (res > 0 && scsi_sense_valid(&sshdr)) {
> + sd_print_sense_hdr(sdkp, &sshdr);
> + /* 0x3a is medium not present */
> + if (sshdr.asc == 0x3a)
> + res = 0;
> + }
> + }
>
> - return sd_submit_start(sdkp, cmd, sizeof(cmd));
> + /* SCSI error codes must not go to the generic layer */
> + if (res)
> + return -EIO;
> +
> + return 0;
> }
>
> /*
> @@ -3697,8 +3651,6 @@ static void sd_shutdown(struct device *dev)
> sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
> sd_start_stop_device(sdkp, 0);
> }
> -
> - flush_work(&sdkp->start_done_work);
> }
>
> static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index b89187761d61..5eea762f84d1 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -150,11 +150,6 @@ struct scsi_disk {
> unsigned urswrz : 1;
> unsigned security : 1;
> unsigned ignore_medium_access_errors : 1;
> -
> - int start_result;
> - u32 start_sense_len;
> - u8 start_sense_buffer[SCSI_SENSE_BUFFERSIZE];
> - struct work_struct start_done_work;
> };
> #define to_scsi_disk(obj) container_of(obj, struct scsi_disk, disk_dev)
>
next prev parent reply other threads:[~2022-08-17 20:08 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-16 17:26 [PATCH] scsi: sd: Revert "Rework asynchronous resume support" Bart Van Assche
2022-08-16 18:00 ` John Garry
2022-08-16 18:06 ` Bart Van Assche
2022-08-17 8:20 ` John Garry
2022-08-17 20:06 ` Vlastimil Babka [this message]
2022-08-22 20:53 ` Vlastimil Babka
2022-08-20 15:37 ` Hans de Goede
2022-08-21 9:16 ` Geert Uytterhoeven
2022-08-22 2:52 ` Bart Van Assche
2022-08-23 6:41 ` Geert Uytterhoeven
2022-08-23 18:10 ` Bart Van Assche
2022-08-26 7:54 ` Geert Uytterhoeven
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=06afc774-063d-1aba-f75c-9a4c80e2f836@suse.cz \
--to=vbabka@suse.cz \
--cc=bvanassche@acm.org \
--cc=damien.lemoal@opensource.wdc.com \
--cc=geert@linux-m68k.org \
--cc=gzhqyz@gmail.com \
--cc=hare@suse.de \
--cc=jejb@linux.ibm.com \
--cc=linux-scsi@vger.kernel.org \
--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).