fatfs: Add lower bound to block sizes #4843

geky · 2017-08-01T21:42:00Z

Some block devices (for example I2C EEPROM) can be erased at the byte level. These aren't really block devices, but fall under the block device API. For these devices, the fat filesystem needs to use a lower bound on the block size. In this case we used 512 bytes is used since it is already a standard.

Nodraak · 2017-08-02T07:37:06Z

Stupid question: it seems that you are dupplicating code, why don't you update get_erase_size() instead?

pan-

Reading the code, I realize that ssize in disk_write, disk_read or the sector count ioctl represent the sector size. It is a good candidate for the introduction of a new member function get_sector_size.

pan- · 2017-08-02T12:11:24Z

features/filesystem/fat/FATFileSystem.cpp

@@ -175,15 +175,23 @@ DSTATUS disk_initialize(BYTE pdrv)
 DRESULT disk_read(BYTE pdrv, BYTE *buff, DWORD sector, UINT count)
 {
    debug_if(FFS_DBG, "disk_read(sector %d, count %d) on pdrv [%d]\n", sector, count, pdrv);
-    bd_size_t ssize = _ffs[pdrv]->get_erase_size();
+    DWORD ssize = _ffs[pdrv]->get_erase_size();


Why not use get_read_size ? Is it checked anywhere that get_erase_size is a multiple of get_read_size ?

The block device API requires that the erase size is a multiple of the program size, and the program size is a multiple of the read size:
https://github.com/ARMmbed/mbed-os/blob/master/features/filesystem/bd/BlockDevice.h#L112
https://github.com/ARMmbed/mbed-os/blob/master/features/filesystem/bd/BlockDevice.h#L105

Although it does look like we don't currently have any tests that cover that

pan- · 2017-08-02T12:13:36Z

features/filesystem/fat/FATFileSystem.cpp

    int err = _ffs[pdrv]->read(buff, sector*ssize, count*ssize);
    return err ? RES_PARERR : RES_OK;
 }

 DRESULT disk_write(BYTE pdrv, const BYTE *buff, DWORD sector, UINT count)
 {
    debug_if(FFS_DBG, "disk_write(sector %d, count %d) on pdrv [%d]\n", sector, count, pdrv);
-    bd_size_t ssize = _ffs[pdrv]->get_erase_size();
+    DWORD ssize = _ffs[pdrv]->get_erase_size();


It may be relevant to use a different ssize acquired from get_program_size for the program operation or ensure the erase size is a multiple of the program size.

Hmm, I think I see your point. 512 might not be a multiple of the read/write sizes. I'm thinking this should be provided as additional parameters to the FATFileSystem. I'm thinking I'll close this for now to revise.

geky · 2017-08-02T15:25:56Z

@Nodraak, get_erase_size returns the minimum erase size of the underlying device. While the FATFileSystem doesn't support blocks less than 512 bytes, other filesystem might.

For example, the SPIFFileSystem uses an erase size of 65536 bytes:
https://github.com/ARMmbed/mbed-spiffs/blob/master/mbed_lib.json#L11

And the LittleFileSystem could go as low as 64 bytes per block:
https://github.com/ARMmbed/mbed-littlefs/blob/master/mbed_lib.json#L16

@pan-, what would get_sector_size return that is different than the existing size functions?

geky · 2017-08-02T15:37:10Z

Closing for now (see #4843 (comment))

pan- · 2017-08-02T15:42:31Z

@geky Ahhh, I'm an idiot, I didn't notice disk_read, disk_write and disk_ioctl are free functions. A static free function in FatFileSystem.cpp might be enough then:

static DWORD get_sector_size(BYTE pdrv) { 
    DWORD ssize = _ffs[pdrv]->get_erase_size()
    if (ssize < 512) {
         ssize = 512;
    }
}

geky · 2017-08-02T16:35:59Z

@pan- Ah! that is a good point, I'll add it in

JanneKiiskila · 2017-08-15T11:02:10Z

@adbridge @sg- - requesting this to be patched to next Mbed OS 5.5.x release, ASAP.

Some block devices (for example I2C EEPROM) can be erased at the byte level. These aren't really block devices, but fall under the block device API. For these devices, the fat filesystem needs to use a lower bound on the block size. In this case we used 512 bytes is used since it is already a standard.

adbridge · 2017-08-16T11:05:08Z

@geky What is the status of this? Janne is asking for this patch go into the next release (will be 5.5.6 now) but you closed the PR .... ??

geky · 2017-08-16T15:19:14Z

@JanneKiiskila why the priority? I understand this is needed for #4899, but is there a seperate issue?

@adbridge, this needed work and no one needed it, so I closed it to work on other things. I can reopen, although I still need to update it based on @pan-'s points.

JanneKiiskila · 2017-08-16T15:21:24Z

All SD-card corruption issues are a priority, they cause a massive havoc in CI if there's any potential for issues with corruption.

geky · 2017-08-16T15:25:06Z

Fair enough 👍

geky · 2017-08-16T15:26:50Z

Although I do believe he is modifying the FATFileSystem to let the problematic block size through. By default the FATFileSystem should assert if the block size is too small.

LemonBoy · 2017-08-16T17:40:20Z

Not really, there seems to be no check to make sure the relations between the {read,erase,program}_size are respected, in fact with a vanilla driver I get a nicely corrupted filesystem every time because of the erase_size incongruence (erase_size is 128 while the block size is 512).

geky · 2017-08-16T19:27:00Z

Ah, my bad, it looks like this check is only performed during format, not mount:
https://github.com/ARMmbed/mbed-os/blob/master/features/filesystem/fat/ChaN/ff.cpp#L4134

geky · 2017-08-16T20:38:03Z

Is open for review now

LemonBoy · 2017-08-18T09:26:34Z

features/filesystem/fat/FATFileSystem.cpp

+        ssize = 512;
+    }
+
+    MBED_ASSERT(ssize >= _MIN_SS && ssize <= _MAX_SS);


I think the assertions should be moved in the constructor, something like:

MBED_ASSERT(bd->get_program_size() == bd->get_read_size());

The erase size should then be checked by is_valid_erase so the optimization kicks in when it's possible to do so and should return the real size instead of returning _block_size.

The block device needs to be initialized before get_erase_size and friends can be called reliably. Currently the init is called from the chan layer during mount, but mount may also write to the device.

We could bring up the block device to check things before mounting, but since these are asserts anyways it's easier to just put them in the chan-disk layer.

The assert for is_valid_erase should live in the block device driver.

I did add some other checks on program/read size though 👍

Alright, I forgot the fields are populated after the init(). I'd still move the assertions in the init codepath since those don't really change and the read/write paths should be as fast as possible.

The read/write paths are already bounded by the speed of the underlying block devices. In the read/program/erase funtions you will find similar asserts. If you are looking for speed can't you just compile in release mode? Release mode will disable these asserts, but they will still have been validated while developing.

LemonBoy · 2017-08-18T16:33:32Z

features/filesystem/fat/FATFileSystem.cpp

@@ -160,6 +160,26 @@ void ff_memfree(void *p)
 }

 // Implementation of diskio functions (see ChaN/diskio.h)
+static WORD disk_get_sector_size(BYTE pdrv)
+{
+    WORD ssize = _ffs[pdrv]->get_erase_size();


It probably makes sense to use get_read_size here (the invariant here is that read size == wirte size, right?) since get_erase_size() can assume whatever value the HW reports and the block driver will act accordingly.
The rounding to 512 isn't needed anymore since we found the erase size for that particular card isn't 128 but 64K, I think.

For most of the other block devices an erase is required before programming (flash in particular). The SD card is the exception. If we program less than an erase block on NOR flash, the data will just become corrupted.

For the block device API, the erase function makes a block available for programming. The get_erase_size should return the smallest block size that can be rewritten. Since the FAT fs doesn't schedule erases seperately, it needs to erase a block during a write. So the sector size here is bounded to the underlying device's get_erase_size.

The rounding to 512 is still useful for I2C EEPROM chips, which actually have get_erase_size of 1 byte.

0xc0170 · 2017-08-21T14:34:11Z

@LemonBoy Happy with all the responses and the code as it is now?

LemonBoy · 2017-08-22T07:58:13Z

The assertions are enough to not to accidentally corrupt the filesystem when the values are wrong and that's fine by me.
Further changes to the erase() logic were proposed in sd-driver to work with cards with big erase sectors but that's something for another PR.

adbridge · 2017-08-24T11:04:44Z

/morph test-nightly

mbed-bot · 2017-08-24T11:48:28Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1108

Build failed!

studavekar · 2017-08-28T20:35:52Z

/morph test-nightly

mbed-bot · 2017-08-29T15:50:48Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1133

All builds and test passed!

geky · 2017-08-29T16:08:51Z

what's going on with continuous-integration/jenkins/pr-head?

theotherjimmy · 2017-08-29T18:40:58Z

@tommikas What's happening with the Jenkins over there? Can we get this PR tested?

tommikas · 2017-08-30T08:32:23Z

The build had passed but for some reason the notification hadn't reached github. Triggered a rebuild and it seems to be ok now.

geky · 2017-08-30T16:12:01Z

thanks! LGTM!

pan- reviewed Aug 2, 2017

View reviewed changes

geky closed this Aug 2, 2017

geky mentioned this pull request Aug 14, 2017

FAT filesystem causes constant corruption #4899

Closed

geky reopened this Aug 16, 2017

geky force-pushed the fat-min-block branch from fa35108 to fa851db Compare August 16, 2017 20:36

geky added the needs: review label Aug 16, 2017

geky force-pushed the fat-min-block branch from ec7bea4 to e37fa04 Compare August 17, 2017 18:36

LemonBoy reviewed Aug 18, 2017

View reviewed changes

geky force-pushed the fat-min-block branch 2 times, most recently from 26b018f to ca47d4f Compare August 18, 2017 16:04

LemonBoy reviewed Aug 18, 2017

View reviewed changes

0xc0170 approved these changes Aug 21, 2017

View reviewed changes

theotherjimmy changed the title ~~fatfs: Added lower bound to block sizes~~ fatfs: Add lower bound to block sizes Aug 21, 2017

adbridge added needs: CI and removed needs: review labels Aug 24, 2017

geky mentioned this pull request Aug 28, 2017

bd: Tweaked block device API to fit SD cards and FTLs better #4983

Merged

geky added 2 commits August 28, 2017 14:54

fatfs: Changed documentation of format to emphasize byte clusters

a153dc6

fatfs: Added some asserts on block device geometry

626256e

geky force-pushed the fat-min-block branch from ca47d4f to 626256e Compare August 28, 2017 19:54

geky added ready for merge and removed needs: CI labels Aug 30, 2017

0xc0170 added the release-version: 5.6.0 label Sep 4, 2017

0xc0170 merged commit 3f347ed into ARMmbed:master Sep 4, 2017

0xc0170 removed the ready for merge label Sep 4, 2017

fatfs: Add lower bound to block sizes #4843

fatfs: Add lower bound to block sizes #4843

Uh oh!

Conversation

geky commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nodraak commented Aug 2, 2017

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

pan- Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geky commented Aug 2, 2017

Uh oh!

pan- commented Aug 2, 2017

Uh oh!

geky commented Aug 2, 2017

Uh oh!

JanneKiiskila commented Aug 15, 2017

Uh oh!

adbridge commented Aug 16, 2017

Uh oh!

geky commented Aug 16, 2017

Uh oh!

JanneKiiskila commented Aug 16, 2017

Uh oh!

geky commented Aug 16, 2017

Uh oh!

geky commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LemonBoy commented Aug 16, 2017

Uh oh!

geky commented Aug 16, 2017

Uh oh!

geky commented Aug 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Aug 21, 2017

Uh oh!

LemonBoy commented Aug 22, 2017

Uh oh!

adbridge commented Aug 24, 2017

Uh oh!

mbed-bot commented Aug 24, 2017

Output

Uh oh!

studavekar commented Aug 28, 2017

Uh oh!

mbed-bot commented Aug 29, 2017

geky commented Aug 1, 2017 •

edited

Loading

pan- Aug 2, 2017 •

edited

Loading

pan- Aug 2, 2017 •

edited

Loading

geky commented Aug 2, 2017 •

edited

Loading

geky commented Aug 16, 2017 •

edited

Loading

geky Aug 18, 2017 •

edited

Loading

geky Aug 18, 2017 •

edited

Loading