| Message ID | 1443765973-5897-5-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
On 10/02/2015 12:06 AM, Stephen Warren wrote: > Enhance f_read() to find the maximum contiguous set of clusters to read, > and read it all at once (which is fast) rather one by one (which is > slow). Hmm. I had hoped that the author of ff.c would accept this patch upstream, so we could pick up a later upstream version that included this patch. However, it seems the author of ff.c has a policy of not accepting outside contributions: http://elm-chan.org/fsw/ff/bd/?show=2472 (That's a link to the author's reply to my patch, on the forum system associated with his/her SW) I wonder how much of a liability incorporating ff.c into U-Boot will be, if we can't ever get any fixes merged upstream. Perhaps we just fork it, although I had hoped we'd be able to keep picking up new versions.
On Tue, Oct 20, 2015 at 12:57:32PM -0600, Stephen Warren wrote: > On 10/02/2015 12:06 AM, Stephen Warren wrote: > >Enhance f_read() to find the maximum contiguous set of clusters to read, > >and read it all at once (which is fast) rather one by one (which is > >slow). > > Hmm. I had hoped that the author of ff.c would accept this patch > upstream, so we could pick up a later upstream version that included > this patch. However, it seems the author of ff.c has a policy of not > accepting outside contributions: > > http://elm-chan.org/fsw/ff/bd/?show=2472 > (That's a link to the author's reply to my patch, on the forum > system associated with his/her SW) The bit about the license is at http://elm-chan.org/fsw/ff/en/appnote.html#license > I wonder how much of a liability incorporating ff.c into U-Boot will > be, if we can't ever get any fixes merged upstream. Perhaps we just > fork it, although I had hoped we'd be able to keep picking up new > versions. Arg, that really does take away one of the potential nice features. I guess, sadly, at this point I'd rather stick with the version we have unless you want to deal with re-syncing their releases but still effectively doing a fork (so that we can also make use of caches which I think you said before you thought might be part of the performance problem. Or we take a look at borrowing the kernel's code, similar to how we leverage UBIFS today. Regardless, thanks for the time you've already put in on this!
On 10/20/2015 01:10 PM, Tom Rini wrote: > On Tue, Oct 20, 2015 at 12:57:32PM -0600, Stephen Warren wrote: >> On 10/02/2015 12:06 AM, Stephen Warren wrote: >>> Enhance f_read() to find the maximum contiguous set of clusters to read, >>> and read it all at once (which is fast) rather one by one (which is >>> slow). >> >> Hmm. I had hoped that the author of ff.c would accept this patch >> upstream, so we could pick up a later upstream version that included >> this patch. However, it seems the author of ff.c has a policy of not >> accepting outside contributions: >> >> http://elm-chan.org/fsw/ff/bd/?show=2472 >> (That's a link to the author's reply to my patch, on the forum >> system associated with his/her SW) > > The bit about the license is at > http://elm-chan.org/fsw/ff/en/appnote.html#license > >> I wonder how much of a liability incorporating ff.c into U-Boot will >> be, if we can't ever get any fixes merged upstream. Perhaps we just >> fork it, although I had hoped we'd be able to keep picking up new >> versions. > > Arg, that really does take away one of the potential nice features. I > guess, sadly, at this point I'd rather stick with the version we have > unless you want to deal with re-syncing their releases but still > effectively doing a fork (so that we can also make use of caches which I > think you said before you thought might be part of the performance > problem. FWIW, the caching (of FAT bitmap, not CPU memory) turned out not to be an issue at all; it was just my wild conjecture before I'd investigated the read performance issues.
On 10/20/15 1:10 PM, Tom Rini wrote: > On Tue, Oct 20, 2015 at 12:57:32PM -0600, Stephen Warren wrote: >> On 10/02/2015 12:06 AM, Stephen Warren wrote: >>> Enhance f_read() to find the maximum contiguous set of clusters to read, >>> and read it all at once (which is fast) rather one by one (which is >>> slow). >> >> Hmm. I had hoped that the author of ff.c would accept this patch >> upstream, so we could pick up a later upstream version that included >> this patch. However, it seems the author of ff.c has a policy of not >> accepting outside contributions: >> >> http://elm-chan.org/fsw/ff/bd/?show=2472 >> (That's a link to the author's reply to my patch, on the forum >> system associated with his/her SW) > > The bit about the license is at > http://elm-chan.org/fsw/ff/en/appnote.html#license > >> I wonder how much of a liability incorporating ff.c into U-Boot will >> be, if we can't ever get any fixes merged upstream. Perhaps we just >> fork it, although I had hoped we'd be able to keep picking up new >> versions. > > Arg, that really does take away one of the potential nice features. I > guess, sadly, at this point I'd rather stick with the version we have > unless you want to deal with re-syncing their releases but still > effectively doing a fork (so that we can also make use of caches which I > think you said before you thought might be part of the performance > problem. > > Or we take a look at borrowing the kernel's code, similar to how we > leverage UBIFS today. > > Regardless, thanks for the time you've already put in on this! If anyone is still interested in replacing the FAT code, I just noticed that FreeRTOS has been relicensed to MIT along with some/all of its extensions, including the FAT code: https://www.freertos.org/FreeRTOS-Plus/FreeRTOS_Plus_FAT/index.html I'd assume that code is reasonably well maintained and embedded-suitable. I have not investigated: - If it has the same "no contributions" issue that FF had. - If it's modular enough to plug into other hosts besides FreeRTOS. - What its performance is like.
diff --git a/fs/fat/ff.c b/fs/fat/ff.c index 478daa7d6b0b..0df5a9ddcf7c 100644 --- a/fs/fat/ff.c +++ b/fs/fat/ff.c @@ -2593,7 +2593,7 @@ FRESULT f_read ( ) { FRESULT res; - DWORD clst, sect, remain; + DWORD clst, sect, remain, cfptr, cclst, maxclust; UINT rcnt, cc; BYTE csect, *rbuff = (BYTE*)buff; @@ -2614,27 +2614,46 @@ FRESULT f_read ( if ((fp->fptr % SS(fp->fs)) == 0) { /* On the sector boundary? */ csect = (BYTE)(fp->fptr / SS(fp->fs) & (fp->fs->csize - 1)); /* Sector offset in the cluster */ if (!csect) { /* On the cluster boundary? */ - if (fp->fptr == 0) { /* On the top of the file? */ - clst = fp->sclust; /* Follow from the origin */ - } else { /* Middle or end of the file */ + maxclust = 0; + cfptr = fp->fptr; + cclst = fp->clust; + for (;;) { + if (cfptr == 0) { /* On the top of the file? */ + clst = fp->sclust; /* Follow from the origin */ + } else { /* Middle or end of the file */ #if _USE_FASTSEEK - if (fp->cltbl) - clst = clmt_clust(fp, fp->fptr); /* Get cluster# from the CLMT */ - else + if (fp->cltbl) + clst = clmt_clust(fp, cfptr); /* Get cluster# from the CLMT */ + else #endif - clst = get_fat(fp->fs, fp->clust); /* Follow cluster chain on the FAT */ + clst = get_fat(fp->fs, cclst); /* Follow cluster chain on the FAT */ + } + if (clst < 2) ABORT(fp->fs, FR_INT_ERR); + if (clst == 0xFFFFFFFF) ABORT(fp->fs, FR_DISK_ERR); + if (!maxclust) { + fp->clust = clst; /* Update current cluster */ + } else { + if (clst != (cclst + 1)) + break; + } + maxclust++; + if ((maxclust * fp->fs->csize * SS(fp->fs)) >= btr) + break; + cclst = clst; + cfptr += fp->fs->csize * SS(fp->fs); } - if (clst < 2) ABORT(fp->fs, FR_INT_ERR); - if (clst == 0xFFFFFFFF) ABORT(fp->fs, FR_DISK_ERR); - fp->clust = clst; /* Update current cluster */ + } else { + maxclust = 1; } sect = clust2sect(fp->fs, fp->clust); /* Get current sector */ + if (maxclust > 1) + fp->clust += (maxclust - 1); if (!sect) ABORT(fp->fs, FR_INT_ERR); sect += csect; cc = btr / SS(fp->fs); /* When remaining bytes >= sector size, */ if (cc) { /* Read maximum contiguous sectors directly */ - if (csect + cc > fp->fs->csize) /* Clip at cluster boundary */ - cc = fp->fs->csize - csect; + if (csect + cc > (fp->fs->csize * maxclust)) /* Clip at cluster boundary */ + cc = (fp->fs->csize * maxclust) - csect; if (disk_read(fp->fs->drv, rbuff, sect, cc) != RES_OK) ABORT(fp->fs, FR_DISK_ERR); #if !_FS_READONLY && _FS_MINIMIZE <= 2 /* Replace one of the read sectors with cached data if it contains a dirty sector */
Enhance f_read() to find the maximum contiguous set of clusters to read, and read it all at once (which is fast) rather one by one (which is slow). Signed-off-by: Stephen Warren <swarren@wwwdotorg.org> --- V3: New patch. --- fs/fat/ff.c | 45 ++++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-)