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

fatfs: Add lower bound to block sizes #4843

Merged
merged 3 commits into from Sep 4, 2017

Conversation

Projects
None yet
@geky
Member

geky commented Aug 1, 2017

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

This comment has been minimized.

Contributor

Nodraak commented Aug 2, 2017

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.

features/filesystem/fat/FATFileSystem.cpp Outdated
@@ -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();

This comment has been minimized.

@pan-

pan- Aug 2, 2017

Member

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

This comment has been minimized.

@geky

geky Aug 2, 2017

Member

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

features/filesystem/fat/FATFileSystem.cpp Outdated
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();

This comment has been minimized.

@pan-

pan- Aug 2, 2017

Member

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.

This comment has been minimized.

@geky

geky Aug 2, 2017

Member

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

This comment has been minimized.

Member

geky commented Aug 2, 2017

@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

This comment has been minimized.

Member

geky commented Aug 2, 2017

Closing for now (see #4843 (comment))

@geky geky closed this Aug 2, 2017

@pan-

This comment has been minimized.

Member

pan- commented Aug 2, 2017

@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

This comment has been minimized.

Member

geky commented Aug 2, 2017

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

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Aug 15, 2017

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

fatfs: Added lower bound to block sizes
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

This comment has been minimized.

Contributor

adbridge commented Aug 16, 2017

@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

This comment has been minimized.

Member

geky commented Aug 16, 2017

@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.

@geky geky reopened this Aug 16, 2017

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Aug 16, 2017

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

This comment has been minimized.

Member

geky commented Aug 16, 2017

Fair enough 👍

@geky

This comment has been minimized.

Member

geky commented Aug 16, 2017

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

This comment has been minimized.

Contributor

LemonBoy commented Aug 16, 2017

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

This comment has been minimized.

Member

geky commented Aug 16, 2017

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 geky force-pushed the geky:fat-min-block branch Aug 16, 2017

@geky geky added the needs: review label Aug 16, 2017

@geky

This comment has been minimized.

Member

geky commented Aug 16, 2017

Is open for review now

@geky geky force-pushed the geky:fat-min-block branch Aug 17, 2017

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

This comment has been minimized.

@LemonBoy

LemonBoy Aug 18, 2017

Contributor

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.

This comment has been minimized.

@geky

geky Aug 18, 2017

Member

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 👍

This comment has been minimized.

@LemonBoy

LemonBoy Aug 18, 2017

Contributor

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.

This comment has been minimized.

@geky

geky Aug 18, 2017

Member

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.

@geky geky force-pushed the geky:fat-min-block branch 2 times, most recently Aug 18, 2017

@@ -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();

This comment has been minimized.

@LemonBoy

LemonBoy Aug 18, 2017

Contributor

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.

This comment has been minimized.

@geky

geky Aug 18, 2017

Member

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

This comment has been minimized.

Member

0xc0170 commented Aug 21, 2017

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

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

@LemonBoy

This comment has been minimized.

Contributor

LemonBoy commented Aug 22, 2017

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

This comment has been minimized.

Contributor

adbridge commented Aug 24, 2017

/morph test-nightly

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

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 24, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1108

Build failed!

@geky geky force-pushed the geky:fat-min-block branch to 626256e Aug 28, 2017

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Aug 28, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 29, 2017

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

This comment has been minimized.

Member

geky commented Aug 29, 2017

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 29, 2017

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

@tommikas

This comment has been minimized.

Contributor

tommikas commented Aug 30, 2017

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

This comment has been minimized.

Member

geky commented Aug 30, 2017

thanks! LGTM!

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

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

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test-nightly Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment