Fix: in case of filesystem corruption, prevent lookup soft lockup

[EricKim27]

This is a new fix that continues from pull request #88

In this code, it ensures in case of nr_blk == 0, the system does not soft lockup, and return -EUCLEAN, because nr_blk == 0 means the filesystem is corrupted.

---

Summary by cubic

Prevent lookup soft lockups on corrupted directory entries. In __file_lookup, return -EUCLEAN when dblock->files[_fi].nr_blk == 0, and mark the check unlikely() as an edge case.

^{Written for commit 3cc3392. Summary will update on new commits.}

[RoyWFHuang]

As I stated in PR88,

I believe we don't need this check. From an algorithmic perspective, dblock->files[_fi].nr_blk = 0 shouldn't appear in this function. If it does, it indicates we need to address other issues.

The nr_blk field is designed to speed up reads and writes . If a check is required for every read and write operation, it defeats the original design purpose.

Therefore, if you encounter an issue with dblock->files[_fi].nr_blk = 0, you can ask this question and describe how you triggered it.

[EricKim27]

@RoyWFHuang
I agree that nr_blk == 0 should never occur and your fix would prevent any of this from happening.

But this error I encountered caused a soft lockup due to __file_lookup looping indefinitely, and I think it is better to return -EUCLEAN rather than allowing it to loop indefinitely.

It is not that I think nr_blk being 0 is what happens on normal operation, but if a corrupted metadata reaches this path, whether it be a bug in the future or a corrupted image, the current __file_lookup may loop forever, and my proposed check makes it a filesystem error that is visible on userspace.

I do understand the concern about preserving invariants and keeping lookup without any unnecessary checks for impossible on a regular filesystem(which is why I added unlikely() to the context). I just wanted to explain why I considered the guarding mechanism to be necessary even after the root cause was remedied.

[EricKim27]

Following the discussion, I documented the investigation that led to identifying the root cause found on pull request #90

Posting it here as a supplementary documentation in case it is useful for future debugging, maintenance, or anyone curious.

link

[jserv]

Rebase latest master branch and squash commits.
Avoid semantic commits.

[RoyWFHuang]

Please squash these commits. Commits should not include changes such as code style adjustments or the removal of parentheses; please clean them up before committing, retaining only the necessary changes.

Unless these (coding style, whitespace, etc.) already exist in the current codebase.

[EricKim27]

@jserv I have squashed my commits accordingly. Thanks for the feedback.

[RoyWFHuang]

As I stated in #88,

I believe we don't need this check. From an algorithmic perspective, dblock->files[_fi].nr_blk = 0 shouldn't appear in this function. If it does, it indicates we need to address other issues.

The nr_blk field is designed to speed up reads and writes . If a check is required for every read and write operation, it defeats the original design purpose.

Therefore, to prevent such issues, we should implement appropriate preventive mechanisms—such as the CRC check feature I am currently developing, which is designed to resolve this problem at its source.

[EricKim27]

As I stated in #88,

I believe we don't need this check. From an algorithmic perspective, dblock->files[_fi].nr_blk = 0 shouldn't appear in this function. If it does, it indicates we need to address other issues.

The nr_blk field is designed to speed up reads and writes . If a check is required for every read and write operation, it defeats the original design purpose.

Therefore, to prevent such issues, we should implement appropriate preventive mechanisms—such as the CRC check feature I am currently developing, which is designed to resolve this problem at its source.

Thanks for your explanation. I agree that the matter is to be addressed elsewhere, and adding additional checks that wouldn't occur in normal operation in read/write path would introduce unnecesary overhead.

I will close this PR.

[RoyWFHuang]

Of these two commits, only the last one is valid; the first commit represents your development process.

Please only push valid versions and ignore any changes made during development, unless you have added multi features in this PR.

[EricKim27]

Of these two commits, only the last one is valid; the first commit represents your development process.

Please only push valid versions and ignore any changes made during development, unless you have added multi features in this PR.

Understood. I'll make sure to push commits to include final valid commits and squash the intermediate development commits. Thanks for the feedback.