From: Damien Le Moal <dlemoal@kernel.org>
To: Phillip Susi <phill@thesusis.net>, linux-ide@vger.kernel.org
Cc: Sergey Shtylyov <s.shtylyov@omp.ru>
Subject: Re: [PATCH 1/3] libata: avoid waking disk for several commands
Date: Mon, 8 Jan 2024 15:25:51 +0900 [thread overview]
Message-ID: <f6110204-338d-42b5-8ec2-153dd862e799@kernel.org> (raw)
In-Reply-To: <20240107180258.360886-2-phill@thesusis.net>
On 1/8/24 03:02, Phillip Susi wrote:
> When a disk is in SLEEP mode it can not respond to any
> any commands. Several commands are simply a NOOP to a disk
> that is in standby mode, but when a disk is in SLEEP mode,
> they frequencly cause the disk to spin up for no reason.
> To avoid this, complete these commands in libata without
> waking the disk. These commands are:
As commented in patch 3/3, please use full 72-char lines for commit messages.
>
> CHECK POWER MODE
> FLUSH CACHE
> SLEEP
> STANDBY IMMEDIATE
> IDENTIFY
>
> If we know the disk is sleeping, we don't need to wake it up
> to find out if it is in standby, so just pretend it is in
sleep and standby are different power states. So saying that a disk that is
sleeping is in standby does not make sense. And if you wake up a drive from
sleep mode, it will *not* be in standby (need to re-check, but I think that
holds true even with PUIS enabled).
> standby. While asleep, there's no dirty pages in the cache,
> so there's no need to flush it. There's no point in waking
> a disk from sleep just to put it back to sleep. We also have
> a cache of the IDENTIFY information so just return that
> instead of waking the disk.
The problem here is that ATA_DFLAG_SLEEPING is a horrible hack to not endup with
lots of timeout failures if the user execute "hdparm -Y". Executing such
passthrough command with a disk being used by an FS (for instance) is complete
nonsense and should not be done.
So I would rather see this handled correctly, through the kernel pm runtime
suspend/resume:
1) Define a libata device sysfs attribute that allows going to sleep instead of
the default standby when the disk is runtime suspended. If sleep is used, set
ATA_DFLAG_SLEEPING.
2) With that, any command issued to the disk will trigger runtime resume. If
ATA_DFLAG_SLEEPING is set, then the drive can be woken up with a link reset from
EH, going through ata_port_runtime_resume(), exactly like with the default
standby state for suspend. ATA_DFLAG_SLEEPING being set or not will indicate if
a simple verify command can spinup the disk or if a link abort is needed (like
done now in ata_qc_issue() which is really a nasty place to do that).
Now, the annoying thing is the drive being randomly woken-up due to commands
being issued, like the ones you mention. This is indeed bad, and seeing news
like this:
https://www.phoronix.com/news/Linux-PM-Regulatory-Bugs
I think we really should do better...
But I do not think the kernel is necessarilly the right place to fix this, at
least in the case of commands issued from userspace by things like smartd or
udevd. Patching there is needed to avoid uselessly waking up disks in runtime
suspend. systemd already has power policies etc, so there is integration with
the kernel side power management. Your issues come from using a tool (hdparm)
that has no integration at all with the OS daemons.
For FSes issued commands like flush, these are generally not random at all. If
you see them appearing randomly, then there is a problem with the FS and
patching the FS may be needed. Beside flush, there are other things to consider
here. Ex: FSes using zoned block devices (SMR disks) doing garbage collection
while idle. We cannot prevent this from happening, which is why I seriously
dislike the idea of faking any command for a sleeping disk.
> ---
> drivers/ata/libata-core.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 09ed67772fae..6c5269de4bf2 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5040,6 +5040,26 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
>
> /* if device is sleeping, schedule reset and abort the link */
> if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
> + switch (qc->tf.command)
> + {
> + case ATA_CMD_CHK_POWER:
> + case ATA_CMD_SLEEP:
> + case ATA_CMD_FLUSH:
> + case ATA_CMD_FLUSH_EXT:
> + case ATA_CMD_STANDBYNOW1:
> + if (qc->tf.command == ATA_CMD_ID_ATA)
> + {
> + /* only fake the reply for IDENTIFY if it is from userspace */
> + if (ata_tag_internal(qc->tag))
> + break;
> + sg_copy_from_buffer(qc->sg, 1, qc->dev->id, 2 * ATA_ID_WORDS);
> + }
> + /* fake reply to avoid waking drive */
> + qc->flags |= ATA_QCFLAG_RTF_FILLED;
> + qc->result_tf.nsect = 0;
> + ata_qc_complete(qc);
> + return;
> + }
> link->eh_info.action |= ATA_EH_RESET;
> ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
> ata_link_abort(link);
--
Damien Le Moal
Western Digital Research
next prev parent reply other threads:[~2024-01-08 6:25 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-25 15:19 [PATCH 0/1] Only activate drive once during system resume Phillip Susi
2023-12-25 15:19 ` [PATCH 1/1] libata: only wake a drive once on " Phillip Susi
2023-12-30 18:21 ` [PATCH 0/1 v2] Only activate drive once during " Phillip Susi
2023-12-30 18:21 ` [PATCH 1/1] libata: only wake a drive once on " Phillip Susi
2023-12-30 19:42 ` Sergey Shtylyov
2024-01-02 23:17 ` Damien Le Moal
2024-01-03 21:00 ` Phillip Susi
2024-01-04 1:21 ` Damien Le Moal
2024-01-04 14:05 ` Phillip Susi
2024-01-04 22:39 ` [PATCH 1/4] " Phillip Susi
2024-01-04 22:39 ` [PATCH 2/4] libata: don't wake sleeping disk during system suspend Phillip Susi
2024-01-05 12:25 ` Damien Le Moal
2024-01-05 16:18 ` Phillip Susi
2024-01-04 22:39 ` [PATCH 3/4] libata: avoid waking disk for several commands Phillip Susi
2024-01-05 8:46 ` Sergei Shtylyov
2024-01-05 16:24 ` Phillip Susi
2024-01-05 18:33 ` Sergei Shtylyov
2024-01-06 19:49 ` Phillip Susi
2024-01-06 20:29 ` Phillip Susi
2024-01-08 8:57 ` Sergei Shtylyov
2024-01-05 12:29 ` Damien Le Moal
2024-01-05 16:30 ` Phillip Susi
2024-01-06 23:14 ` Damien Le Moal
2024-01-07 17:57 ` Phillip Susi
2024-01-07 18:02 ` [PATCH 0/3] Let sleeping disks lie Phillip Susi
2024-01-07 18:02 ` [PATCH 1/3] libata: avoid waking disk for several commands Phillip Susi
2024-01-08 6:25 ` Damien Le Moal [this message]
2024-01-08 13:27 ` Phillip Susi
2024-01-10 2:39 ` Damien Le Moal
2024-01-16 17:06 ` Phillip Susi
2024-01-19 20:43 ` Phillip Susi
2024-01-20 18:08 ` Phillip Susi
2024-01-21 0:37 ` Damien Le Moal
2024-01-21 0:37 ` Damien Le Moal
2024-01-24 16:04 ` Phillip Susi
2024-01-24 21:51 ` Damien Le Moal
2024-02-01 20:01 ` Phillip Susi
2024-02-02 1:08 ` Damien Le Moal
2024-02-02 19:53 ` Phillip Susi
2024-02-02 23:17 ` Damien Le Moal
2024-02-05 19:52 ` Phillip Susi
2024-01-08 8:48 ` Sergey Shtylyov
2024-01-08 13:30 ` Phillip Susi
2024-01-07 18:02 ` [PATCH 2/3] libata: only wake a drive once on system resume Phillip Susi
2024-01-08 6:04 ` Damien Le Moal
2024-01-07 18:02 ` [PATCH 3/3] libata: don't start PuiS disks on resume Phillip Susi
2024-01-08 6:03 ` Damien Le Moal
2024-01-08 13:39 ` Phillip Susi
2024-01-10 2:19 ` Damien Le Moal
2024-01-16 17:13 ` Phillip Susi
2024-01-04 22:39 ` [PATCH 4/4] " Phillip Susi
2024-01-05 8:57 ` Sergei Shtylyov
2024-01-05 12:42 ` Damien Le Moal
2024-01-05 16:44 ` Phillip Susi
2024-01-05 12:13 ` [PATCH 1/4] libata: only wake a drive once on system resume Damien Le Moal
2024-01-05 17:03 ` Phillip Susi
2024-01-06 23:06 ` Damien Le Moal
2024-01-05 12:44 ` Damien Le Moal
2024-01-09 15:20 ` [PATCH 0/1 v2] Only activate drive once during " Niklas Cassel
2024-01-16 17:23 ` Phillip Susi
2024-01-02 22:46 ` [PATCH 0/1] " Damien Le Moal
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=f6110204-338d-42b5-8ec2-153dd862e799@kernel.org \
--to=dlemoal@kernel.org \
--cc=linux-ide@vger.kernel.org \
--cc=phill@thesusis.net \
--cc=s.shtylyov@omp.ru \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).