Skip to content

fatfs: Add lower bound to block sizes #4843

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 4, 2017
Merged

fatfs: Add lower bound to block sizes #4843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f927996
fatfs: Added lower bound to block sizes
geky Apr 21, 2017
a153dc6
fatfs: Changed documentation of format to emphasize byte clusters
geky Aug 15, 2017
626256e
fatfs: Added some asserts on block device geometry
geky Aug 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions features/filesystem/fat/FATFileSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (ssize < 512) {
ssize = 512;
}

MBED_ASSERT(ssize >= _MIN_SS && ssize <= _MAX_SS);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@geky geky Aug 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@geky geky Aug 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

MBED_ASSERT(_ffs[pdrv]->get_read_size() <= _ffs[pdrv]->get_erase_size());
MBED_ASSERT(_ffs[pdrv]->get_program_size() <= _ffs[pdrv]->get_erase_size());
return ssize;
}

static DWORD disk_get_sector_count(BYTE pdrv)
{
DWORD scount = _ffs[pdrv]->size() / disk_get_sector_size(pdrv);
MBED_ASSERT(scount >= 64);
return scount;
}

DSTATUS disk_status(BYTE pdrv)
{
debug_if(FFS_DBG, "disk_status on pdrv [%d]\n", pdrv);
Expand All @@ -175,15 +195,15 @@ 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 = disk_get_sector_size(pdrv);
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 = disk_get_sector_size(pdrv);
int err = _ffs[pdrv]->erase(sector*ssize, count*ssize);
if (err) {
return RES_PARERR;
Expand Down Expand Up @@ -211,16 +231,14 @@ DRESULT disk_ioctl(BYTE pdrv, BYTE cmd, void *buff)
if (_ffs[pdrv] == NULL) {
return RES_NOTRDY;
} else {
DWORD count = _ffs[pdrv]->size() / _ffs[pdrv]->get_erase_size();
*((DWORD*)buff) = count;
*((DWORD*)buff) = disk_get_sector_count(pdrv);
return RES_OK;
}
case GET_SECTOR_SIZE:
if (_ffs[pdrv] == NULL) {
return RES_NOTRDY;
} else {
WORD size = _ffs[pdrv]->get_erase_size();
*((WORD*)buff) = size;
*((WORD*)buff) = disk_get_sector_size(pdrv);
return RES_OK;
}
case GET_BLOCK_SIZE:
Expand Down Expand Up @@ -295,7 +313,7 @@ int FATFileSystem::unmount()

/* See http://elm-chan.org/fsw/ff/en/mkfs.html for details of f_mkfs() and
* associated arguments. */
int FATFileSystem::format(BlockDevice *bd, int allocation_unit) {
int FATFileSystem::format(BlockDevice *bd, bd_size_t cluster_size) {
FATFileSystem fs;
int err = fs.mount(bd, false);
if (err) {
Expand All @@ -304,7 +322,7 @@ int FATFileSystem::format(BlockDevice *bd, int allocation_unit) {

// Logical drive number, Partitioning rule, Allocation unit size (bytes per cluster)
fs.lock();
FRESULT res = f_mkfs(fs._fsid, 1, allocation_unit);
FRESULT res = f_mkfs(fs._fsid, 1, cluster_size);
fs.unlock();
if (res != FR_OK) {
return fat_error_remap(res);
Expand Down
18 changes: 9 additions & 9 deletions features/filesystem/fat/FATFileSystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,18 @@ class FATFileSystem : public FileSystem {
* The block device to format should be mounted when this function is called.
*
* @param bd
* This is the block device that will be formated.
*
* @param allocation_unit
* This is the number of bytes per cluster size. The valid value is N
* times the sector size. N is a power of 2 from 1 to 128 for FAT
* volume and upto 16MiB for exFAT volume. If zero is given,
* the default allocation unit size is selected by the underlying
* filesystem, which depends on the volume size.
* This is the block device that will be formatted.
*
* @param cluster_size
* This is the number of bytes per cluster. A larger cluster size will decrease
* the overhead of the FAT table, but also increase the minimum file size. The
* cluster_size must be a multiple of the underlying device's allocation unit
* and is currently limited to a max of 32,768 bytes. If zero, a cluster size
* will be determined from the device's allocation unit. Defaults to zero.
*
* @return 0 on success, negative error code on failure
*/
static int format(BlockDevice *bd, int allocation_unit = 0);
static int format(BlockDevice *bd, bd_size_t cluster_size = 0);

/** Mounts a filesystem to a block device
*
Expand Down