• Status Closed
  • Percent Complete
  • Task Type Bugs
  • Category Music playback
  • Assigned To No-one
  • Operating System SW-codec
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by dreamlayers - 2009-01-10
Last edited by speachy - 2024-04-22

FS#9775 - Premature and multiple storage_sleep() in buffering.c

While examining disk performance with  FS#9708 , I found one album where the disk stayed spinning with no activity for 5 s longer when using PIO. This is repeatable with that particular album. With the downloaded r19739-090109 build, this did not happen with the default settings but it started happening as soon as I enabled dircache. Prolonging the disk timeout prolongs the extra sleeping time.

Experimentation with shows that the storage_sleep() part of fill_buffer() (in apps/buffering.c) is executed early in the buffering and it is not executed at the end. With another album where spindown happens properly, ata_sleep() is called twice. Note that ata_sleep() just sets last_disk_activity, and sleep happens over half a second later if there is no disk activity during that period and last_user_activity has timed out.

I think this is a race condition based on how dircache and transfer rates change things. I wonder if it is related to  FS#9541 .

The data mentioned is located at and the delayed spindown is seen in the EP7 PIO test. In that data, also note how sectors written are 5, 10 or 15, with differences between different tests with the same album.

I have a 30GB 5th generation iPod, but I expect this issue affects all SW-codec players.

Closed by  speachy
2024-04-22 21:22
Reason for closing:  Out of Date
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

The buffering and storage sleep code have seen many changes in the past ..15 years. It is highly unlikely to be relevant still

The failure to storage_sleep() at the end happens because buffer filling is completed via buf_request_buffer_handle() and Q_SLEEP, without involving fill_buffer().

Sorry about the typo earlier. I meant increasing disk timeout prolongs extra spinning time, not sleeping time. I need to proofread more before I submit.

*sigh* I mean: The failure to call storage_sleep() at the end happens because buffer filling is completed via buf_request_buffer_handle() and Q_START_FILL, without involving fill_buffer().

It seems multiple calls to storage_sleep() routinely happen while loading albums with multiple tracks. They happen after various queue events in buffering_thread(). As mentioned earlier, in ata.c they don't actually cause sleep as long as other ATA activity happens soon. They do result in call_storage_idle_notifys(false) calls, but because of that false they shouldn't result in more than one execution of the idle notifys. I see two executions, I guess as a result of the call_storage_idle_notifys(true) after ACTION_WPS_PLAY.

buffer_handle() only returns false when there isn't enough space left for one more BUFFERING_DEFAULT_FILECHUNK. It returns true in all other cases, including if a file has been buffered up to its end or if an event arrived on the buffering_queue and interrupted buffering. In fill_buffer(), if buffer_handle() returns true, the loop moves on to the next handle. Now, consider what happens when buffer_handle() for the last handle is interrupted by a buffering_queue event. The next pointer is NULL, so m becomes NULL, and the test after the loop causes storage_sleep() and returns false, which sets filling in buffering_thread() to zero.

Here is a simple patch for the issue described in my previous comment. It resolves the delayed spindown problem mentioned earlier. It does not cause storage_sleep() to be called less while loading other albums however, and spindown might still be delayed sometimes when the entire playlist fits in the buffer (search for "filling = false" in buffering.c). I think the storage_sleep() calls happen whenever the buffering thread has loaded everything the playback thread has given it up to that point, and this can happen multiple times during a single buffer filling.

Is this still valid after the rework of the buffering?

MikeS commented on 2011-10-21 13:25

Unlikely. Nothing really changed in the function in question and I thought about how to resolve it properly from within buffering and the point is that one cannot really do that since only playback knows when it has completed adding handles and only buffering really knows when it has completed all data for the last handle. Playback and buffering would have to better share info.

When I first created this task, I don't think I understood the code very well. Now, I don't think this is really a bug because it doesn't cause problems. The mmc_sleep() and sd_sleep() functions do nothing. Calling ata_sleep() only causes a spindown after half a second if there is no new disk activity during that time. If this was fixed, that half second of disk spinning time per buffer fill could be eliminated. I expect that on most targets the additional power usage is negligible. Should this remain open due to that half second?

MikeS commented on 2011-11-05 00:09

I think opening file descriptors is particularly slow and it opens quite a few of them and that's what causes the delay between tracks. With dircache, there's no appreciable delay. If there's any concern, increasing the actual timeout to go to sleep could be increase but I've not had any player with the disk going to sleep and coming back online between fills.

When there's no dircache and file descriptors are being opened, there must be disk activity. The activity keeps resetting last_disk_activity in ata.c and prevents sleep.

I was just looking at fill_buffer-if_interrupted.patch. It is still relevant, but it doesn't improve anything because the extra storage_sleep() that can happen there shouldn't have any effect.


Available keyboard shortcuts


Task Details

Task Editing