| Message ID | 20240104223940.339290-3-phill@thesusis.net |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] libata: only wake a drive once on system resume | expand |
Hello! On 1/5/24 1:39 AM, Phillip Susi wrote: > When a disk is in SLEEP mode it can not respond to any > commands. Instead of waking up the sleeping disk, fake the > commands. The commands include: > > CHECK POWER This command is "officially" called CHECK POWER MODE... > FLUSH CACHE > SLEEP > STANDBY IMMEDIATE > IDENTIFY > > If we konw the disk is sleeping, we don't need to wake it up Know. > to to find out if it is in standby, so just pretend it is in Double "to". > standby. While alseep, there's no dirty pages in the cache, Asleep. > 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. > --- > drivers/ata/libata-core.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 1244da8f77e2..d9e889fa2881 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -5045,6 +5045,22 @@ 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)) { > + if (unlikely(qc->tf.command == ATA_CMD_CHK_POWER || > + qc->tf.command == ATA_CMD_SLEEP || > + qc->tf.command == ATA_CMD_FLUSH || > + qc->tf.command == ATA_CMD_FLUSH_EXT || > + qc->tf.command == ATA_CMD_STANDBYNOW1 || > + (qc->tf.command == ATA_CMD_ID_ATA && > + !ata_tag_internal(qc->tag)))) How about a *switch* instead? > + { > + /* fake reply to avoid waking drive */ > + qc->flags |= ATA_QCFLAG_RTF_FILLED; > + qc->result_tf.nsect = 0; > + if (qc->tf.command == ATA_CMD_ID_ATA) > + sg_copy_from_buffer(qc->sg, 1, qc->dev->id, 2 * ATA_ID_WORDS); > + 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); > MBR, Sergey
On 1/5/24 07:39, Phillip Susi wrote: > When a disk is in SLEEP mode it can not respond to any > commands. Instead of waking up the sleeping disk, fake the > commands. The commands include: > > CHECK POWER > FLUSH CACHE > SLEEP > STANDBY IMMEDIATE > IDENTIFY > > If we konw the disk is sleeping, we don't need to wake it up > to to find out if it is in standby, so just pretend it is in > standby. While alseep, 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. What ? If you wake up the drive, it will not be in standby... So I do not get your point here. Can you clarify ? What is the problem you are trying to solve here ? Is it related to system or runtime suspend/resume ? And no, not a chance we fake commands like this. > --- > drivers/ata/libata-core.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 1244da8f77e2..d9e889fa2881 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -5045,6 +5045,22 @@ 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)) { > + if (unlikely(qc->tf.command == ATA_CMD_CHK_POWER || > + qc->tf.command == ATA_CMD_SLEEP || > + qc->tf.command == ATA_CMD_FLUSH || > + qc->tf.command == ATA_CMD_FLUSH_EXT || > + qc->tf.command == ATA_CMD_STANDBYNOW1 || > + (qc->tf.command == ATA_CMD_ID_ATA && > + !ata_tag_internal(qc->tag)))) > + { > + /* fake reply to avoid waking drive */ > + qc->flags |= ATA_QCFLAG_RTF_FILLED; > + qc->result_tf.nsect = 0; > + if (qc->tf.command == ATA_CMD_ID_ATA) > + sg_copy_from_buffer(qc->sg, 1, qc->dev->id, 2 * ATA_ID_WORDS); > + 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);
Sergei Shtylyov <sergei.shtylyov@gmail.com> writes: > This command is "officially" called CHECK POWER MODE... Right. > Know. > Double "to". Ooops. > Asleep. Why would you capitalize that A? It isn't a proper noun, nor the first word of the sentance. > How about a *switch* instead? Ok.
Damien Le Moal <dlemoal@kernel.org> writes: > What ? If you wake up the drive, it will not be in standby... So I do not get > your point here. Can you clarify ? What is the problem you are trying to solve > here ? Is it related to system or runtime suspend/resume ? The whole point is that we don't want to spin up the drive. A drive that is in standby simply treats these commands as a NOOP. One that is in SLEEP can not do that, so we must do it for the drive. Without this patch, SLEEP mode is basically useless since the drive will be woken up by one of these commands quite soon after you put it to SLEEP. This is just to make hdparm -Y not useless. It has nothing to do with suspend/resume. I was thinking of splitting this patch series into two parts, one with just the patches related to SLEEP and one with the patches related to suspend/resume.
On 1/5/24 7:24 PM, Phillip Susi wrote: [...] >> This command is "officially" called CHECK POWER MODE... > > Right. > >> Know. >> Double "to". > > Ooops. > >> Asleep. > > Why would you capitalize that A? It isn't a proper noun, nor the first > word of the sentance. It was a start of my (single-word) sentence. I didn't mean you should capitalize it, it just had a typo. :-) >> How about a *switch* instead? > > Ok. Should be convertable into one... MBR, Sergey
Sergei Shtylyov <sergei.shtylyov@gmail.com> writes: > It was a start of my (single-word) sentence. I didn't mean you > should capitalize it, it just had a typo. :-) Got it. Applying changes now.
Sergei Shtylyov <sergei.shtylyov@gmail.com> writes:
> How about a *switch* instead?
So what's the status on switch case fall through these days? I thought
you just had to add a /* fallthrough */ comment to make it explicit, but
gcc is still complaining.
On 1/6/24 01:30, Phillip Susi wrote: > Damien Le Moal <dlemoal@kernel.org> writes: > >> What ? If you wake up the drive, it will not be in standby... So I do not get >> your point here. Can you clarify ? What is the problem you are trying to solve >> here ? Is it related to system or runtime suspend/resume ? > > The whole point is that we don't want to spin up the drive. A drive > that is in standby simply treats these commands as a NOOP. One that is > in SLEEP can not do that, so we must do it for the drive. But who is issuing these commands ? If it is systemd/udev, then *that* need to be patched to avoid it issuing these commands when the drive is sleeping. Otherwise, there is no end to this. Next time systemd/udev is modified and start issuing another command, we'll need to ignore that one as well. This is a dangerous path that I am not willing to accept. That would mean having a sysfs attribute indicating that the device is sleeping though. So the sleep case needs more work. > Without this patch, SLEEP mode is basically useless since the drive will > be woken up by one of these commands quite soon after you put it to > SLEEP.> > This is just to make hdparm -Y not useless. It has nothing to do with > suspend/resume. I was thinking of splitting this patch series into two > parts, one with just the patches related to SLEEP and one with the > patches related to suspend/resume. As long as you can only set sleep mode with hdparm, there is not much we can do. hdparm uses passthrough commands, and so handling whatever that tool does in the kernel becomes a nightmare as libata would need to parse the issued commands and handle their result. Only a few cases are done now. Extending that would be difficult and fragile. Rather, I would recommend improving the runtime pm code to allow for "going to sleep" instead of "going to standby". A sysfs attribute switch can be used for that, with the default being standby like now. And yes, please split the series ! One for what you want to do for puis and another for improved sleep handling. Everything together is simply too confusing.
Damien Le Moal <dlemoal@kernel.org> writes: > But who is issuing these commands ? If it is systemd/udev, then *that* need to > be patched to avoid it issuing these commands when the drive is sleeping. > Otherwise, there is no end to this. Next time systemd/udev is modified and start > issuing another command, we'll need to ignore that one as well. This is a > dangerous path that I am not willing to accept. I have seen both smartd and udisks2 do this when they attempt to check the SMART status of the drive. They already issue a CHECK POWER MODE command first and skip the SMART read if the drive is in standby, but even this causes a drive that is in SLEEP mode to be woken up. Also filesystems often issue FLUSH CACHE even though they have not written anything to the disk, and there is nothing in the cache. Drives in standby just treat this as a NOOP, but a drive in SLEEP can not. > That would mean having a sysfs attribute indicating that the device is sleeping > though. So the sleep case needs more work. Having a sysfs attribute that smartd/udisks2 could check before issuing CHECK POWER MODE would help that case, but not the FLUSH CACHE case. > As long as you can only set sleep mode with hdparm, there is not much we can do. > hdparm uses passthrough commands, and so handling whatever that tool does in the > kernel becomes a nightmare as libata would need to parse the issued commands and > handle their result. Only a few cases are done now. Extending that would be > difficult and fragile. It already does that. When the SLEEP command is issued, libata sets ATA_DFLAG_SLEEPING. > And yes, please split the series ! One for what you want to do for puis and > another for improved sleep handling. Everything together is simply too confusing. I realized that one of the patches was redundant anyhow, and just finished writing up a good cover letter for the remaining 3. Incoming.
On 1/6/24 11:29 PM, Phillip Susi wrote: [...] >> How about a *switch* instead? > > So what's the status on switch case fall through these days? I thought > you just had to add a /* fallthrough */ comment to make it explicit, but > gcc is still complaining. There's a fallthrough macro in <linux//compiler_attributes.h> now, I think you're suppoed to use it now... MBR, Sergey
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 1244da8f77e2..d9e889fa2881 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -5045,6 +5045,22 @@ 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)) { + if (unlikely(qc->tf.command == ATA_CMD_CHK_POWER || + qc->tf.command == ATA_CMD_SLEEP || + qc->tf.command == ATA_CMD_FLUSH || + qc->tf.command == ATA_CMD_FLUSH_EXT || + qc->tf.command == ATA_CMD_STANDBYNOW1 || + (qc->tf.command == ATA_CMD_ID_ATA && + !ata_tag_internal(qc->tag)))) + { + /* fake reply to avoid waking drive */ + qc->flags |= ATA_QCFLAG_RTF_FILLED; + qc->result_tf.nsect = 0; + if (qc->tf.command == ATA_CMD_ID_ATA) + sg_copy_from_buffer(qc->sg, 1, qc->dev->id, 2 * ATA_ID_WORDS); + 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);