[Mesa-dev] [PATCH 1/4] util/disk_cache: add support for detecting corrupt cache entries

[Timothy Arceri]

On 01/03/17 23:58, Grazvydas Ignotas wrote:
> On Wed, Mar 1, 2017 at 7:25 AM, Timothy Arceri <tarceri at itsqueeze.com> wrote:
>> ---
>>  src/util/disk_cache.c | 37 ++++++++++++++++++++++++++++++++++---
>>  1 file changed, 34 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/util/disk_cache.c b/src/util/disk_cache.c
>> index f5e1145..2a0edca 100644
>> --- a/src/util/disk_cache.c
>> +++ b/src/util/disk_cache.c
>> @@ -31,20 +31,21 @@
>>  #include <sys/file.h>
>>  #include <sys/types.h>
>>  #include <sys/stat.h>
>>  #include <sys/mman.h>
>>  #include <unistd.h>
>>  #include <fcntl.h>
>>  #include <pwd.h>
>>  #include <errno.h>
>>  #include <dirent.h>
>>
>> +#include "util/crc32.h"
>>  #include "util/u_atomic.h"
>>  #include "util/mesa-sha1.h"
>>  #include "util/ralloc.h"
>>  #include "main/errors.h"
>>
>>  #include "disk_cache.h"
>>
>>  /* Number of bits to mask off from a cache key to get an index. */
>>  #define CACHE_INDEX_KEY_BITS 16
>>
>> @@ -702,34 +703,48 @@ disk_cache_put(struct disk_cache *cache,
>>     /* OK, we're now on the hook to write out a file that we know is
>>      * not in the cache, and is also not being written out to the cache
>>      * by some other process.
>>      *
>>      * Before we do that, if the cache is too large, evict something
>>      * else first.
>>      */
>>     if (*cache->size + size > cache->max_size)
>>        evict_random_item(cache);
>>
>> +   /* Create CRC of the data and store at the start of the file. We will
>> +    * read this when restoring the cache and use it to check for corruption.
>> +    */
>> +   uint32_t crc32 = util_hash_crc32(data, size);
>> +   size_t crc_size = sizeof(crc32);
>> +   for (len = 0; len < crc_size; len += ret) {
>> +      ret = write(fd, &crc32, crc_size - len);
>
> I think you need to advance the data pointer if retrying. There is
> also a case of EINTR where you should be retrying on error (ret ==
> -1). Might be worth making a helper function.

So looking at the docs:

"       * read(2), readv(2), write(2), writev(2), and ioctl(2) calls on
          "slow" devices.  A "slow" device is one where the I/O call may
          block for an indefinite time, for example, a terminal, pipe, or
          socket.  If an I/O call on a slow device has already transferred
          some data by the time it is interrupted by a signal handler, then
          the call will return a success status (normally, the number of
          bytes transferred).  Note that a (local) disk is not a slow device
          according to this definition; I/O operations on disk devices are
          not interrupted by signals."


I assume this means I don't even need the loop? Can anyone with more 
knowledge on this confirm?


>
>> +      if (ret == -1) {
>> +         unlink(filename_tmp);
>> +         goto done;
>> +      }
>> +   }
>> +
>>     /* Now, finally, write out the contents to the temporary file, then
>>      * rename them atomically to the destination filename, and also
>>      * perform an atomic increment of the total cache size.
>>      */
>>     for (len = 0; len < size; len += ret) {
>>        ret = write(fd, p + len, size - len);
>>        if (ret == -1) {
>>           unlink(filename_tmp);
>>           goto done;
>>        }
>>     }
>>
>>     rename(filename_tmp, filename);
>>
>> +   size += crc_size;
>>     p_atomic_add(cache->size, size);
>>
>>   done:
>>     if (fd_final != -1)
>>        close(fd_final);
>>     /* This close finally releases the flock, (now that the final dile
>>      * has been renamed into place and the size has been added).
>>      */
>>     if (fd != -1)
>>        close(fd);
>> @@ -758,31 +773,47 @@ disk_cache_get(struct disk_cache *cache, cache_key key, size_t *size)
>>     if (fd == -1)
>>        goto fail;
>>
>>     if (fstat(fd, &sb) == -1)
>>        goto fail;
>>
>>     data = malloc(sb.st_size);
>>     if (data == NULL)
>>        goto fail;
>>
>> -   for (len = 0; len < sb.st_size; len += ret) {
>> -      ret = read(fd, data + len, sb.st_size - len);
>> +   /* Load the CRC that was created when the file was written. */
>> +   uint32_t crc32;
>> +   size_t crc_size = sizeof(crc32);
>> +   assert(sb.st_size > crc_size);
>> +   for (len = 0; len < crc_size; len += ret) {
>> +      ret = read(fd, &crc32 + len, crc_size - len);
>
> &crc32 + len doesn't look right, it's a uint32_t * pointer. EINTR
> handling is also missing.
>
> Gražvydas
>