Skip to content

os/fs: Add error handling code when finding a free physical sector. - #3600

Closed
chanijjani wants to merge 1 commit into
Samsung:masterfrom
chanijjani:fs_fix_smartfs_freecount
Closed

chanijjani wants to merge 1 commit into
Samsung:masterfrom
chanijjani:fs_fix_smartfs_freecount

Conversation

@chanijjani

@chanijjani chanijjani commented Jul 25, 2019 •

Copy link
Copy Markdown
Contributor
  • The freecount value of an erasable block mismatches with
    actual block status when MTD_ERASE fails.
    As a result, no physical sector is allocated and file system errors
    occur at various moments.
    Error handling code is added to avoid this situation.
  • Useful debug messages are added also.

@seinfra

seinfra commented Jul 25, 2019

Copy link
Copy Markdown

Target : [c8dec000062c5b2eabf9d6aa782edee820d0fd5f] - Code Rule Check OK.

@seinfra

seinfra commented Jul 25, 2019

Copy link
Copy Markdown

Target : [c8dec000062c5b2eabf9d6aa782edee820d0fd5f] - Code Rule Check (C++) OK.

Comment thread os/fs/driver/mtd/smart.c Outdated

@Taejun-Kwon Taejun-Kwon Jul 26, 2019 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, you have to clean up this & current code
As you know, flow is below.

  1. Check all erase block and check count of free-sector of each block.
  2. If there's free sector in specific block, check wear level.
    2-1. if wearlevel is acceptable, checked block becomes a target block
  3. If there's no free sector after flow Merge Posix Cancellation Point #2, relocate block & retry Create README.md #1~Merge Posix Cancellation Point #2
  4. Now we found target block, then check all sector header in block.
  5. If it found sector which has clean header, then verify that contents area's data.
    5-1 if any of contents is flipped, update erase count and something, then retry it
  6. If any of sector is all cleaned, then allocate it, otherwise it is program bug.

So your approach is to prevent #6 because of invalid header, I think, am I right?
Then this is not efficient code, because we already check header's status in #6.
I think you can modify something in below.

		if ((UINT8TOUINT16(header.logicalsector) == 0xFFFF) &&
#if SMART_STATUS_VERSION == 1
			((header.seq == 0xFF) && (header.crc8 == 0xFF)) &&
#else
			(header.seq == CONFIG_SMARTFS_ERASEDSTATE) &&
#endif
			(!(SECTOR_IS_COMMITTED(header)))) {

This can be modify to HEADER_IS_CLEAN but HEADER_IS_CLEAN doesn't care CONFIG_SMARTFS_ERASEDSTATE, you have to modify it too.

if (HEADER_IS_CLEAN(header)) {
    check_bitflip_in_contents;
} else {
    compare_with_sMap;
    if (sMap is valid but header is not clean) {
          release_sector;
          update_freecount & release_count;
   }
   ...
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if allocation falied because of flipped bit, retry is necessary for both of if-else in my comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Taejun-Kwon Thanks for your well-organized summary.

I uploaded this PR since a situation that the 'Program bug!' message in line 3605 happened during easy-setup tests on NXP boards.

This is due to the phenomenon that the freecount value of an erasable block(=EB) selected in line 3433 has no clean sectors in actual so that no physical sectors in the EB passed "if" statement in line 3563. As a result, the execution reaches at 3605 since no physical sector is allocated even though an EB is chosen.

Now I'm trying to find the root cause and fix it in a more efficient way.

@chanijjani
chanijjani force-pushed the fs_fix_smartfs_freecount branch from c8dec00 to ec87add Compare August 13, 2019 12:39
@seinfra

seinfra commented Aug 13, 2019

Copy link
Copy Markdown

Target : [ec87addbe79a2ff1b488ae11d608b72da1779021] - Code Rule Check (C++) OK.

@seinfra

seinfra commented Aug 13, 2019

Copy link
Copy Markdown

Target : [ec87addbe79a2ff1b488ae11d608b72da1779021] - Code Rule Check OK.

@chanijjani
chanijjani force-pushed the fs_fix_smartfs_freecount branch from ec87add to 985761b Compare August 13, 2019 12:46
@seinfra

seinfra commented Aug 13, 2019

Copy link
Copy Markdown

Target : [985761b45cec8a8503e877716a851672f69d6e60] - Code Rule Check (C++) OK.

@seinfra

seinfra commented Aug 13, 2019

Copy link
Copy Markdown

Target : [985761b45cec8a8503e877716a851672f69d6e60] - Code Rule Check OK.

@chanijjani
chanijjani force-pushed the fs_fix_smartfs_freecount branch from 985761b to 04cbef6 Compare August 13, 2019 12:48
@seinfra

seinfra commented Aug 13, 2019

Copy link
Copy Markdown

Target : [04cbef6d70776f48158bdbe599bf296da3aea7d6] - Code Rule Check (C++) OK.

@seinfra

seinfra commented Aug 13, 2019

Copy link
Copy Markdown

Target : [04cbef6d70776f48158bdbe599bf296da3aea7d6] - Code Rule Check OK.

@seinfra

seinfra commented Sep 6, 2019

Copy link
Copy Markdown

Target : [cde39e5b96118685e61c02844c41f99bd653165d] - Code Rule Check (C++) OK.

@seinfra

seinfra commented Sep 6, 2019

Copy link
Copy Markdown

Target : [cde39e5b96118685e61c02844c41f99bd653165d] - Code Rule Check OK.

@chanijjani
chanijjani force-pushed the fs_fix_smartfs_freecount branch from cde39e5 to bf4904f Compare September 6, 2019 05:48
- The freecount value of an erasable block mismatches with
  actual block status when MTD_ERASE fails.
  As a result, no physical sector is allocated and file system errors
  occur at various moments.
  Error handling code is added to avoid this situation.
- Useful debug messages are added also.
@seinfra

seinfra commented Sep 6, 2019

Copy link
Copy Markdown

Target : [bf4904f] - Code Rule Check (C++) OK.

@seinfra

seinfra commented Sep 6, 2019

Copy link
Copy Markdown

Target : [bf4904f] - Code Rule Check OK.

@chanijjani chanijjani closed this Sep 6, 2019
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.

3 participants