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

[Timothy Arceri]

On 02/03/17 11:00, Timothy Arceri wrote:
>
>
> 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?

Ok, so this could still happen on NFS so I've left it in but ignored the 
EINTR case. I'm happy to let it fallback to compiling rather than adding 
in more code for a use-case (home dirs on NFS) that is unlikely.


>
>
>>
>>> +      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
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev