From 7b151e1abbfcd4e45c3c4fbe61bdf4784644bf18 Mon Sep 17 00:00:00 2001 From: Colin Foster Date: Thu, 26 Jan 2023 11:26:37 -0800 Subject: [PATCH 1/4] Add test scenario for truncating to a block size When truncation is done on a file to the block size, there seems to be an error where it points to an incorrect block. Perform a write / truncate / readback operation to verify this issue. Signed-off-by: Colin Foster --- tests/test_truncate.toml | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/test_truncate.toml b/tests/test_truncate.toml index 850d7aae..4817ea23 100644 --- a/tests/test_truncate.toml +++ b/tests/test_truncate.toml @@ -437,3 +437,37 @@ code = ''' lfs_file_close(&lfs, &file) => 0; lfs_unmount(&lfs) => 0; ''' + +[[case]] # barrier truncate +code = ''' + lfs_format(&lfs, &cfg) => 0; + lfs_mount(&lfs, &cfg) => 0; + lfs_file_open(&lfs, &file, "barrier", + LFS_O_RDWR | LFS_O_CREAT) => 0; + + uint8_t bad_byte = 2; + uint8_t *rb = buffer + cfg.block_size; + + /* Write a series of 1s to the first block */ + memset(buffer, 1, cfg.block_size); + lfs_file_write(&lfs, &file, buffer, + cfg.block_size) => cfg.block_size; + + /* Write a single non-one to the second block */ + lfs_file_write(&lfs, &file, &bad_byte, 1) => 1; + lfs_file_close(&lfs, &file) => 0; + + lfs_file_open(&lfs, &file, "barrier", LFS_O_RDWR) => 0; + lfs_file_truncate(&lfs, &file, cfg.block_size) => 0; + lfs_file_close(&lfs, &file) => 0; + + /* Read the first block, which should match buffer */ + lfs_file_open(&lfs, &file, "barrier", LFS_O_RDWR) => 0; + lfs_file_read(&lfs, &file, rb, + cfg.block_size) => cfg.block_size; + lfs_file_close(&lfs, &file) => 0; + + lfs_unmount(&lfs) => 0; + + memcmp(buffer, rb, cfg.block_size) => 0; +''' From d5dc4872cba4d266156795587d5a52a77ec87c26 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 17 Apr 2023 12:03:35 -0500 Subject: [PATCH 2/4] Expanded truncate tests to test more corner cases Removed the weird alignment requirement from the general truncate tests. This explicitly hid off-by-one truncation errors. These tests now reveal the same issue as the block-sized truncation test while also testing for other potential off-by-one errors. --- tests/test_truncate.toml | 126 ++++++++++++++++++--------------------- 1 file changed, 57 insertions(+), 69 deletions(-) diff --git a/tests/test_truncate.toml b/tests/test_truncate.toml index 4817ea23..4801a48a 100644 --- a/tests/test_truncate.toml +++ b/tests/test_truncate.toml @@ -1,6 +1,7 @@ [[case]] # simple truncate -define.MEDIUMSIZE = [32, 2048] -define.LARGESIZE = 8192 +define.MEDIUMSIZE = [31, 32, 33, 511, 512, 513, 2047, 2048, 2049] +define.LARGESIZE = [32, 33, 512, 513, 2048, 2049, 8192, 8193] +if = 'MEDIUMSIZE < LARGESIZE' code = ''' lfs_format(&lfs, &cfg) => 0; lfs_mount(&lfs, &cfg) => 0; @@ -10,13 +11,14 @@ code = ''' strcpy((char*)buffer, "hair"); size = strlen((char*)buffer); for (lfs_off_t j = 0; j < LARGESIZE; j += size) { - lfs_file_write(&lfs, &file, buffer, size) => size; + lfs_file_write(&lfs, &file, buffer, lfs_min(size, LARGESIZE-j)) + => lfs_min(size, LARGESIZE-j); } lfs_file_size(&lfs, &file) => LARGESIZE; lfs_file_close(&lfs, &file) => 0; lfs_unmount(&lfs) => 0; - + lfs_mount(&lfs, &cfg) => 0; lfs_file_open(&lfs, &file, "baldynoop", LFS_O_RDWR) => 0; lfs_file_size(&lfs, &file) => LARGESIZE; @@ -33,8 +35,9 @@ code = ''' size = strlen("hair"); for (lfs_off_t j = 0; j < MEDIUMSIZE; j += size) { - lfs_file_read(&lfs, &file, buffer, size) => size; - memcmp(buffer, "hair", size) => 0; + lfs_file_read(&lfs, &file, buffer, lfs_min(size, MEDIUMSIZE-j)) + => lfs_min(size, MEDIUMSIZE-j); + memcmp(buffer, "hair", lfs_min(size, MEDIUMSIZE-j)) => 0; } lfs_file_read(&lfs, &file, buffer, size) => 0; @@ -43,8 +46,9 @@ code = ''' ''' [[case]] # truncate and read -define.MEDIUMSIZE = [32, 2048] -define.LARGESIZE = 8192 +define.MEDIUMSIZE = [31, 32, 33, 511, 512, 513, 2047, 2048, 2049] +define.LARGESIZE = [32, 33, 512, 513, 2048, 2049, 8192, 8193] +if = 'MEDIUMSIZE < LARGESIZE' code = ''' lfs_format(&lfs, &cfg) => 0; lfs_mount(&lfs, &cfg) => 0; @@ -54,7 +58,8 @@ code = ''' strcpy((char*)buffer, "hair"); size = strlen((char*)buffer); for (lfs_off_t j = 0; j < LARGESIZE; j += size) { - lfs_file_write(&lfs, &file, buffer, size) => size; + lfs_file_write(&lfs, &file, buffer, lfs_min(size, LARGESIZE-j)) + => lfs_min(size, LARGESIZE-j); } lfs_file_size(&lfs, &file) => LARGESIZE; @@ -70,8 +75,9 @@ code = ''' size = strlen("hair"); for (lfs_off_t j = 0; j < MEDIUMSIZE; j += size) { - lfs_file_read(&lfs, &file, buffer, size) => size; - memcmp(buffer, "hair", size) => 0; + lfs_file_read(&lfs, &file, buffer, lfs_min(size, MEDIUMSIZE-j)) + => lfs_min(size, MEDIUMSIZE-j); + memcmp(buffer, "hair", lfs_min(size, MEDIUMSIZE-j)) => 0; } lfs_file_read(&lfs, &file, buffer, size) => 0; @@ -84,8 +90,9 @@ code = ''' size = strlen("hair"); for (lfs_off_t j = 0; j < MEDIUMSIZE; j += size) { - lfs_file_read(&lfs, &file, buffer, size) => size; - memcmp(buffer, "hair", size) => 0; + lfs_file_read(&lfs, &file, buffer, lfs_min(size, MEDIUMSIZE-j)) + => lfs_min(size, MEDIUMSIZE-j); + memcmp(buffer, "hair", lfs_min(size, MEDIUMSIZE-j)) => 0; } lfs_file_read(&lfs, &file, buffer, size) => 0; @@ -136,7 +143,7 @@ code = ''' lfs_file_truncate(&lfs, &file, trunc) => 0; lfs_file_tell(&lfs, &file) => qsize; lfs_file_size(&lfs, &file) => trunc; - + /* Read should produce second quarter */ lfs_file_read(&lfs, &file, rb, size) => trunc - qsize; memcmp(rb, wb + qsize, trunc - qsize) => 0; @@ -146,8 +153,9 @@ code = ''' ''' [[case]] # truncate and write -define.MEDIUMSIZE = [32, 2048] -define.LARGESIZE = 8192 +define.MEDIUMSIZE = [31, 32, 33, 511, 512, 513, 2047, 2048, 2049] +define.LARGESIZE = [32, 33, 512, 513, 2048, 2049, 8192, 8193] +if = 'MEDIUMSIZE < LARGESIZE' code = ''' lfs_format(&lfs, &cfg) => 0; lfs_mount(&lfs, &cfg) => 0; @@ -157,7 +165,8 @@ code = ''' strcpy((char*)buffer, "hair"); size = strlen((char*)buffer); for (lfs_off_t j = 0; j < LARGESIZE; j += size) { - lfs_file_write(&lfs, &file, buffer, size) => size; + lfs_file_write(&lfs, &file, buffer, lfs_min(size, LARGESIZE-j)) + => lfs_min(size, LARGESIZE-j); } lfs_file_size(&lfs, &file) => LARGESIZE; @@ -168,13 +177,16 @@ code = ''' lfs_file_open(&lfs, &file, "baldywrite", LFS_O_RDWR) => 0; lfs_file_size(&lfs, &file) => LARGESIZE; + /* truncate */ lfs_file_truncate(&lfs, &file, MEDIUMSIZE) => 0; lfs_file_size(&lfs, &file) => MEDIUMSIZE; + /* and write */ strcpy((char*)buffer, "bald"); size = strlen((char*)buffer); for (lfs_off_t j = 0; j < MEDIUMSIZE; j += size) { - lfs_file_write(&lfs, &file, buffer, size) => size; + lfs_file_write(&lfs, &file, buffer, lfs_min(size, MEDIUMSIZE-j)) + => lfs_min(size, MEDIUMSIZE-j); } lfs_file_size(&lfs, &file) => MEDIUMSIZE; @@ -187,8 +199,9 @@ code = ''' size = strlen("bald"); for (lfs_off_t j = 0; j < MEDIUMSIZE; j += size) { - lfs_file_read(&lfs, &file, buffer, size) => size; - memcmp(buffer, "bald", size) => 0; + lfs_file_read(&lfs, &file, buffer, lfs_min(size, MEDIUMSIZE-j)) + => lfs_min(size, MEDIUMSIZE-j); + memcmp(buffer, "bald", lfs_min(size, MEDIUMSIZE-j)) => 0; } lfs_file_read(&lfs, &file, buffer, size) => 0; @@ -198,7 +211,7 @@ code = ''' [[case]] # truncate write under powerloss define.SMALLSIZE = [4, 512] -define.MEDIUMSIZE = [32, 1024] +define.MEDIUMSIZE = [0, 3, 4, 5, 31, 32, 33, 511, 512, 513, 1023, 1024, 1025] define.LARGESIZE = 2048 reentrant = true code = ''' @@ -216,10 +229,11 @@ code = ''' size == MEDIUMSIZE || size == SMALLSIZE); for (lfs_off_t j = 0; j < size; j += 4) { - lfs_file_read(&lfs, &file, buffer, 4) => 4; - assert(memcmp(buffer, "hair", 4) == 0 || - memcmp(buffer, "bald", 4) == 0 || - memcmp(buffer, "comb", 4) == 0); + lfs_file_read(&lfs, &file, buffer, lfs_min(4, size-j)) + => lfs_min(4, size-j); + assert(memcmp(buffer, "hair", lfs_min(4, size-j)) == 0 || + memcmp(buffer, "bald", lfs_min(4, size-j)) == 0 || + memcmp(buffer, "comb", lfs_min(4, size-j)) == 0); } lfs_file_close(&lfs, &file) => 0; } @@ -230,19 +244,23 @@ code = ''' strcpy((char*)buffer, "hair"); size = strlen((char*)buffer); for (lfs_off_t j = 0; j < LARGESIZE; j += size) { - lfs_file_write(&lfs, &file, buffer, size) => size; + lfs_file_write(&lfs, &file, buffer, lfs_min(size, LARGESIZE-j)) + => lfs_min(size, LARGESIZE-j); } lfs_file_size(&lfs, &file) => LARGESIZE; lfs_file_close(&lfs, &file) => 0; lfs_file_open(&lfs, &file, "baldy", LFS_O_RDWR) => 0; lfs_file_size(&lfs, &file) => LARGESIZE; + /* truncate */ lfs_file_truncate(&lfs, &file, MEDIUMSIZE) => 0; lfs_file_size(&lfs, &file) => MEDIUMSIZE; + /* and write */ strcpy((char*)buffer, "bald"); size = strlen((char*)buffer); for (lfs_off_t j = 0; j < MEDIUMSIZE; j += size) { - lfs_file_write(&lfs, &file, buffer, size) => size; + lfs_file_write(&lfs, &file, buffer, lfs_min(size, MEDIUMSIZE-j)) + => lfs_min(size, MEDIUMSIZE-j); } lfs_file_size(&lfs, &file) => MEDIUMSIZE; lfs_file_close(&lfs, &file) => 0; @@ -254,7 +272,8 @@ code = ''' strcpy((char*)buffer, "comb"); size = strlen((char*)buffer); for (lfs_off_t j = 0; j < SMALLSIZE; j += size) { - lfs_file_write(&lfs, &file, buffer, size) => size; + lfs_file_write(&lfs, &file, buffer, lfs_min(size, SMALLSIZE-j)) + => lfs_min(size, SMALLSIZE-j); } lfs_file_size(&lfs, &file) => SMALLSIZE; lfs_file_close(&lfs, &file) => 0; @@ -394,7 +413,7 @@ code = ''' ''' [[case]] # noop truncate -define.MEDIUMSIZE = [32, 2048] +define.MEDIUMSIZE = [32, 33, 512, 513, 2048, 2049, 8192, 8193] code = ''' lfs_format(&lfs, &cfg) => 0; lfs_mount(&lfs, &cfg) => 0; @@ -404,10 +423,11 @@ code = ''' strcpy((char*)buffer, "hair"); size = strlen((char*)buffer); for (lfs_off_t j = 0; j < MEDIUMSIZE; j += size) { - lfs_file_write(&lfs, &file, buffer, size) => size; + lfs_file_write(&lfs, &file, buffer, lfs_min(size, MEDIUMSIZE-j)) + => lfs_min(size, MEDIUMSIZE-j); // this truncate should do nothing - lfs_file_truncate(&lfs, &file, j+size) => 0; + lfs_file_truncate(&lfs, &file, j+lfs_min(size, MEDIUMSIZE-j)) => 0; } lfs_file_size(&lfs, &file) => MEDIUMSIZE; @@ -417,8 +437,9 @@ code = ''' lfs_file_size(&lfs, &file) => MEDIUMSIZE; for (lfs_off_t j = 0; j < MEDIUMSIZE; j += size) { - lfs_file_read(&lfs, &file, buffer, size) => size; - memcmp(buffer, "hair", size) => 0; + lfs_file_read(&lfs, &file, buffer, lfs_min(size, MEDIUMSIZE-j)) + => lfs_min(size, MEDIUMSIZE-j); + memcmp(buffer, "hair", lfs_min(size, MEDIUMSIZE-j)) => 0; } lfs_file_read(&lfs, &file, buffer, size) => 0; @@ -430,44 +451,11 @@ code = ''' lfs_file_open(&lfs, &file, "baldynoop", LFS_O_RDWR) => 0; lfs_file_size(&lfs, &file) => MEDIUMSIZE; for (lfs_off_t j = 0; j < MEDIUMSIZE; j += size) { - lfs_file_read(&lfs, &file, buffer, size) => size; - memcmp(buffer, "hair", size) => 0; + lfs_file_read(&lfs, &file, buffer, lfs_min(size, MEDIUMSIZE-j)) + => lfs_min(size, MEDIUMSIZE-j); + memcmp(buffer, "hair", lfs_min(size, MEDIUMSIZE-j)) => 0; } lfs_file_read(&lfs, &file, buffer, size) => 0; lfs_file_close(&lfs, &file) => 0; lfs_unmount(&lfs) => 0; ''' - -[[case]] # barrier truncate -code = ''' - lfs_format(&lfs, &cfg) => 0; - lfs_mount(&lfs, &cfg) => 0; - lfs_file_open(&lfs, &file, "barrier", - LFS_O_RDWR | LFS_O_CREAT) => 0; - - uint8_t bad_byte = 2; - uint8_t *rb = buffer + cfg.block_size; - - /* Write a series of 1s to the first block */ - memset(buffer, 1, cfg.block_size); - lfs_file_write(&lfs, &file, buffer, - cfg.block_size) => cfg.block_size; - - /* Write a single non-one to the second block */ - lfs_file_write(&lfs, &file, &bad_byte, 1) => 1; - lfs_file_close(&lfs, &file) => 0; - - lfs_file_open(&lfs, &file, "barrier", LFS_O_RDWR) => 0; - lfs_file_truncate(&lfs, &file, cfg.block_size) => 0; - lfs_file_close(&lfs, &file) => 0; - - /* Read the first block, which should match buffer */ - lfs_file_open(&lfs, &file, "barrier", LFS_O_RDWR) => 0; - lfs_file_read(&lfs, &file, rb, - cfg.block_size) => cfg.block_size; - lfs_file_close(&lfs, &file) => 0; - - lfs_unmount(&lfs) => 0; - - memcmp(buffer, rb, cfg.block_size) => 0; -''' From 6dc18c38c1c652f319fce09393dd3b2663cb33a6 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 17 Apr 2023 13:44:35 -0500 Subject: [PATCH 3/4] Fixed block-boundary truncate issue There has been a bug in the filesystem for a while where truncating to a block boundary suffers from an off-by-one mistake that corrupts the internal representation of the CTZ skip-list. This mostly appears when the file_size == block_size, as file_size > block_size includes CTZ skip-list metadata, so the underlying block boundaries appear at slightly different offsets. --- The reason for off-by-one issue is a nuance in lfs_ctz_find that we sort of abuse to get two different behaviors. Consider the situation where this bug occurs: block 0 block 1 .--------. .--------. | abcdef |<-| {ptr0} | | ghijkl | | yzabcd | | mnopqr | | | | stuvwx | | | '--------' '--------' With these 24-byte blocks, there's an ambiguity if we wanted to point to offset 24. We could point before the block boundary, or we could point after the block boundary Before: block 0 block 1 .--------. .--------. | abcdef |<-| {ptr0} | | ghijkl | | yzabcd | | mnopqr | | | | stuvwx | | | '-------^' '--------' '-- off=24 is here After: block 0 block 1 .--------. .--------. | abcdef |<-| {ptr0} | | ghijkl | | yzabcd | | mnopqr | | ^ | | stuvwx | | | | '--------' '-|------' '-- off=24 is here When we want these two offsets depends on the context. We want the offset to be conservative if it represents a size, but eager if it is being used to prepare a block for writing. The workaround/hack is to prefer the eager offset, after the block boundary, but use `size-1` as the argument if we need the conservative offset. This finds the correct block, but is off-by-one in the calculated block-offset. Fortunately we happen to not use the block-offset in the places we need this workaround/hack. --- To get back to the bug, the wrong mode of lfs_ctz_find was used in lfs_file_truncate, leading to internal corruption of the CTZ skip-list. The correct behavior is size-1, with care to avoid underflow. Also I've tweaked the code to make it clear the calculated block-offset goes unused in these situations. Thanks to ghost, ajaybhargav, and others for reporting the issue, colin-foster-advantage for a reproducible test case, and rvanschoren, hgspbs for the initial solution. --- lfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lfs.c b/lfs.c index 26280fa8..f4a933af 100644 --- a/lfs.c +++ b/lfs.c @@ -3348,7 +3348,7 @@ static lfs_ssize_t lfs_file_flushedwrite(lfs_t *lfs, lfs_file_t *file, // find out which block we're extending from int err = lfs_ctz_find(lfs, NULL, &file->cache, file->ctz.head, file->ctz.size, - file->pos-1, &file->block, &file->off); + file->pos-1, &file->block, &(lfs_off_t){0}); if (err) { file->flags |= LFS_F_ERRED; return err; @@ -3535,7 +3535,7 @@ static int lfs_file_rawtruncate(lfs_t *lfs, lfs_file_t *file, lfs_off_t size) { // lookup new head in ctz skip list err = lfs_ctz_find(lfs, NULL, &file->cache, file->ctz.head, file->ctz.size, - size, &file->block, &file->off); + size-lfs_min(1, size), &file->block, &(lfs_off_t){0}); if (err) { return err; } From e57402c8e948bba26604a2a71dfa0804dae6cafb Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 17 Apr 2023 15:31:03 -0500 Subject: [PATCH 4/4] Added ability to revert to inline file in lfs_file_truncate Before, once converted to a CTZ skip-list, a file would remain a CTZ skip-list even if truncated back to a size that could be inlined. This was just a shortcut in implementation. And since the fix for boundary truncates needed special handling for size==0, it made sense to extend this special condition to allow reverting to inline files. --- The only case I can think of, where reverting to an inline file would be detrimental, is if it's a readonly file that you would otherwise not need to pay the metadata overhead for. But as a tradeoff, inlining the file would free up the block it was on, so it's unclear if this really is a net loss. If the truncate is followed by a write, reverting to an inline file will always be beneficial. We assume writes will change the data, so in the non-inlined case there's no way to avoid copying the underlying block. Even if we assume padding issues are solved. --- lfs.c | 65 ++++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 18 deletions(-) diff --git a/lfs.c b/lfs.c index f4a933af..0a18c484 100644 --- a/lfs.c +++ b/lfs.c @@ -3526,26 +3526,55 @@ static int lfs_file_rawtruncate(lfs_t *lfs, lfs_file_t *file, lfs_off_t size) { lfs_off_t pos = file->pos; lfs_off_t oldsize = lfs_file_rawsize(lfs, file); if (size < oldsize) { - // need to flush since directly changing metadata - int err = lfs_file_flush(lfs, file); - if (err) { - return err; - } + // revert to inline file? + if (size <= lfs_min(0x3fe, lfs_min( + lfs->cfg->cache_size, + (lfs->cfg->metadata_max ? + lfs->cfg->metadata_max : lfs->cfg->block_size) / 8))) { + // flush+seek to head + lfs_soff_t res = lfs_file_rawseek(lfs, file, 0, LFS_SEEK_SET); + if (res < 0) { + return (int)res; + } - // lookup new head in ctz skip list - err = lfs_ctz_find(lfs, NULL, &file->cache, - file->ctz.head, file->ctz.size, - size-lfs_min(1, size), &file->block, &(lfs_off_t){0}); - if (err) { - return err; - } + // read our data into rcache temporarily + lfs_cache_drop(lfs, &lfs->rcache); + res = lfs_file_flushedread(lfs, file, + lfs->rcache.buffer, size); + if (res < 0) { + return (int)res; + } - // need to set pos/block/off consistently so seeking back to - // the old position does not get confused - file->pos = size; - file->ctz.head = file->block; - file->ctz.size = size; - file->flags |= LFS_F_DIRTY | LFS_F_READING; + file->ctz.head = LFS_BLOCK_INLINE; + file->ctz.size = size; + file->flags |= LFS_F_DIRTY | LFS_F_READING | LFS_F_INLINE; + file->cache.block = file->ctz.head; + file->cache.off = 0; + file->cache.size = lfs->cfg->cache_size; + memcpy(file->cache.buffer, lfs->rcache.buffer, size); + + } else { + // need to flush since directly changing metadata + int err = lfs_file_flush(lfs, file); + if (err) { + return err; + } + + // lookup new head in ctz skip list + err = lfs_ctz_find(lfs, NULL, &file->cache, + file->ctz.head, file->ctz.size, + size-1, &file->block, &(lfs_off_t){0}); + if (err) { + return err; + } + + // need to set pos/block/off consistently so seeking back to + // the old position does not get confused + file->pos = size; + file->ctz.head = file->block; + file->ctz.size = size; + file->flags |= LFS_F_DIRTY | LFS_F_READING; + } } else if (size > oldsize) { // flush+seek if not already at end lfs_soff_t res = lfs_file_rawseek(lfs, file, 0, LFS_SEEK_END);