Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Damien Le Moal <dlemoal@kernel.org>]

On 1/8/24 22:27, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> writes:
> 
>> 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
> 
> There is no way to answer CHECK POWER MODE and say the drive is in
> sleep.  It can only be either active, or standby, so standby is the
> closest fit.  At least it gets smartd and udisks2 to leave the drive
> alone.

I was not commenting about what to do about your problem, but about the fact
that your sentence was very hard to understand because it was not technically
accurate.

>> 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.
> 
> I'm not sure what you propose be done instead.  Regardless, this is how
> it has always been done, so I don't think there is any changing it now.

I never proposed to change that in any way. That is fine and can stay as it is.
What I do NOT want to do is expand upon this to try to solve issues. The reason
for that, which I already stated, is that hdparm issue passthrough commands. And
if the user wants to use passthrough commands, then most of the time, he/she
will have to deal with the consequences of not using kernel-provided management
methods.

I did propose to allow for runtime suspend to to use sleep state instead of
standby. That would be fairly easy to do and replace manual "hdparm -Y" with a
well integrated control of the disk power state up to the block layer.
You never commented back about this.

> You also have the legacy standby timer that is exposed to users through
> udisks2/gnome-disk-utility that still has to be supported.

What is this legacy standby timer ? What control path does it trigger ? Do
udisks2/gnome-disk-utility use that timer to issue commands like "hdparm -Y"  ?
Or does that timer tigh into the regular runtime suspend ?

>> So I would rather see this handled correctly, through the kernel pm runtime
>> suspend/resume:
> 
> I'd eventually like to as well, but it should also work in kernels that
> aren't built with runtime pm enabled.

No. As said many times now, I am not going to do anything about the hdparm -Y
hacking. If a user want better power management features, he/she should enable
power management in their kernels.

>> 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
> 
> I'm not sure the filesystem maintainers will see it that way.  They
> generally issue barriers as part of a commit at regular intervals, and
> that gets turned into FLUSH CACHE.  Also the kernel issues one during
> system suspend, and I think that happens even if no filesystem is
> mounted.  I think systemd also issues a sync() during shutdown, which
> would wake up a sleeping disk only to shut down.

No. The scsi layer issues a FLUSH CACHE whenever a disk is removed, goes to
sleep or the system shutdown. And there is no need to do that if the disk is
already in standby. If you see that happening, then we need to fix that.

If the device is in sleep state from "hdparm -Y", then only libata knows that
the device is sleeping with the ATA_DFLAG_SLEEPING flag. That is the fundamental
problem here: pm-core, scsi and the block layer do not know that the block
device is sleeping (and so already had its write cache flushed). Your patches
are not solving this root cause issue. They are only hidding it by faking the
commands. This is a hack, wich likely will need more hacks in the future for
different cases. See my point ? I do not want to go down such route. Let's fix
things properly.

> I don't think it is up to all of these other sources to be patched to
> avoid this.  libata knows the disk is in sleep mode, so that is the
> place to handle it.

Not that simple. See above.


-- 
Damien Le Moal
Western Digital Research