playlist: Remove control file cache

Control cache entries cost 24 bytes per command, but cacheable
commands are always less than that when written out to the file.
We can actually cache *more* data by writing commands directly
to the fd (native Rockbox has a 512-byte cache per fd) and it's
much simpler.

Change-Id: Ibb1b9ffa56ef17431b281419a04082e14d0cbd85

apps/playlist.c

@@ -387,178 +387,55 @@ static int rotate_index(const struct playlist_info* playlist, int index)
     return index;
 }
 
-/*
+static void sync_control_unlocked(struct playlist_info* playlist)
- * sync control file to disk
- */
-static void sync_control(struct playlist_info* playlist, bool force)
 {
-#ifndef HAVE_DIRCACHE /*non dircache targets sync every time */
-    force = true;
-#endif
-
-    if (playlist->started && force)
-    {
-        if (playlist->pending_control_sync)
-        {
-            playlist_write_lock(playlist);
-
-            fsync(playlist->control_fd);
-            playlist->pending_control_sync = false;
-
-            playlist_write_unlock(playlist);
-        }
-    }
-}
-
-static int flush_cached_control_unlocked(struct playlist_info* playlist);
-
-#if USING_STORAGE_CALLBACK
-static void flush_control_cache_idle_cb(unsigned short id, void *ev, void *ud)
-{
-    (void)id;
-    (void)ev;
-
-    struct playlist_info *playlist = ud;
-    playlist_write_lock(playlist);
-
     if (playlist->control_fd >= 0)
-        flush_cached_control_unlocked(playlist);
+        fsync(playlist->control_fd);
-
-    playlist_write_unlock(playlist);
-}
-#endif
-
-/*
- * Flush any cached control commands to disk.  Called when playlist is being
- * modified.  Returns 0 on success and -1 on failure.
- */
-static int flush_cached_control_unlocked(struct playlist_info* playlist)
-{
-    int result = 0;
-
-    if (playlist->num_cached <= 0)
-        return 0;
-
-    lseek(playlist->control_fd, 0, SEEK_END);
-
-    for (int i = 0; i < playlist->num_cached; i++)
-    {
-        struct playlist_control_cache* cache = &playlist->control_cache[i];
-        switch (cache->command)
-        {
-            case PLAYLIST_COMMAND_PLAYLIST:
-                result = fdprintf(playlist->control_fd, "P:%d:%s:%s\n",
-                    cache->i1, cache->s1, cache->s2);
-                break;
-            case PLAYLIST_COMMAND_ADD:
-            case PLAYLIST_COMMAND_QUEUE:
-                result = fdprintf(playlist->control_fd, "%c:%d:%d:",
-                    (cache->command == PLAYLIST_COMMAND_ADD)?'A':'Q',
-                    cache->i1, cache->i2);
-                if (result > 0)
-                {
-                    /* save the position in file where name is written */
-                    int* seek_pos = (int *)cache->data;
-                    *seek_pos = lseek(playlist->control_fd, 0, SEEK_CUR);
-                    result = fdprintf(playlist->control_fd, "%s\n", cache->s1);
-                }
-                break;
-            case PLAYLIST_COMMAND_DELETE:
-                result = fdprintf(playlist->control_fd, "D:%d\n", cache->i1);
-                break;
-            case PLAYLIST_COMMAND_SHUFFLE:
-                result = fdprintf(playlist->control_fd, "S:%d:%d\n",
-                    cache->i1, cache->i2);
-                break;
-            case PLAYLIST_COMMAND_UNSHUFFLE:
-                result = fdprintf(playlist->control_fd, "U:%d\n", cache->i1);
-                break;
-            case PLAYLIST_COMMAND_RESET:
-                result = fdprintf(playlist->control_fd, "%s\n", "R");
-                break;
-            case PLAYLIST_COMMAND_CLEAR:
-                result = fdprintf(playlist->control_fd, "%s\n", "C");
-                break;
-            default:
-                break;
-        }
-
-        if (result <= 0)
-            break;
-    }
-
-    if (result > 0)
-    {
-        playlist->num_cached = 0;
-        playlist->pending_control_sync = true;
-        result = 0;
-    }
-    else
-    {
-        /* At this point the control file is likely corrupted. We still
-         * need to clear the cache to avoid a buffer overflow from the
-         * next command. It's unsafe to splash() because this function
-         * can be called off the main thread.
-         *
-         * TODO: recover from failed playlist control file writes.
-         */
-        playlist->num_cached = 0;
-        result = -1;
-    }
-
-#if USING_STORAGE_CALLBACK
-    remove_event_ex(DISK_EVENT_SPINUP, flush_control_cache_idle_cb, playlist);
-#endif
-    return result;
 }
 
-/*
+static int update_control_unlocked(struct playlist_info* playlist,
- * Update control data with new command.  Depending on the command, it may be
- * cached or flushed to disk.
- */
-static int update_control(struct playlist_info* playlist,
                                    enum playlist_command command, int i1, int i2,
-                          const char* s1, const char* s2, void* data)
+                                   const char* s1, const char* s2, int *seekpos)
 {
-    int result = 0;
+    int fd = playlist->control_fd;
-    struct playlist_control_cache* cache;
+    int result;
-    bool flush = false;
 
-    playlist_write_lock(playlist);
+    lseek(fd, 0, SEEK_END);
-    cache = &playlist->control_cache[playlist->num_cached++];
-
-    cache->command = command;
-    cache->i1 = i1;
-    cache->i2 = i2;
-    cache->s1 = s1;
-    cache->s2 = s2;
-    cache->data = data;
 
     switch (command)
     {
     case PLAYLIST_COMMAND_PLAYLIST:
+        result = fdprintf(fd, "P:%d:%s:%s\n", i1, s1, s2);
+        break;
     case PLAYLIST_COMMAND_ADD:
     case PLAYLIST_COMMAND_QUEUE:
-            /*
+        result = fdprintf(fd, "%c:%d:%d:",
-             * These commands can use s1/s2, which may point to
+                          command == PLAYLIST_COMMAND_ADD ? 'A' : 'Q', i1, i2);
-             * stack allocated buffers, so flush them immediately.
+        if (result > 0)
-             */
+        {
-            flush = true;
+            *seekpos = lseek(fd, 0, SEEK_CUR);
+            result = fdprintf(fd, "%s\n", s1);
+        }
+        break;
+    case PLAYLIST_COMMAND_DELETE:
+        result = fdprintf(fd, "D:%d\n", i1);
+        break;
+    case PLAYLIST_COMMAND_SHUFFLE:
+        result = fdprintf(fd, "S:%d:%d\n", i1, i2);
+        break;
+    case PLAYLIST_COMMAND_UNSHUFFLE:
+        result = fdprintf(fd, "U:%d\n", i1);
+        break;
+    case PLAYLIST_COMMAND_RESET:
+        result = write(fd, "R\n", 2);
+        break;
+    case PLAYLIST_COMMAND_CLEAR:
+        result = write(fd, "C\n", 2);
         break;
     default:
-            break;
+        return -1;
     }
 
-    if (flush || playlist->num_cached == PLAYLIST_MAX_CACHE)
-        result = flush_cached_control_unlocked(playlist);
-    else
-    {
-#if USING_STORAGE_CALLBACK
-        add_event_ex(DISK_EVENT_SPINUP, true, flush_control_cache_idle_cb, playlist);
-#endif
-    }
-
-    playlist_write_unlock(playlist);
     return result;
 }
 
@@ -602,7 +479,6 @@ static void empty_playlist_unlocked(struct playlist_info* playlist, bool resume)
     playlist->filename[0] = '\0';
 
     playlist->seed = 0;
-    playlist->num_cached = 0;
 
     playlist->utf8 = true;
     playlist->control_created = false;
@@ -617,7 +493,6 @@ static void empty_playlist_unlocked(struct playlist_info* playlist, bool resume)
     playlist->last_insert_pos = -1;
 
     playlist->started = false;
-    playlist->pending_control_sync = false;
 
     if (!resume && playlist == &current_playlist)
     {
@@ -713,9 +588,9 @@ static void new_playlist_unlocked(struct playlist_info* playlist,
 
     if (playlist->control_fd >= 0)
     {
-        update_control(playlist, PLAYLIST_COMMAND_PLAYLIST,
+        update_control_unlocked(playlist, PLAYLIST_COMMAND_PLAYLIST,
             PLAYLIST_CONTROL_FILE_VERSION, -1, dirused, fileused, NULL);
-        sync_control(playlist, false);
+        sync_control_unlocked(playlist);
     }
 }
 
@@ -740,9 +615,9 @@ static int check_control(struct playlist_info* playlist)
 
             playlist->filename[playlist->dirlen-1] = '\0';
 
-            update_control(playlist, PLAYLIST_COMMAND_PLAYLIST,
+            update_control_unlocked(playlist, PLAYLIST_COMMAND_PLAYLIST,
                 PLAYLIST_CONTROL_FILE_VERSION, -1, dir, file, NULL);
-            sync_control(playlist, false);
+            sync_control_unlocked(playlist);
             playlist->filename[playlist->dirlen-1] = c;
         }
     }
@@ -824,9 +699,8 @@ static int recreate_control_unlocked(struct playlist_info* playlist)
 
         playlist->filename[playlist->dirlen-1] = '\0';
 
-        /* cannot call update_control() because of mutex */
+        update_control_unlocked(playlist, PLAYLIST_COMMAND_PLAYLIST,
-        result = fdprintf(playlist->control_fd, "P:%d:%s:%s\n",
+                        PLAYLIST_CONTROL_FILE_VERSION, -1, dir, file, NULL);
-            PLAYLIST_CONTROL_FILE_VERSION, dir, file);
 
         playlist->filename[playlist->dirlen-1] = c;
 
@@ -1381,8 +1255,9 @@ static int remove_all_tracks_unlocked(struct playlist_info *playlist, bool write
 
     if (write && playlist->control_fd >= 0)
     {
-        update_control(playlist, PLAYLIST_COMMAND_CLEAR, -1, -1, NULL, NULL, NULL);
+        update_control_unlocked(playlist, PLAYLIST_COMMAND_CLEAR,
-        sync_control(playlist, false);
+                                -1, -1, NULL, NULL, NULL);
+        sync_control_unlocked(playlist);
     }
 
     return 0;
@@ -1534,7 +1409,7 @@ static int add_track_to_playlist_unlocked(struct playlist_info* playlist,
 
     if (seek_pos < 0 && playlist->control_fd >= 0)
     {
-        int result = update_control(playlist,
+        int result = update_control_unlocked(playlist,
             (queue?PLAYLIST_COMMAND_QUEUE:PLAYLIST_COMMAND_ADD), position,
             playlist->last_insert_pos, filename, NULL, &seek_pos);
 
@@ -1639,10 +1514,10 @@ static int remove_track_unlocked(struct playlist_info* playlist,
 
     if (write && playlist->control_fd >= 0)
     {
-        result = update_control(playlist, PLAYLIST_COMMAND_DELETE,
+        result = update_control_unlocked(playlist, PLAYLIST_COMMAND_DELETE,
             position, -1, NULL, NULL, NULL);
         if (result >= 0)
-            sync_control(playlist, false);
+            sync_control_unlocked(playlist);
     }
 
     return result;
@@ -1720,7 +1595,7 @@ static int randomise_playlist_unlocked(struct playlist_info* playlist,
 
     if (write)
     {
-        update_control(playlist, PLAYLIST_COMMAND_SHUFFLE, seed,
+        update_control_unlocked(playlist, PLAYLIST_COMMAND_SHUFFLE, seed,
             playlist->first_index, NULL, NULL, NULL);
     }
 
@@ -1785,7 +1660,7 @@ static int sort_playlist_unlocked(struct playlist_info* playlist,
     if (write && playlist->control_fd >= 0)
     {
         playlist->first_index = 0;
-        update_control(playlist, PLAYLIST_COMMAND_UNSHUFFLE,
+        update_control_unlocked(playlist, PLAYLIST_COMMAND_UNSHUFFLE,
             playlist->first_index, -1, NULL, NULL, NULL);
     }
 
@@ -2144,10 +2019,7 @@ void playlist_shutdown(void)
     playlist_write_lock(playlist);
 
     if (playlist->control_fd >= 0)
-    {
-        flush_cached_control_unlocked(playlist);
         pl_close_control(playlist);
-    }
 
     playlist_write_unlock(playlist);
 }
@@ -2645,7 +2517,7 @@ int playlist_insert_directory(struct playlist_info* playlist,
     result = playlist_directory_tracksearch(dirname, recurse,
         directory_search_callback, &context);
 
-    sync_control(playlist, false);
+    sync_control_unlocked(playlist);
 
     cpu_boost(false);
 
@@ -2779,7 +2651,7 @@ int playlist_insert_playlist(struct playlist_info* playlist, const char *filenam
 
     close(fd);
 
-    sync_control(playlist, false);
+    sync_control_unlocked(playlist);
 
     display_playlist_count(count, count_str, true);
 
@@ -3092,15 +2964,16 @@ int playlist_next(int steps)
             playlist->last_insert_pos = -1;
             if (playlist->control_fd >= 0)
             {
-                int result = update_control(playlist, PLAYLIST_COMMAND_RESET,
+                int result = update_control_unlocked(playlist,
+                                                     PLAYLIST_COMMAND_RESET,
                                                      -1, -1, NULL, NULL, NULL);
-
                 if (result < 0)
                 {
                     index = result;
                     goto out;
                 }
-                sync_control(playlist, false);
+
+                sync_control_unlocked(playlist);
             }
         }
     }
@@ -3928,11 +3801,6 @@ int playlist_set_current(struct playlist_info* playlist)
     current_playlist.seed = playlist->seed;
     current_playlist.modified = playlist->modified;
 
-    memcpy(current_playlist.control_cache, playlist->control_cache,
-        sizeof(current_playlist.control_cache));
-
-    current_playlist.num_cached = playlist->num_cached;
-    current_playlist.pending_control_sync = playlist->pending_control_sync;
     result = 0;
 
 out:
@@ -4034,7 +3902,7 @@ void playlist_start(int start_index, unsigned long elapsed,
     playlist->index = start_index;
     playlist->started = true;
 
-    sync_control(playlist, false);
+    sync_control_unlocked(playlist);
 
     playlist_write_unlock(playlist);
 
@@ -4047,7 +3915,12 @@ void playlist_sync(struct playlist_info* playlist)
     if (!playlist)
         playlist = &current_playlist;
 
-    sync_control(playlist, false);
+    playlist_write_lock(playlist);
+
+    sync_control_unlocked(playlist);
+
+    playlist_write_unlock(playlist);
+
     if ((audio_status() & AUDIO_STATUS_PLAY) && playlist->started)
         audio_flush_and_reload_tracks();
 }

apps/playlist.h

@@ -33,7 +33,6 @@
 #define PLAYLIST_ATTR_QUEUED    0x01
 #define PLAYLIST_ATTR_INSERTED  0x02
 #define PLAYLIST_ATTR_SKIPPED   0x04
-#define PLAYLIST_MAX_CACHE      16
 
 #define PLAYLIST_DISPLAY_COUNT  10
 
@@ -61,15 +60,6 @@ enum {
     PLAYLIST_INSERT_LAST_SHUFFLED = -7
 };
 
-struct playlist_control_cache {
-    enum playlist_command command;
-    int i1;
-    int i2;
-    const char* s1;
-    const char* s2;
-    void* data;
-};
-
 struct playlist_info
 {
     bool utf8;           /* playlist is in .m3u8 format             */
@@ -87,14 +77,9 @@ struct playlist_info
     int  amount;         /* number of tracks in the index           */
     int  last_insert_pos; /* last position we inserted a track      */
     bool started;       /* has playlist been started?               */
-    bool pending_control_sync; /* control file needs to be synced   */
     int last_shuffled_start; /* number of tracks when insert last
                                     shuffled command start */
     int  seed;           /* shuffle seed                            */
-    /* cache of playlist control commands waiting to be flushed to
-       to disk                                                      */
-    struct playlist_control_cache control_cache[PLAYLIST_MAX_CACHE];
-    int num_cached;      /* number of cached entries                */
     struct mutex mutex; /* mutex for control file access    */
 #ifdef HAVE_DIRCACHE
     int dcfrefs_handle;