Rockbox

Tasklist

FS#11774 - Unmount all disks before entering USB slave mode to prevent filesystem corruption

Attached to Project: Rockbox
Opened by sideral (sideral) - Wednesday, 24 November 2010, 13:44 GMT
Last edited by Frank Gevaerts (fg) - Sunday, 28 November 2010, 15:25 GMT
Task Type Bugs
Category Drivers
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

I recently enabled Rockbox-native USB slave mode on my Sansa ClipV2 through the patch provided in  FS#11664  - Workaround for usb random failures on amsv2. Ever since then, I have had very stable USB connections, but regular filesystem corruption on the player's flash drive. This had never happened before, when I still was had to boot into the OF to use its USB support.

As I allow Rockbox to write to my disk for various reasons, such as scrobbler logging and database statistics updates. Also, my flash drive is fairly full, leading to frequent disk-block reallocation. Thus I suspected that either (1) the filesystem wasn't fully flushed when USB slave mode was entered, or (2) that Rockbox was using stale filesystem data after returning from USB slave mode that would later lead to filesystem corruption. I slightly lean towards explanation 2, because I never observed filesystem corruption when powering off Rockbox (to use the OF's USB support) even though the shutdown code does not unmount the disks either, AFAIK.

As an experiment, I added support for unmounting all disks before entering USB slave mode – see attached patch. I've been running with this patch for a week now, and I haven't seen any filesystem corruption since then.

Comments?
This task depends upon

Closed by  Frank Gevaerts (fg)
Sunday, 28 November 2010, 15:25 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed as r28693
Comment by amaury pouly (pamaury) - Wednesday, 24 November 2010, 15:48 GMT
That seems rather strange, I'll have a look at it but I'm pretty sure firmware/usb.c already does something like this.
If it doesn't, then there might be a problem like a file handle not released (typically a font handle) which might eventually need to crash but I think we solved that issue quite some time ago.

Last thing which is bothering me: usb slave is mode for devices which have a hardware bridge and thus which don't handle usb in software. As far as I know, this is not the case of the clipv2 thus I 'm a bit confused by your patch !
Comment by Frank Gevaerts (fg) - Wednesday, 24 November 2010, 17:06 GMT
I think this is a good idea. Why do you basically reimplement disk_unmount() inside disk_unmount_all() though, instead of just calling disk_unmount()?
Comment by sideral (sideral) - Thursday, 25 November 2010, 00:14 GMT
pamaury, I think fg answered your questions already on IRC (http://www.rockbox.org/irc/log-20101124#17:00), so let me go straight to his question:

fg writes:
> Why do you basically reimplement disk_unmount() inside
> disk_unmount_all() though, instead of just calling disk_unmount()?

The implementation is subtly different (look at the “if (voldrive…)” line). Yes, this probably could be factored better, but I cannot just call disk_unmount because that function expects a drive ID and not a volume ID. Calling this function means that we'd have to go through the vol_drive array twice for each drive.

Suggestion: Keep the loops in these function as is and factor out the inner loop's body. OK?
Comment by amaury pouly (pamaury) - Thursday, 25 November 2010, 12:15 GMT
I don't see the problem. You can loop on all drives and call disk_umount. Something like:

#ifndef HAVE_MULTIDRIVE
disk_unmount(0);
#else
for(i = 0; i < NUM_DRIVES; i++)
{
#ifdef HAVE_HOTSWAP
if(storage_present(i))
#endif
disk_unmount(i);
}
#endif

Perhaps the hotswap define is even useless. Notice that this is correct since disk_unmount(i) unmounts all volume on drive i. So this will unmounts all volumes on any drive. No ?
Comment by sideral (sideral) - Saturday, 27 November 2010, 22:50 GMT
OK, I changed the code according to your advice. New patch attached.
Comment by amaury pouly (pamaury) - Saturday, 27 November 2010, 23:24 GMT
This looks ok to me, I hope the ifdef are ok.
Comment by Frank Gevaerts (fg) - Sunday, 28 November 2010, 15:25 GMT
I removed those complicated #ifdefs. They ended up meaning "on every target except mini2440 and application", which I think isn't worth it

Loading...