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

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

Merged
merged 3 commits into from Sep 21, 2017

Conversation

Projects
None yet
6 participants
@geky
Member

geky commented Aug 28, 2017

The block device API is currently tied very closely to flash devices. This is because flash devices are our primary targets, but it does make porting to other non-flash block devices a bit more confusing than is necessary (this includes SD cards, FTLs, and more ridiculous spinny disks).

These are a few tweaks from offline discussions with @deepikabhavnani that should improve things:

  1. Provide default for erase function
    This means the function can simply be omitted from the SD card repo and the block device should behave as expected

  2. Add trim function to provide hints for unused blocks
    See https://en.wikipedia.org/wiki/Trim_(computing) for prior art
    This allows a filesystem to hint when blocks are no longer in use, and can allow much better wear leveling performance when an FTL is present, such as in SD cards.

relevant range 626256e...geky:bd-trim-erase
dependent on #4843
related ARMmbed/sd-driver#55
cc @deepikabhavnani, @sg-

@geky geky force-pushed the geky:bd-trim-erase branch Aug 28, 2017

@geky

This comment has been minimized.

Member

geky commented Aug 28, 2017

Adopted trim function in the FAT filesystem, however that makes this pr now dependent on #4843
relevant range 626256e...geky:bd-trim-erase

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

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 29, 2017

Contributor

get_erase_size() / get_program_size() / disk_get_sector_size() are bit confusing, please verify my understanding below:

  1. program size - Can be 1 to 512 bytes (based on device type). Will this be > 512?
  2. Erase size - Set as program size (Can we set it as sector size instead) = 512 ?
  3. disk_get_sector_size - Here we are forcing it to be 512, why can't we simply set it as 512? Sector size is for file system and not dependent on device.

This comment has been minimized.

@geky

geky Aug 30, 2017

Member

program size - Can be 1 to 512 bytes (based on device type). Will this be > 512?

It could be > 512 though I don't know of any devices currently that go that high.

Erase size - Set as program size (Can we set it as sector size instead) = 512 ?

Are you asking about the get_erase_size function inside the driver? The driver doesn't have a concept of sector size. These functions just report the physical limitations of the device. It's up to the filesystem on top to choose a sector size.

disk_get_sector_size - Here we are forcing it to be 512, why can't we simply set it as 512? Sector size is for file system and not dependent on device.

nor flash specificially needs a sector size of at least 4096 bytes, since it can't erase at <4096 bytes. However, the larger sector size comes with a ram cost, so it's better to use ~512 byte sectors where possible.

} else {
DWORD *sectors = (DWORD*)buff;
DWORD ssize = disk_get_sector_size(pdrv);
int err = _ffs[pdrv]->trim(sectors[0]*ssize, (sectors[1]-sectors[0]+1)*ssize);

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 29, 2017

Contributor

Arguments to erase function were start sector, and total size to erase. Please clarify what are the arguments of trim? sectors[0] / sectors[1] what do they contain?

This comment has been minimized.

@geky

geky Aug 30, 2017

Member

Ah sure: http://elm-chan.org/fsw/ff/doc/dioctl.html

The sector block is specified by a DWORD array {<start sector>, <end sector>}

So sectors[0] is the start sector, and sectors[1] is the end sector (inclusive) of the trim operation.

features/filesystem/fat/FATFileSystem.h Outdated
* 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 nearest power-of-2 of the underlying
* device's erase_size. If zero, a cluster size will be determined from the

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 29, 2017

Contributor

Device erase_size can be as less as single block. Cluster size (nearest power of 2 of erase_size 512bytes) will be 1024 bytes and not good value to set.

Cluster size is logical boundary in files system.

AU size on SD cards is not logical boundary, but physical boundary in user area of card, and formatting card with correct AU size gives optimal performance. Setting cluster size equivalent to AU size is recommended for SD cards.

This comment has been minimized.

@geky

geky Aug 30, 2017

Member

Ah, I changed this over here
https://github.com/ARMmbed/mbed-os/pull/4843/files#diff-d2c0be70be00c3c1688a6e00f622217bR54

I just haven't rebased this pr yet

@geky geky force-pushed the geky:bd-trim-erase branch Aug 30, 2017

@deepikabhavnani

LGTM 👍

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 5, 2017

The dependency was merged, shall this be rebased now?

@adbridge adbridge added the needs: work label Sep 5, 2017

geky added some commits Aug 28, 2017

bd: Changed BlockDevice erase to default as no-op
This should help with confusion around devices that don't have
a physical erase operation, such as SD cards and spinny disks.

@geky geky force-pushed the geky:bd-trim-erase branch to ee88097 Sep 5, 2017

@geky

This comment has been minimized.

Member

geky commented Sep 5, 2017

rebased 👍
/morph test-nightly

@geky geky added needs: CI and removed needs: work labels Sep 5, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 6, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1224

Test failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 6, 2017

I assume the failure is not related to this changeset. Please be aware of the timing limits now for testing as we are close to the code freeze, nightly on this only if it is required to get in 5.6, let us know.

@mazimkhan uvisor CI did not report the status?

@geky

This comment has been minimized.

Member

geky commented Sep 6, 2017

Ah sorry. Can we add a 5.6.1 label?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 20, 2017

@mazimkhan uvisor CI did not report the status?

Bump

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 20, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 21, 2017

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1348

All builds and test passed!

@adbridge adbridge closed this Sep 21, 2017

@adbridge adbridge reopened this Sep 21, 2017

@adbridge adbridge added needs: CI and removed needs: CI labels Sep 21, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 21, 2017

retest uvisor

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Sep 21, 2017

@theotherjimmy theotherjimmy merged commit bfc1c5a into ARMmbed:master Sep 21, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment