[Mesa-stable] [Mesa-dev] [PATCH 2/2] disk_cache: use block size rather than file size

[Grazvydas Ignotas]

On Thu, Apr 27, 2017 at 4:15 AM, Timothy Arceri <tarceri at itsqueeze.com>
wrote:

> The majority of cache files are less than 1kb this resulted in us
> greatly miscalculating the amount of disk space used by the cache.
>
> Using the number of blocks allocated to the file is more
> conservative and less likely to cause issues.
>
> This change will result in cache sizes being miscalculated further
> until old items added with the previous calculation have all been
> removed. However I don't see anyway around that, the previous
> patch should help limit that problem.
>

It may underflow now though, as it will be subtracting more than it was
adding. Because size is unsigned, it will get stuck in "cache is too large,
let's evict" state forever until user manually clears the cache. Not sure
how to solve that...


>
> Cc: "17.1" <mesa-stable at lists.freedesktop.org>
> ---
>  src/util/disk_cache.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/disk_cache.c b/src/util/disk_cache.c
> index 9fd7b96..2764017 100644
> --- a/src/util/disk_cache.c
> +++ b/src/util/disk_cache.c
> @@ -525,21 +525,21 @@ unlink_lru_file_from_directory(const char *path)
>        return 0;
>
>     if (stat(filename, &sb) == -1) {
>        free (filename);
>        return 0;
>     }
>
>     unlink(filename);
>     free (filename);
>
> -   return sb.st_size;
> +   return sb.st_blocks * 512;
>

I think other block sizes than 512 are possible, you can use sb.st_blocks *
sb.st_blksize to take care of that.


>  }
>
>  /* Is entry a directory with a two-character name, (and not the
>   * special name of ".."). We also return false if the dir is empty.
>   */
>  static bool
>  is_two_character_sub_directory(const char *path, const struct stat *sb,
>                                 const char *d_name, const size_t len)
>  {
>     if (!S_ISDIR(sb->st_mode))
> @@ -630,22 +630,22 @@ disk_cache_remove(struct disk_cache *cache, const
> cache_key key)
>     }
>
>     if (stat(filename, &sb) == -1) {
>        free(filename);
>        return;
>     }
>
>     unlink(filename);
>     free(filename);
>
> -   if (sb.st_size)
> -      p_atomic_add(cache->size, - (uint64_t)sb.st_size);
> +   if (sb.st_blocks * 512)
> +      p_atomic_add(cache->size, - (uint64_t)sb.st_blocks * 512);
>  }
>
>  static ssize_t
>  read_all(int fd, void *buf, size_t count)
>  {
>     char *in = buf;
>     ssize_t read_ret;
>     size_t done;
>
>     for (done = 0; done < count; done += read_ret) {
> @@ -873,22 +873,28 @@ cache_put(void *job, int thread_index)
>     if (file_size == 0) {
>        unlink(filename_tmp);
>        goto done;
>     }
>     ret = rename(filename_tmp, filename);
>     if (ret == -1) {
>        unlink(filename_tmp);
>        goto done;
>     }
>
> -   file_size += cf_data_size + dc_job->cache->driver_keys_blob_size;
> -   p_atomic_add(dc_job->cache->size, file_size);
> +   struct stat sb;
> +   if (stat(filename, &sb) == -1) {
>

fstat(fd, &sb) might be easier on the kernel (no need to resolve the
pathname), but stat() is also fine if you prefer it.


> +      /* Something went wrong remove the file */
> +      unlink(filename);
> +      goto done;
> +   }
>

Maybe keep file_size calculation above and add an assert(file_size <=
sb.st_blocks * sb.st_blksize); ?

Grazvydas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20170427/639ad763/attachment-0001.html>