From b19a51c04437e04d5f530b33f2dd1fca67aedf83 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 16 Dec 2020 21:15:30 -0600 Subject: [PATCH] Reduced when custom attributse are written to strictly when files are dirty This is a bit of a complicated area for the custom-attribute API without much precedent. littlefs allows users to provide custom attributes in the lfs_file_config struct, which get written along with other file metadata. Sounds great on paper, but the devil is in the details. When does the metadata actually get written? What about this case? lfs_file_opencfg(lfs, file, "path", LFS_O_WRONLY, cfg_with_attrs); lfs_file_close(lfs, file); // does not write metadata This normally doesn't write out metadata! We've opened the file for writing, but made no changes, so normally littlefs doesn't bother to commit anything to disk. Before, as a courtesy, littlefs marked the file as dirty if it noticed the file was opened for writing with custom attributes, but this is inaccurate could to leave to problems after a file is synced: lfs_file_opencfg(lfs, file, "path", LFS_O_WRONLY, cfg_with_attrs); lfs_file_sync(lfs, file); change_attrs(); lfs_file_close(lfs, file); // does not write metadata Unfortunately, it isn't easy to know when metadata needs to be written. Custom attributes are provided as read-only pointers to buffers which may be updated without additional filesystem calls, this means we don't know if custom attributes have actually changed on the device side. If they haven't changed, writing out metadata on every sync would be wasteful. Another solution would be to compare our device-side attributes with the disk-side attributes every sync, but that would be even more expensive. --- So for now, the simpliest and most efficient solution wins. Custom attributes attached to open files, are not written unless the file data itself changes. Note that explicit calls to lfs_setattr always update on-disk attributes, and opening a file with LFS_O_CREATE | LFS_O_TRUNC will also always update the on-disk attributes (though not with just LFS_O_CREAT!). There are a few ways we could provide an API that manually forces a write of custom attributes, such as lfs_file_setattr, though without dynamic memory, providing these APIs gets a bit complicated. So for now we will see if users run into issues with the current scheme. --- lfs.c | 8 +-- tests/test_attrs.toml | 126 ++++++++++++++++++++++-------------------- 2 files changed, 69 insertions(+), 65 deletions(-) diff --git a/lfs.c b/lfs.c index bd902ea..8376d7d 100644 --- a/lfs.c +++ b/lfs.c @@ -2506,11 +2506,12 @@ static int lfs_file_rawopencfg(lfs_t *lfs, lfs_file_t *file, } // get next slot and create entry to remember name - // TODO commit with attrs? err = lfs_dir_commit(lfs, &file->m, LFS_MKATTRS( {LFS_MKTAG(LFS_TYPE_CREATE, file->id, 0), NULL}, {LFS_MKTAG(LFS_TYPE_REG, file->id, nlen), path}, - {LFS_MKTAG(LFS_TYPE_INLINESTRUCT, file->id, 0), NULL})); + {LFS_MKTAG(LFS_TYPE_INLINESTRUCT, file->id, 0), NULL}, + {LFS_MKTAG(LFS_FROM_USERATTRS, file->id, + file->cfg->attr_count), file->cfg->attrs})); if (err) { err = LFS_ERR_NAMETOOLONG; goto cleanup; @@ -2563,9 +2564,6 @@ static int lfs_file_rawopencfg(lfs_t *lfs, lfs_file_t *file, err = LFS_ERR_NOSPC; goto cleanup; } - - // TODO remove this? - file->flags |= LFS_F_DIRTY; } #endif } diff --git a/tests/test_attrs.toml b/tests/test_attrs.toml index db8d0c7..6148463 100644 --- a/tests/test_attrs.toml +++ b/tests/test_attrs.toml @@ -16,41 +16,41 @@ code = ''' lfs_getattr(&lfs, "hello", 'A', buffer, 4) => 4; lfs_getattr(&lfs, "hello", 'B', buffer+4, 6) => 6; lfs_getattr(&lfs, "hello", 'C', buffer+10, 5) => 5; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "bbbbbb", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "bbbbbb", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); lfs_setattr(&lfs, "hello", 'B', "", 0) => 0; lfs_getattr(&lfs, "hello", 'A', buffer, 4) => 4; lfs_getattr(&lfs, "hello", 'B', buffer+4, 6) => 0; lfs_getattr(&lfs, "hello", 'C', buffer+10, 5) => 5; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "\0\0\0\0\0\0", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "\0\0\0\0\0\0", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); lfs_removeattr(&lfs, "hello", 'B') => 0; lfs_getattr(&lfs, "hello", 'A', buffer, 4) => 4; lfs_getattr(&lfs, "hello", 'B', buffer+4, 6) => LFS_ERR_NOATTR; lfs_getattr(&lfs, "hello", 'C', buffer+10, 5) => 5; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "\0\0\0\0\0\0", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "\0\0\0\0\0\0", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); lfs_setattr(&lfs, "hello", 'B', "dddddd", 6) => 0; lfs_getattr(&lfs, "hello", 'A', buffer, 4) => 4; lfs_getattr(&lfs, "hello", 'B', buffer+4, 6) => 6; lfs_getattr(&lfs, "hello", 'C', buffer+10, 5) => 5; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "dddddd", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "dddddd", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); lfs_setattr(&lfs, "hello", 'B', "eee", 3) => 0; lfs_getattr(&lfs, "hello", 'A', buffer, 4) => 4; lfs_getattr(&lfs, "hello", 'B', buffer+4, 6) => 3; lfs_getattr(&lfs, "hello", 'C', buffer+10, 5) => 5; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "eee\0\0\0", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "eee\0\0\0", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); lfs_setattr(&lfs, "hello", 'A', buffer, LFS_ATTR_MAX+1) => LFS_ERR_NOSPC; lfs_setattr(&lfs, "hello", 'B', "fffffffff", 9) => 0; @@ -65,13 +65,13 @@ code = ''' lfs_getattr(&lfs, "hello", 'A', buffer, 4) => 4; lfs_getattr(&lfs, "hello", 'B', buffer+4, 9) => 9; lfs_getattr(&lfs, "hello", 'C', buffer+13, 5) => 5; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "fffffffff", 9) => 0; - memcmp(buffer+13, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "fffffffff", 9) == 0); + assert(memcmp(buffer+13, "ccccc", 5) == 0); lfs_file_open(&lfs, &file, "hello/hello", LFS_O_RDONLY) => 0; lfs_file_read(&lfs, &file, buffer, sizeof(buffer)) => strlen("hello"); - memcmp(buffer, "hello", strlen("hello")) => 0; + assert(memcmp(buffer, "hello", strlen("hello")) == 0); lfs_file_close(&lfs, &file); lfs_unmount(&lfs) => 0; ''' @@ -94,41 +94,41 @@ code = ''' lfs_getattr(&lfs, "/", 'A', buffer, 4) => 4; lfs_getattr(&lfs, "/", 'B', buffer+4, 6) => 6; lfs_getattr(&lfs, "/", 'C', buffer+10, 5) => 5; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "bbbbbb", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "bbbbbb", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); lfs_setattr(&lfs, "/", 'B', "", 0) => 0; lfs_getattr(&lfs, "/", 'A', buffer, 4) => 4; lfs_getattr(&lfs, "/", 'B', buffer+4, 6) => 0; lfs_getattr(&lfs, "/", 'C', buffer+10, 5) => 5; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "\0\0\0\0\0\0", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "\0\0\0\0\0\0", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); lfs_removeattr(&lfs, "/", 'B') => 0; lfs_getattr(&lfs, "/", 'A', buffer, 4) => 4; lfs_getattr(&lfs, "/", 'B', buffer+4, 6) => LFS_ERR_NOATTR; lfs_getattr(&lfs, "/", 'C', buffer+10, 5) => 5; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "\0\0\0\0\0\0", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "\0\0\0\0\0\0", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); lfs_setattr(&lfs, "/", 'B', "dddddd", 6) => 0; lfs_getattr(&lfs, "/", 'A', buffer, 4) => 4; lfs_getattr(&lfs, "/", 'B', buffer+4, 6) => 6; lfs_getattr(&lfs, "/", 'C', buffer+10, 5) => 5; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "dddddd", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "dddddd", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); lfs_setattr(&lfs, "/", 'B', "eee", 3) => 0; lfs_getattr(&lfs, "/", 'A', buffer, 4) => 4; lfs_getattr(&lfs, "/", 'B', buffer+4, 6) => 3; lfs_getattr(&lfs, "/", 'C', buffer+10, 5) => 5; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "eee\0\0\0", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "eee\0\0\0", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); lfs_setattr(&lfs, "/", 'A', buffer, LFS_ATTR_MAX+1) => LFS_ERR_NOSPC; lfs_setattr(&lfs, "/", 'B', "fffffffff", 9) => 0; @@ -142,13 +142,13 @@ code = ''' lfs_getattr(&lfs, "/", 'A', buffer, 4) => 4; lfs_getattr(&lfs, "/", 'B', buffer+4, 9) => 9; lfs_getattr(&lfs, "/", 'C', buffer+13, 5) => 5; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "fffffffff", 9) => 0; - memcmp(buffer+13, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "fffffffff", 9) == 0); + assert(memcmp(buffer+13, "ccccc", 5) == 0); lfs_file_open(&lfs, &file, "hello/hello", LFS_O_RDONLY) => 0; lfs_file_read(&lfs, &file, buffer, sizeof(buffer)) => strlen("hello"); - memcmp(buffer, "hello", strlen("hello")) => 0; + assert(memcmp(buffer, "hello", strlen("hello")) == 0); lfs_file_close(&lfs, &file); lfs_unmount(&lfs) => 0; ''' @@ -176,48 +176,52 @@ code = ''' memcpy(buffer, "aaaa", 4); memcpy(buffer+4, "bbbbbb", 6); memcpy(buffer+10, "ccccc", 5); + lfs_file_write(&lfs, &file, "hi", 2) => 2; lfs_file_close(&lfs, &file) => 0; memset(buffer, 0, 15); lfs_file_opencfg(&lfs, &file, "hello/hello", LFS_O_RDONLY, &cfg1) => 0; lfs_file_close(&lfs, &file) => 0; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "bbbbbb", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "bbbbbb", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); attrs1[1].size = 0; lfs_file_opencfg(&lfs, &file, "hello/hello", LFS_O_WRONLY, &cfg1) => 0; + lfs_file_write(&lfs, &file, "hi", 2) => 2; lfs_file_close(&lfs, &file) => 0; memset(buffer, 0, 15); attrs1[1].size = 6; lfs_file_opencfg(&lfs, &file, "hello/hello", LFS_O_RDONLY, &cfg1) => 0; lfs_file_close(&lfs, &file) => 0; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "\0\0\0\0\0\0", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "\0\0\0\0\0\0", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); attrs1[1].size = 6; lfs_file_opencfg(&lfs, &file, "hello/hello", LFS_O_WRONLY, &cfg1) => 0; memcpy(buffer+4, "dddddd", 6); + lfs_file_write(&lfs, &file, "hi", 2) => 2; lfs_file_close(&lfs, &file) => 0; memset(buffer, 0, 15); attrs1[1].size = 6; lfs_file_opencfg(&lfs, &file, "hello/hello", LFS_O_RDONLY, &cfg1) => 0; lfs_file_close(&lfs, &file) => 0; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "dddddd", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "dddddd", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); attrs1[1].size = 3; lfs_file_opencfg(&lfs, &file, "hello/hello", LFS_O_WRONLY, &cfg1) => 0; memcpy(buffer+4, "eee", 3); + lfs_file_write(&lfs, &file, "hi", 2) => 2; lfs_file_close(&lfs, &file) => 0; memset(buffer, 0, 15); attrs1[1].size = 6; lfs_file_opencfg(&lfs, &file, "hello/hello", LFS_O_RDONLY, &cfg1) => 0; lfs_file_close(&lfs, &file) => 0; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "eee\0\0\0", 6) => 0; - memcmp(buffer+10, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "eee\0\0\0", 6) == 0); + assert(memcmp(buffer+10, "ccccc", 5) == 0); attrs1[0].size = LFS_ATTR_MAX+1; lfs_file_opencfg(&lfs, &file, "hello/hello", LFS_O_WRONLY, &cfg1) @@ -231,6 +235,7 @@ code = ''' struct lfs_file_config cfg2 = {.attrs=attrs2, .attr_count=3}; lfs_file_opencfg(&lfs, &file, "hello/hello", LFS_O_RDWR, &cfg2) => 0; memcpy(buffer+4, "fffffffff", 9); + lfs_file_write(&lfs, &file, "hi", 2) => 2; lfs_file_close(&lfs, &file) => 0; attrs1[0].size = 4; lfs_file_opencfg(&lfs, &file, "hello/hello", LFS_O_RDONLY, &cfg1) => 0; @@ -249,13 +254,13 @@ code = ''' lfs_file_opencfg(&lfs, &file, "hello/hello", LFS_O_RDONLY, &cfg3) => 0; lfs_file_close(&lfs, &file) => 0; - memcmp(buffer, "aaaa", 4) => 0; - memcmp(buffer+4, "fffffffff", 9) => 0; - memcmp(buffer+13, "ccccc", 5) => 0; + assert(memcmp(buffer, "aaaa", 4) == 0); + assert(memcmp(buffer+4, "fffffffff", 9) == 0); + assert(memcmp(buffer+13, "ccccc", 5) == 0); lfs_file_open(&lfs, &file, "hello/hello", LFS_O_RDONLY) => 0; lfs_file_read(&lfs, &file, buffer, sizeof(buffer)) => strlen("hello"); - memcmp(buffer, "hello", strlen("hello")) => 0; + assert(memcmp(buffer, "hillo", strlen("hello")) == 0); lfs_file_close(&lfs, &file); lfs_unmount(&lfs) => 0; ''' @@ -287,17 +292,18 @@ code = ''' lfs_getattr(&lfs, "hello/hello", 'B', buffer, 9) => 9; lfs_getattr(&lfs, "hello/hello", 'C', buffer+9, 9) => 5; lfs_getattr(&lfs, "hello/hello", 'D', buffer+18, 9) => LFS_ERR_NOATTR; - memcmp(buffer, "fffffffff", 9) => 0; - memcmp(buffer+9, "ccccc\0\0\0\0", 9) => 0; - memcmp(buffer+18, "\0\0\0\0\0\0\0\0\0", 9) => 0; + assert(memcmp(buffer, "fffffffff", 9) == 0); + assert(memcmp(buffer+9, "ccccc\0\0\0\0", 9) == 0); + assert(memcmp(buffer+18, "\0\0\0\0\0\0\0\0\0", 9) == 0); + lfs_file_write(&lfs, &file, "hi", 2) => 2; lfs_file_sync(&lfs, &file) => 0; lfs_getattr(&lfs, "hello/hello", 'B', buffer, 9) => 4; lfs_getattr(&lfs, "hello/hello", 'C', buffer+9, 9) => 0; lfs_getattr(&lfs, "hello/hello", 'D', buffer+18, 9) => 4; - memcmp(buffer, "gggg\0\0\0\0\0", 9) => 0; - memcmp(buffer+9, "\0\0\0\0\0\0\0\0\0", 9) => 0; - memcmp(buffer+18, "hhhh\0\0\0\0\0", 9) => 0; + assert(memcmp(buffer, "gggg\0\0\0\0\0", 9) == 0); + assert(memcmp(buffer+9, "\0\0\0\0\0\0\0\0\0", 9) == 0); + assert(memcmp(buffer+18, "hhhh\0\0\0\0\0", 9) == 0); lfs_file_close(&lfs, &file) => 0; lfs_unmount(&lfs) => 0;