Allow clearing FAT dirty flag. #66

reitermarkus · 2022-05-26T04:01:58Z

When the FAT dirty flag is set (which happens after ejecting an SD card without unmounting first on macOS), creating directories does not work as expected afterwards: When remounting the SD card after creating directories, these directories will be files instead.

If the dirty flag is cleared before creating the directories, mounting the SD card again under macOS works as expected, i.e. the directories are accessible as normal.

I have added a set_fat_dirty_flag method to clear the flag, but I am not sure when clearing this flag should be done.

When the dirty flag is set, this should result in an error when creating the FileSystem (or when trying to write to it).
Alternatively, the flag should ideally be cleared automatically when mounting after the filesystem has been checked somehow.

Closes #64.

src/table.rs

src/fs.rs

rafalh · 2022-05-29T23:48:11Z

src/fs.rs

@@ -604,6 +608,15 @@ impl<IO: Read + Write + Seek, TP, OCC> FileSystem<IO, TP, OCC> {
        Ok(())
    }

+    pub fn set_fat_dirty_flag(&self, dirty: bool) -> Result<(), Error<IO::Error>> {


I think handling it when filesystem is mounted would be better. Fail by default if dirty flag is set + add option in FsOptions to allow mounting a dirty FS (and clear the flag during mounting - this probably deserves another option). Also handle dirty flag from BSP. I don't like showing in user-facing API those weird and legacy things from FAT like doubled dirty flags by making two setters and getters.
Of course it is still a bad to work with a dirty FS. Proper solution would be to add some filesystem checking functionality but that's probably a ton of work...

reitermarkus · 2022-06-11T09:48:49Z

@rafalh, I have moved the check and added an option to ignore/clear the flag when mounting the file system.

reitermarkus · 2022-06-13T12:01:47Z

@rafalh, can you review this again? Thanks.

reitermarkus · 2022-08-22T19:48:26Z

@rafalh, ping.

reitermarkus · 2022-10-06T02:43:25Z

@rafalh, could you have another look here? Thanks.

rafalh

I think you mixed in too many things here. Dirty bit handling is a very important thing and can result in undetected filesystem corruption if not handled with care. I don't like changes to it being mixed with some refactors like const functions or some weird changes to errors in DiskSlice.
Please split this into smaller PRs that are easy to review.

rafalh · 2022-10-14T11:30:11Z

tests/write.rs

    let status_flags = fs.read_status_flags().unwrap();
-    assert_eq!(status_flags.dirty(), true);
+    assert_eq!(status_flags.dirty(), false);


You could also add one more remount step to make sure dirty flag is cleared on the disk (it is cached so such test makes sense)

I added another remount step.

rafalh · 2022-10-14T11:52:39Z

src/table.rs

+                val |= FAT32_IO_ERROR_BIT;
+            }
+
+            Fat32::set(fat, 1, FatValue::Data(!val))


I would prefer to keep it more explicit than using bit-wise negation here like that.
Also it's not really a "data" so it makes more sense to use set_raw function

rafalh · 2022-10-14T11:56:22Z

src/fs.rs

    }

-    fn unmount_internal(&self) -> Result<(), Error<IO::Error>> {
+    pub fn flush(&mut self) -> Result<(), Error<IO::Error>> {


Hm, at first I through it's a bad idea to allow clearing the dirty flag like that, but it makes sense because dirty flag is set after first write, not at mount. If I understand correctly &mut will require all files to be closed, so it should be okay.
But if this was to be public it must be documented.

Yes, the &mut is the important part here. This is basically the same as unmount without actually dropping the IO object.

rafalh · 2022-10-14T16:32:54Z

src/fs.rs

-        if bpb.status_flags().dirty {
+        if bpb_status_flags.dirty() {
+            if options.ignore_dirty_flag {
+                warn!("BPB is dirty, clearing dirty flag.");


it's not BPB being dirty, it's entire filesystem. Please change it, e.g. "Filesystem is dirty (BPB), clearing..."

rafalh · 2022-10-14T16:33:07Z

src/fs.rs

+        let mut fat_status_flags = read_fat_flags(&mut fat_slice::<&mut IO, IO::Error, IO>(&mut disk, &bpb), fat_type)?;
+        if fat_status_flags.dirty() {
+            if options.ignore_dirty_flag {
+                warn!("FAT is dirty, clearing dirty flag.");


it's not BPB being dirty, it's entire filesystem. Please change it, e.g. "Filesystem is dirty (FAT), clearing..."

rafalh · 2022-10-14T16:36:48Z

src/fs.rs

@@ -512,7 +552,7 @@ impl<IO: Read + Write + Seek, TP, OCC> FileSystem<IO, TP, OCC> {
    ///
    /// `Error::Io` will be returned if the underlying storage object returned an I/O error.
    pub fn read_status_flags(&self) -> Result<FsStatusFlags, Error<IO::Error>> {
-        let bpb_status = self.bpb.status_flags();
+        let bpb_status = self.current_status_flags.get();


this function allowed to check if dirty flag was set before filesystem was opened. current_status_flags will never have this flag set, even if ignore_dirty_flag is set. I would rather keep the old behavior for now because new one makes it impossible to mount a dirty volume and still get information about it being dirty.

rafalh · 2022-10-14T16:42:33Z

src/fs.rs

-        disk.write_u8(encoded)?;
-        self.current_status_flags.set(flags);
-        Ok(())
+        set_dirty_flag(&mut *disk, self.fat_type(), &self.current_status_flags, dirty)


why adding this new free set_dirty_flag function? AFAIK it is used only in this place.

rafalh · 2022-10-14T16:43:19Z

src/fs.rs

@@ -695,12 +716,12 @@ pub(crate) struct FsIoAdapter<'a, IO: ReadWriteSeek, TP, OCC> {
 }

 impl<IO: ReadWriteSeek, TP, OCC> IoBase for FsIoAdapter<'_, IO, TP, OCC> {
-    type Error = IO::Error;
+    type Error = Error<IO::Error>;


why this change? I see some more changes to error handling but I don't understand how it is related to clearing dirty flags

I don't quite remember exactly. Somewhere the errors types were not compatible during propagation.

src/fs.rs

reitermarkus · 2023-12-21T19:24:57Z

@rafalh, can you take a look here? You should review my other PRs first so I can rebase this one for easier review.

reitermarkus marked this pull request as draft May 26, 2022 04:02

rafalh requested changes May 29, 2022

View reviewed changes

reitermarkus force-pushed the fat-dirty branch 2 times, most recently from c57fad8 to 9bbab4f Compare May 31, 2022 21:08

reitermarkus force-pushed the fat-dirty branch 3 times, most recently from 4971e1c to d05dca7 Compare June 13, 2022 11:03

reitermarkus marked this pull request as ready for review June 13, 2022 11:15

reitermarkus requested a review from rafalh August 22, 2022 19:50

rafalh requested changes Oct 14, 2022

View reviewed changes

This was referenced Oct 30, 2022

Cannot create directories with lfn feature. #64

Open

Return IO object when unmounting and add flush method. #78

Open

reitermarkus added 11 commits November 5, 2022 17:03

Allow clearing FAT dirty flag.

e9590ed

Add option for clearing dirty flag.

215aac2

Format code.

3b92716

Remove unused import.

f6976a9

Reorder types.

4a6f78d

Fix warnings.

9a28fc3

Fix type.

a4b389f

Try fixing BorrowMutError.

2ce03f4

Clear BPB dirty flags before creating FileSystem.

0dd360a

Clear FAT dirty flag before creating FileSystem.

95a1b05

Revert unneeded changes.

7f4aa0c

reitermarkus force-pushed the fat-dirty branch 2 times, most recently from dfb1181 to e8f9663 Compare November 5, 2022 16:08

reitermarkus mentioned this pull request Nov 5, 2022

Add error for dirty file system. #79

Open

reitermarkus force-pushed the fat-dirty branch from e8f9663 to 8e79f69 Compare November 5, 2022 16:12

Move functions into impl blocks.

6ec1aa4

reitermarkus force-pushed the fat-dirty branch from 8e79f69 to 6ec1aa4 Compare November 5, 2022 16:13

Extend test.

a23e9cd

reitermarkus requested a review from rafalh November 13, 2022 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow clearing FAT dirty flag. #66

Allow clearing FAT dirty flag. #66

reitermarkus commented May 26, 2022

rafalh May 29, 2022

reitermarkus commented Jun 11, 2022

reitermarkus commented Jun 13, 2022

reitermarkus commented Aug 22, 2022

reitermarkus commented Oct 6, 2022

rafalh left a comment

rafalh Oct 14, 2022

reitermarkus Nov 5, 2022

rafalh Oct 14, 2022

rafalh Oct 14, 2022

reitermarkus Nov 5, 2022

rafalh Oct 14, 2022

rafalh Oct 14, 2022

rafalh Oct 14, 2022

rafalh Oct 14, 2022

rafalh Oct 14, 2022

reitermarkus Nov 5, 2022

reitermarkus commented Dec 21, 2023

Allow clearing FAT dirty flag. #66

Are you sure you want to change the base?

Allow clearing FAT dirty flag. #66

Conversation

reitermarkus commented May 26, 2022

Choose a reason for hiding this comment

reitermarkus commented Jun 11, 2022

reitermarkus commented Jun 13, 2022

reitermarkus commented Aug 22, 2022

reitermarkus commented Oct 6, 2022

rafalh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus commented Dec 21, 2023