Faster writes for libFAT 1.0.7

[PypeBros]

Hello, folks.

While updating one of my software for the latest libnds and libfat, I observed that writes to the media card was roughly 3 times slower as it was in the past with the same hardware. I first instrumented cache.c so that it gathers statistics about cache misses, cache hits, write back and write through. It allowed me to point out that in its distributed version, libFAT 1.0.7 seems to perform a large amount of useless reads by pre-fetching the content of a cluster before it rewrites its content with new data.

I have then modified the cache management routines to implement a lazy-reading approach, where content of an entry in the cache is only fetched from the media on demand, allowing an entry to receive data-to-be-written first, and committing those writes when the entry is reclaimed.

There's also a slight modification of libfatInit() in that diff due to SCSD.dldi to report 'isInserted()==false' for unknown reason. If someone wants to point me to a SCSD dldi that is more recent than 2007, you're welcome.

Please keep in mind that this is experimental and might lead to data loss if there's any bug still pending. Use at your own risk, preferably on a test-bed device and on a media card that has been backed' up first.

unacceptable patch removed

HTH

[WinterMute]

All patch submissions should be placed on the patch tracker at https://sourceforge.net/tracker/?group_ ... tid=668553

Patches must be made against SVN HEAD and not released tarballs - bugs get fixed in SVN, tested and then released. Currently libfat 1.0.8 is waiting on some further tests but fixes an issue with FAT12 formatted cards being rejected. There are also some changes which will probably make your patch fail.

Any patch which causes problems with the other supported platforms will be rejected immediately, the patch you linked here removed the other targets so obviously you didn't even bother to build them.

The patch also contains large quantities of debug code which should have been removed.

Something I'm currently considering is reverting the cache system and placing it in the wii/gamecube pseudo driver in libogc instead.

[PypeBros]

All patch submissions should be placed on the patch tracker at https://sourceforge.net/tracker/?group_ ... tid=668553

Patches must be made against SVN HEAD and not released tarballs - bugs get fixed in SVN, tested and then released. Currently libfat 1.0.8 is waiting on some further tests but fixes an issue with FAT12 formatted cards being rejected. There are also some changes which will probably make your patch fail.

I see. I will try to do that as soon as time allows.

Any patch which causes problems with the other supported platforms will be rejected immediately, the patch you linked here removed the other targets so obviously you didn't even bother to build them.

Sorry, I'm afraid I'm a DS programmer here. I don't have other targets cross-compilers or support libraries at hand (okay, that might be fixed easily), nor hardware or emulator to perform checks on a platform different from NDS. I commented build instructions for other targets because there is no clean way to indicate they're not available on a system. As I pointed out, "Please keep in mind that this is experimental and might lead to data loss if there's any bug still pending". I don't pretend to offer something that is ready-to-release, but I do believe it might be easier to improve libFAT with the patch I provided than from scratch. I suppose you have some sort of safety checks or automated testbed anyway.

The patch also contains large quantities of debug code which should have been removed.

I guess you relate to the "iprintf" in the fatInit here. I should admittedly have split the "SCSD initialisation problem" away from the "cache perfomance fix", yes, and I'll do so for the tidied-up patch to appear in the tracker system. I haven't found a better way to obtain troubleshooting information that (imho) could be handy for a more descriptive error message. Mapping disc->isInserted==false or mount()==false to errno values isn't straightforward. An additional API function to enquiry about the initialisation status of a specific device could be handy, but defining would be intrusive from me.

Something I'm currently considering is reverting the cache system and placing it in the wii/gamecube pseudo driver in libogc instead.

I think I'd have appreciate the possibility to #define __NO_CACHE__ at some point. Given how FAT filesystem heavily relies on caching of directory entries and the allocation table, it might not be wise ... but perhaps "reverting the cache system" doesn't mean "not using any cache".


PS: Btw, After spending one week to figure out what was going wrong, and after returning my improvements to the projects as the GPL philosophy suggests me to do, I find it pretty harsh to be thrown that "didn't even bother". That makes me more inclined to run away than to enroll or help further, personnally. I'm obviously ignorant of your project habits, I'm fine with being pointed at a "how to submit a patch" thread, but I'd appreciate to be considered as a person, here, rather than as a check list.

[WinterMute]

Any patch which causes problems with the other supported platforms will be rejected immediately, the patch you linked here removed the other targets so obviously you didn't even bother to build them.

Sorry, I'm afraid I'm a DS programmer here. I don't have other targets cross-compilers or support libraries at hand (okay, that might be fixed easily), nor hardware or emulator to perform checks on a platform different from NDS. I commented build instructions for other targets because there is no clean way to indicate they're not available on a system.

There are targets for each system in the Makefile, make nds-install will build and install only the nds lib.

The patch also contains large quantities of debug code which should have been removed.

I guess you relate to the "iprintf" in the fatInit here. I should admittedly have split the "SCSD initialisation problem" away from the "cache perfomance fix", yes, and I'll do so for the tidied-up patch to appear in the tracker system. I haven't found a better way to obtain troubleshooting information that (imho) could be handy for a more descriptive error message. Mapping disc->isInserted==false or mount()==false to errno values isn't straightforward. An additional API function to enquiry about the initialisation status of a specific device could be handy, but defining would be intrusive from me.

There were a few more than that, try this in the msys bash shell, both from the original source directory & from your modifications.

Code: Select all

find . -name *.c -exec grep -H printf {} \;

For SCSD have a look at http://eng.supercard.sc/soft_sd_2.htm for the 1.84 firmware which includes a dldi.scp file which enables autopatching for 1.84+. The latest firmware is 1.85 but doesn't appear to include this file. I'm using 1.85 on mine with the dldi.scp file and I've never had an issue with the card.

Something I'm currently considering is reverting the cache system and placing it in the wii/gamecube pseudo driver in libogc instead.

I think I'd have appreciate the possibility to #define __NO_CACHE__ at some point. Given how FAT filesystem heavily relies on caching of directory entries and the allocation table, it might not be wise ... but perhaps "reverting the cache system" doesn't mean "not using any cache".

The old system was faster on DS, the new one is ~1000% faster on cube/wii. It needs some more thought and investigation really. The DS has slow RAM and a small data cache which may negate the benefits of read ahead caching.

PS: Btw, After spending one week to figure out what was going wrong, and after returning my improvements to the projects as the GPL philosophy suggests me to do, I find it pretty harsh to be thrown that "didn't even bother". That makes me more inclined to run away than to enroll or help further, personnally. I'm obviously ignorant of your project habits, I'm fine with being pointed at a "how to submit a patch" thread, but I'd appreciate to be considered as a person, here, rather than as a check list.

You could always not take it as some kind of personal insult and merely a statement of something you did wrong. I appreciate that loss of non verbal cues in a text only environment can make things seem a lot harsher than they would appear in a face to face situation but it's really difficult to balance informative comments with "polite" vocabulary and still get things done. There's also the possibility that the phrase seems harsher in french than it would in english - I know literal translations of french and german come across really badly in english sometimes. I've had a few emails here and there which sign off with phrases like "you must respond to this criticism immediately" :p

It would also be much appreciated if you didn't release patches to the devkitPro libraries on your blog without checking that they're actually acceptable patches first. This kind of thing creates an absolute nightmare for support and is the primary reason why we don't support PAlib users at all.

Also, when contributing to open source projects it's probably best to try to interpret replies as constructive rather than be insulted and get upset. Things tend to progress quicker that way.

[PypeBros]

(...)
There are targets for each system in the Makefile, make nds-install will build and install only the nds lib.

Ok. I think I get the idea better and understand how I broke things up.

For SCSD have a look at http://eng.supercard.sc/soft_sd_2.htm for the 1.84 firmware which includes a dldi.scp file which enables autopatching for 1.84+. The latest firmware is 1.85 but doesn't appear to include this file. I'm using 1.85 on mine with the dldi.scp file and I've never had an issue with the card.

Thanks for that ^_^

You could always not take it as some kind of personal insult and merely a statement of something you did wrong. I appreciate that loss of non verbal cues in a text only environment can make things seem a lot harsher than they would appear in a face to face situation but it's really difficult to balance informative comments with "polite" vocabulary and still get things done. There's also the possibility that the phrase seems harsher in french than it would in english - I know literal translations of french and german come across really badly in english sometimes. I've had a few emails here and there which sign off with phrases like "you must respond to this criticism immediately" :p
Also, when contributing to open source projects it's probably best to try to interpret replies as constructive rather than be insulted and get upset. Things tend to progress quicker that way.

Thank you. I'll medidate this and ensure I read your replies with less prior expectations and appropriate willingness to work over language mismatches in the future.

It would also be much appreciated if you didn't release patches to the devkitPro libraries on your blog without checking that they're actually acceptable patches first. This kind of thing creates an absolute nightmare for support and is the primary reason why we don't support PAlib users at all.

Point taken. It was not my intent to induce support nightmare here. I removed the link from my blog.

The old system was faster on DS, the new one is ~1000% faster on cube/wii. It needs some more thought and investigation really. The DS has slow RAM and a small data cache which may negate the benefits of read ahead caching.

I must admit I hadn't proceeded with reading performance tests so far. I don't think my modification would affect it (it was designed to alter only the "writing" behaviour), but I'd be curious to give it some more tries. The CPU data cache might be a possible explanation. I also noted that there isn't necessarily a match between page size and cluster size, which may imply that some data would be read without being actually useful for a specific file operation. I'd expect read aheads to show more benefits when the device has a large per-operation overhead, such as spinning disks.

[PypeBros]

There are also some changes which will probably make your patch fail.

I suppose you refer to the "GEKKO" modification that decides whether a "write-through" of a whole set of sector should leave a copy in the cache when there was no previous entry in the cache for them. I will ensure that I don't get backwards when submitting my patch on the tracker. Hopefully enough, it looks that this modification will not conflict with mine but that they're rather complementary as I focused on the case where writes are performed in rather small increments (typically the size of a TCP packet received from WiFi).

[WinterMute]

Fancy having another go at this with 1.0.9?

[ritz]

Long time ago a few versions back, in a galaxy far, far away... I noticed a significant reduction in performance on the DS with libFAT around 1.0.5 (IIRC). It had something to do with the caching that was introduced as mentioned above. For a while I continued using 1.0.4 because of this. Anyway, I'm only posting this because I forgot to mention this back then: This slowness issue was completely rectified for me a version or two ago. The cache and/or cache settings for the DS implemention was modified.

tl;dr: What WinterMute just said... Try again.

[PypeBros]

Fancy having another go at this with 1.0.9?

I've got some time to give that performance issue a look again. I'll grab the libfat 1.0.9 and keep you updated.
(looks like I'll also check the "notify new posts" box ^^")

[PypeBros]

Fancy having another go at this with 1.0.9?

Looks like the performance are now comparable to the one in the "good old" implementation. Great job.