implement FAT table write/read and setting the volume dirty status #132

Murmele · 2024-05-30T08:37:45Z

The dirty flag will be set when opening the volume to indicate that the FAT table might be out of date. In case of an unexpected close without unmount in the next mount it is visible that the FAT table is outdated and can be reread by the operating system (https://en.wikipedia.org/wiki/Design_of_the_FAT_file_system). Without this, the file must always closed before removing the sd card because otherwise the FAT table is outdated and so the files are not visible when mounting the sd card on a computer. For embedded systems it is quite likely that unexpect end can happen and always closing the file after a single write operation is a performance issue. For readonly this dirty flag is not required and therefore it will not set and reset when opening/closing a volume. This is same as the linux kernel is doing.

https://unix.stackexchange.com/questions/230181/why-does-linux-mark-fat-as-dirty-simply-due-to-mounting-it Linux

kernel is doing it during mount, so there is no need to ckeck it every time during a new write

thejpster · 2024-06-01T19:07:09Z

It appears the tests are failing - possible because the API changed?

thejpster · 2024-06-01T19:10:53Z

src/volume_mgr.rs

+    pub fn open_raw_volume(
+        &mut self,
+        volume_idx: VolumeIdx,
+        read_only: bool,


I think I'd prefer an enum VolumeOpenMode { ReadOnly, ReadWrite }, otherwise the caller writes:

thing.open_raw_volume(VolumeIdx(0), true)

and it's totally unclear what true means from the context.

Good point thanks!

changed in 2fd18cb

thejpster · 2024-06-01T19:11:29Z

src/volume_mgr.rs

+    ///
+    /// We do not support GUID Partition Table disks. Nor do we support any
+    /// concept of drive letters - that is for a higher layer to handle.
+    fn _open_volume(


We added an argument to open_raw_volume, so should we just add an argument to open_volume, and not have the two extra methods? Not sure.

I made it like this, so it is downward compatible

Then should we keep open_raw_volume backwards compatible too?

Is there a need for? I though because it will be used mostly internally so the impact is lower?

it's a public API, which I use in my projects.

I can change also to the same structure as for open_volume or do you prefer the argument way for both?

I think I prefer it with the argument.

Perfect then I will change it also for the open_volume

fixed in 70a5fdb

thejpster · 2024-06-01T19:14:05Z

src/volume_mgr.rs

+        }
+        Ok(())
+    }
+    #[cfg(test)]


Tests go in mod test {}.

changed in 9d6865a

thejpster · 2024-06-01T19:14:14Z

src/volume_mgr.rs

        let volume_idx = self.get_volume_by_id(volume)?;
+        if !self.open_volumes[volume_idx].read_only {
+            let VolumeType::Fat(volume_type) = &self.open_volumes[volume_idx].volume_type;


I've tried to write this code on the assumption that a non-FAT filesystem will be added in the future. The irrefutable match here will break when that happens. I think set_volume_status_dirty doesn't need volume_type as an argument - it should take volume_idx instead.

I changed this

thejpster · 2024-06-01T19:16:17Z

src/volume_mgr.rs

+    /// Sets the volume status dirty to dirty if true, to not dirty if false
+    fn set_volume_status_dirty(
+        &self,
+        volume: &FatVolume,


I think this should take volume_idx and handle the different types of volume internally. It should also skip updating the FAT if the FAT is already marked as dirty.

fixed in 56a07fe

thejpster · 2024-06-01T19:18:10Z

src/volume_mgr.rs

+        if !read_only {
+            let idx = self.get_volume_by_id(v)?;
+            let VolumeType::Fat(volume_type) = &self.open_volumes[idx].volume_type;
+            self.set_volume_status_dirty(volume_type, true)?;


I don't think you need to mark it as dirty until the first write occurs?

Ah, I read the comment thread from the kernel. Yeah, I can see the value in marking it dirty on first mount.

Note to self - the Neotron OS will need changing to ensure the volume is unmounted when the last file is closed.

But this flag does not fix my problem haha, I have to update somehow the fat, at least Linux does not check the fat and repairs it if needed. I thought when the bit is cleared (is dirty) during mount it will be "repaired" and the FAT table gets update, but this is not the case

I don't think Linux will run an fsck on a block device unless you ask it to. It is Windows that prompts to run Scandisk if it things the volume wasn't cleanly shut down.

thejpster

I like the idea - left a few notes.

Murmele · 2024-06-01T21:02:19Z

It appears the tests are failing - possible because the API changed?

I will have a look into it next days

thejpster · 2024-06-06T09:39:02Z

So we already have functions for updating the FAT: FatVolume::update_fat. Could we just create some new ClusterId variants which represent clean/dirty volumes and use the update_fat function to write them to ClusterIdx 1?

Murmele · 2024-06-06T17:22:38Z

So we already have functions for updating the FAT: FatVolume::update_fat. Could we just create some new ClusterId variants which represent clean/dirty volumes and use the update_fat function to write them to ClusterIdx 1?

My idea was now with this flag, after a restart of my device I check this flag and if dirty I wanna update somehow the dir entries (I think this is currently my problem I have, if I don't update the dirEntry (maybe the size property?) it will not shown in the file explorer correctly). I have to check if I can get there somehow (Maybe iterating through the complete FAT and setting the new file size?).

Sorry right now I am not that deep into the topic to understand your idea. I will think about and doing some research.

But reparing the FAT could be done in another MR

In my case the sd card never leaves the device but at the end over usb storage access shall be established, so I can easily update the FAT at that point

…me dirty status Description: The dirty flag will be set when opening the volume to indicate that the FAT table might be out of date. In case of an unexpected close without unmount in the next mount it is visible that the FAT table is outdated and can be reread by the operating system (https://en.wikipedia.org/wiki/Design_of_the_FAT_file_system). Without this, the file must always closed before removing the sd card because otherwise the FAT table is outdated and so the files are not visible when mounting the sd card on a computer. For embedded systems it is quite likely that unexpect end can happen and always closing the file after a single write operation is a performance issue. For readonly this dirty flag is not required and therefore it will not set and reset when opening/closing a volume. https://unix.stackexchange.com/questions/230181/why-does-linux-mark-fat-as-dirty-simply-due-to-mounting-it Linux kernel is doing it during mount, so there is no need to ckeck it every time during a new write

…ond block of the fat table

Reason: makes the funktionality more clear and less error prone

so it can be also used outside of tests

…eady set Reason: the function can determine it self.

…volume

Murmele · 2024-07-07T07:25:16Z

I fixed the tests

thejpster · 2024-07-07T09:29:29Z

So we already have functions for updating the FAT: FatVolume::update_fat. Could we just create some new ClusterId variants which represent clean/dirty volumes and use the update_fat function to write them to ClusterIdx 1?

So I like the read-only and read-write changes. If they were isolated I'd happily merge them.

But to return to my earlier comment and perhaps clarify - I don't think we need a whole new module for writing a couple of FAT entries to indicate dirty/clean, when we already have functions which write FAT entries for the purposes of tracking cluster allocations for files. Either all the FAT writing code needs to go in to the new module, or the clean/dirty bits need to be set with the existing function.

Murmele · 2024-09-09T13:52:27Z

So we already have functions for updating the FAT: FatVolume::update_fat. Could we just create some new ClusterId variants which represent clean/dirty volumes and use the update_fat function to write them to ClusterIdx 1?

So I like the read-only and read-write changes. If they were isolated I'd happily merge them.

But to return to my earlier comment and perhaps clarify - I don't think we need a whole new module for writing a couple of FAT entries to indicate dirty/clean, when we already have functions which write FAT entries for the purposes of tracking cluster allocations for files. Either all the FAT writing code needs to go in to the new module, or the clean/dirty bits need to be set with the existing function.

I will have a look into it. Thanks!

thejpster reviewed Jun 1, 2024

View reviewed changes

Murmele marked this pull request as ready for review June 3, 2024 20:27

Murmele force-pushed the volume_status_dirty branch 4 times, most recently from c155268 to c1c57d3 Compare June 5, 2024 18:06

Murmele added 14 commits July 7, 2024 09:06

FatTableTooLarge can be ignored, because the value flag is in the sec…

e7d4565

…ond block of the fat table

Add FAT table to test

d2bc1d1

check also if the bit is set that it is really dirty

8d87d1a

Add readonly volume open function

e7a8a77

add test to check write as well

ba5771c

use enum instead of bool

f4bc218

Reason: makes the funktionality more clear and less error prone

make volume_status_dirty public

fbf4d20

so it can be also used outside of tests

use directly raw volume instead of FatVolume. Do not set dirty if alr…

87bf066

…eady set Reason: the function can determine it self.

fix ci

8ab0cea

derive defmt::Format

598e64e

fix compilation

d236739

fix test

0dcab16

use open_volume with parameter instead of two different functions

06fb719

set volume dirty always also for raw volumes

330f807

Murmele force-pushed the volume_status_dirty branch from 2c425a1 to 330f807 Compare July 7, 2024 07:07

Martin Marmsoler added 2 commits July 7, 2024 09:09

fix compilation

35e086b

fix tests. If ReadWrite, the dirty flag will be set when opening the …

e7eb7ba

…volume

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement FAT table write/read and setting the volume dirty status #132

implement FAT table write/read and setting the volume dirty status #132

Murmele commented May 30, 2024

thejpster commented Jun 1, 2024

thejpster Jun 1, 2024

Murmele Jun 1, 2024

Murmele Jun 3, 2024

thejpster Jun 1, 2024

Murmele Jun 3, 2024

thejpster Jun 4, 2024

Murmele Jun 5, 2024

thejpster Jun 5, 2024

Murmele Jun 6, 2024

thejpster Jun 6, 2024

Murmele Jun 6, 2024

Murmele Jun 6, 2024

thejpster Jun 1, 2024

Murmele Jun 3, 2024

thejpster Jun 1, 2024 •

edited

Loading

Murmele Jun 5, 2024

thejpster Jun 1, 2024

Murmele Jun 3, 2024

thejpster Jun 1, 2024

thejpster Jun 1, 2024

Murmele Jun 3, 2024

thejpster Jun 6, 2024

thejpster left a comment

Murmele commented Jun 1, 2024

thejpster commented Jun 6, 2024

Murmele commented Jun 6, 2024 •

edited

Loading

Murmele commented Jul 7, 2024

thejpster commented Jul 7, 2024

Murmele commented Sep 9, 2024

implement FAT table write/read and setting the volume dirty status #132

Are you sure you want to change the base?

implement FAT table write/read and setting the volume dirty status #132

Conversation

Murmele commented May 30, 2024

thejpster commented Jun 1, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejpster Jun 1, 2024 • edited Loading

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

thejpster left a comment

Choose a reason for hiding this comment

Murmele commented Jun 1, 2024

thejpster commented Jun 6, 2024

Murmele commented Jun 6, 2024 • edited Loading

Murmele commented Jul 7, 2024

thejpster commented Jul 7, 2024

Murmele commented Sep 9, 2024

thejpster Jun 1, 2024 •

edited

Loading

Murmele commented Jun 6, 2024 •

edited

Loading