Add support for ledtrig-blkdev

[gulikoza]

Hi,

ugreen-diskiomon currently uses 0.1sec bash loop to check for disk statistics and blink leds. This uses about 0,7%-1% CPU on my DXP6800Pro which is not insignificant.

I was searching for solutions and found that there is a led trigger written for this kind of usage: blkdev. It allows to link a block device to the trigger and blink on activity. Unfortunately it does not seem like it will be merged as the maintainer does not like the sysfs interface of linking/unlinking devices.

I wrote a proof of concept patch for diskiomon and tested the results. The CPU activity with ledtrig-blkdev being used falls down to 0%. Powertop shows ~1050 wakeups with default loop compared to <200 with blkdev. The system spends ~85% in C3 compared to 25% using default functionality.

What do you think? I've retained the default functionality in the script if the module is not available. But maybe we can include it as well? We're building one module, we might as well build two...

The patch is not finalized; ledtrig-blkdev does seem to support invert, so the leds turn on activity instead of turning off. I could probably add invert to the module without much trouble. I also haven't tested all the edge cases as well (since inversion is not supported, trigger should be changed on disk failure in order for the led to be on), so there is still some work to be done if this is to be included. Also haven't tested ZFS at all and I think there's a minor bug there:

"Disk failure detected on /dev/$dev"

...there is no $dev defined inside that function. I suppose this should be $zpool_dev_name which I have also used to unlink the led?

I've built the module for RHEL8 and 9 and can include the SPEC for kmod rpm as well if we will include the module itself.

[miskcoo]

It sounds good and I think it is ok to include it. I also found the ledtrig-blkdev module when writting the script and nice to see that it works.

The bottleneck at the ugreen-diskiomon script is that bash produces too many forks in the main loop.

Maybe directly replacing it with another implementation could also resolve this problem. I also added one solution in the fast-diskiomon branch and found it uses significantly less CPU times (~12% of the bash version). But I am not sure whether the ledtrig-blkdev module will be better compared to this, it would be great if you have time to test it :)

To use the solution in the fast-diskiomon branch, you need to compile the scripts/blink-disk.cpp and put it in /usr/bin/ugreen-blink-disk, and replace the ugreen-diskiomon with the new version.

[gulikoza]

Hi,

Looks fine as well, powertop shows about 280 wakeups/second and ~60% spent in C3. So slightly worse than the ledtrig-blkdev module and significantly better than the bash version.

I'd say either one would be ok, although I'd assume blink-disk.cpp would be easier to manage in the long run as it does not need to retain kernel compatibility like the module. I had to patch the ledtrig-blkdev quite a bit to make it compile for RHEL8.

[gulikoza]

ledtrig-blkdev is segfaulting on poweroff, which would mean the script would have to properly handle shutdown. It segfaulted yesterday as well when I was testing it, but I did not have HDMI connected at the time and I could not see why the NAS did not shut down properly. It works fine if stop is done and rmmod works as well so it appears it does not handle shutdown gracefully.

I suppose it could be handled either in the script or fixing the module, still I don't like it as kernel module is much harder to diagnose than a new cpp loop. Maybe check the ZFS typo I mentioned and close this PR if fast-diskiomon branch will be used.