Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect cache reuse when seeking from end-of-block #1058

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

geky
Copy link
Member

@geky geky commented Dec 19, 2024

In v2.5 (#632 specifically), we introduced an optimization to avoid rereading data when seeking inside the file cache. Unfortunately this used a slightly wrong condition to check if the cache was "live", which meant seeks from end-of-blocks could end up with invalid caches and wrong data. Not great.

The problem is the nuance of when a file's cache is "live":

  1. The file is marked as LFS_F_READING or LFS_F_WRITING.

    But we can't reuse the cache when writing, so we only care about LFS_F_READING.

  2. file->off != lfs->cfg->block_size (end-of-block).

    This is an optimization to avoid eagerly reading blocks we may not actually care about.

We weren't checking for the end-of-block case, which meant if you seeked from the end of a block to a seemingly valid location in the file cache, you could end up with an invalid cache.

Note that end-of-block may not be powers-of-two due to CTZ skip-list pointers.


The fix is to check for the end-of-block case in lfs_file_seek. Note this now matches the need-new-block logic in lfs_file_flushedread.

This logic change may also make lfs_file_seek call lfs_file_flush more often, but only in cases where lfs_file_flush is a noop.

I've also extended the test_seek tests to cover a few more boundary-read cases and prevent a regression in the future.

Found by @wjl and @lrodorigo
Related: #1057, #728

@geky
Copy link
Member Author

geky commented Dec 19, 2024

Clang:

tests/test_seek.toml:234:65: error: adding 'unsigned long' to a string does not append to the string [-Werror,-Wstring-plus-int]
        __PRETTY_ASSERT_MEM_EQ(buffer, "kittycatcatkittycatcat" + ((off+1) % strlen("kittycatcat")), size);
                                       ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I guess this highlights how difficult it is to write warnings that are both useful and not obnoxious.

In v2.5, we introduced an optimization to avoid rereading data when
seeking inside the file cache. Unfortunately this used a slightly
wrong condition to check if the cache was "live", which meant seeks from
end-of-blocks could end up with invalid caches and wrong data. Not
great.

The problem is the nuance of when a file's cache is "live":

1. The file is marked as LFS_F_READING or LFS_F_WRITING.

   But we can't reuse the cache when writing, so we only care about
   LFS_F_READING.

2. file->off != lfs->cfg->block_size (end-of-block).

   This is an optimization to avoid eagerly reading blocks we may not
   actually care about.

We weren't checking for the end-of-block case, which meant if you seeked
_from_ the end of a block to a seemingly valid location in the file
cache, you could end up with an invalid cache.

Note that end-of-block may not be powers-of-two due to CTZ skip-list
pointers.

---

The fix is to check for the end-of-block case in lfs_file_seek. Note
this now matches the need-new-block logic in lfs_file_flushedread.

This logic change may also make lfs_file_seek call lfs_file_flush more
often, but only in cases where lfs_file_flush is a noop.

I've also extended the test_seek tests to cover a few more boundary-read
cases and prevent a regression in the future.

Found by wjl and lrodorigo
@geky geky force-pushed the fix-seek-eob-cache branch from 537dc24 to 366100b Compare December 19, 2024 08:40
@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 17136 B (+0.0%), Stack: 1440 B (+0.0%), Structs: 812 B (+0.0%)
Code Stack Structs Coverage
Default 17136 B (+0.0%) 1440 B (+0.0%) 812 B (+0.0%) Lines 2399/2571 lines (-0.0%)
Readonly 6262 B (+0.2%) 448 B (+0.0%) 812 B (+0.0%) Branches 1285/1618 branches (+0.0%)
Threadsafe 17984 B (+0.0%) 1440 B (+0.0%) 820 B (+0.0%) Benchmarks
Multiversion 17208 B (+0.0%) 1440 B (+0.0%) 816 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18824 B (+0.0%) 1736 B (+0.0%) 816 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17900 B (+0.0%) 1432 B (+0.0%) 812 B (+0.0%) Erased 1568888832 B (+0.0%)

@geky geky changed the title Fixed incorrect cache reuse when seeking from end-of-block Fix incorrect cache reuse when seeking from end-of-block Dec 20, 2024
@geky geky merged commit 0494ce7 into devel Dec 20, 2024
125 checks passed
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Dec 25, 2024
2.10.1 contains a fix for a long-standing wrong-data issue.
littlefs-project/littlefs#1058
acassis pushed a commit to apache/nuttx that referenced this pull request Dec 25, 2024
2.10.1 contains a fix for a long-standing wrong-data issue.
littlefs-project/littlefs#1058
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants