[PATCH 1/6] fs: fat: factor out dentry link create/delete
Gabriel D'Alimonte
gabriel.dalimonte at gmail.com
Sun Jan 26 05:34:37 CET 2025
Hi!
Thank you for the reviews and feedback.
On Wed, Jan 22, 2025 at 2:08 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> Am 22. Januar 2025 06:32:26 MEZ schrieb Gabriel Dalimonte <gabriel.dalimonte at gmail.com>:
> >Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte at gmail.com>
>
> Hello Gabriel,
>
> Thank you for implementing this.
>
> Please, add a commit message to each patch.
>
>
> >---
> >
> > fs/fat/fat_write.c | 122 ++++++++++++++++++++++++---------------------
> > 1 file changed, 66 insertions(+), 56 deletions(-)
> >
> >diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> >index ea877ee917..b86e78abc0 100644
> >--- a/fs/fat/fat_write.c
> >+++ b/fs/fat/fat_write.c
> >@@ -1215,6 +1215,44 @@ static void fill_dentry(fsdata *mydata, dir_entry *dentptr,
> > memcpy(&dentptr->nameext, shortname, SHORT_NAME_SIZE);
> > }
> >
> >+/**
> >+ * create_link() - inserts a directory entry for a cluster
>
> ... for a file or directory
>
> >+ *
> >+ * @itr: directory iterator
> >+ * @basename: name of the new entry
>
> @basename: file name
>
> >+ * @clust: cluster number the new directory entry should point to
>
> ... point to. Use 0 if no cluster is assigned yet.
>
> >+ * @size: file size
> >+ * @attr: file attributes
>
> Do we need to keep the creation timestamp when moving files? And only update the change timestamp?
>
> Should the archive flag be set when moving files?
>
> What does EDK II or Linux in these cases?
>
On Linux (6.12) [1] it looks like the following happens with respect
to timestamps:
1. If new_path is not overwriting an existing file, the parent
directory of new_path has its modification time updated. (via
`vfat_add_entry` [2] )
2. old_path's parent directory has access, and modification time
updated via `vfat_remove_entries` [3] and `vfat_update_dir_metadata`
[4].
3. The moved file maintains its timestamps and does not update them.
EDK II (branch edk2-stable202411) [5] updates the last modification
and last access times on the parent directories (via
`FatRemoveDirEnt`/`FatCreateDirEnt` [9] -> `FatStoreDirEnt` [9] ->
`FatAccessEntry` [9] -> `FatAccessOFile` [8] -- marking the files
dirty). It updates the last modification and last access times (via
FatOFileFlush [6] -- called on line 469 in [5]) while preserving the
creation time on the moved file (via FatCloneDirEnt [7]). The
implementation of `FatOFileFlush` results in timestamps being updated
up the file tree to the root as `FatOFileFlush` calls `FatStoreDirEnt`
marking the subsequent parent directories as dirty.
Linux appears to not touch the ATTR_ARCH attribute on file moves and
treats ATTR_ARCH and ATTR_DIR as mutually exclusive [10]. On the other
hand, EDK II directly sets the archive bit on the file. The flush
operation [6] will set it on each directory up to the root, from my
understanding.
Is there a preference on either implementation? Or are there
constraints in U-Boot that make either of these timestamp and archive
attribute implementations unsuitable to match?
Thanks,
Gabriel
[1] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L959-L996
[2] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L682
[3] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/dir.c#L1083
[4] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L924
[5] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429742/FatPkg/EnhancedFatDxe/Info.c#L389-L442
[6] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429742/FatPkg/EnhancedFatDxe/Flush.c#L245-L249
[7] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429742/FatPkg/EnhancedFatDxe/DirectoryManage.c#L196-L220
[8] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429742/FatPkg/EnhancedFatDxe/ReadWrite.c#L507-L510
[9] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429742/FatPkg/EnhancedFatDxe/DirectoryManage.c
[10] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L643
> Maybe you could describe these design considerations in the commit message.
>
> Please, document which fields and flags are implicitly set in the function description.
>
> >+ * Return: 0 for success
> >+ */
> >+static int create_link(fat_itr *itr, char *basename, __u32 clust, __u32 size,
> >+ __u8 attr)
> >+{
> >+ char shortname[SHORT_NAME_SIZE];
> >+ int ndent;
> >+ int ret;
> >+
> >+ /* Check if long name is needed */
> >+ ndent = set_name(itr, basename, shortname);
> >+ if (ndent < 0)
> >+ return ndent;
> >+ ret = fat_find_empty_dentries(itr, ndent);
> >+ if (ret)
> >+ return ret;
> >+ if (ndent > 1) {
> >+ /* Set long name entries */
> >+ ret = fill_dir_slot(itr, basename, shortname);
> >+ if (ret)
> >+ return ret;
> >+ }
> >+
> >+ /* Add attribute as archive */
>
> Could this be changed to
> "Add archive attribute"?.
>
> >+ fill_dentry(itr->fsdata, itr->dent, shortname, clust, size,
> >+ attr | ATTR_ARCH);
> >+
> >+ return 0;
> >+}
> >+
> > /**
> > * find_directory_entry() - find a directory entry by filename
> > *
> >@@ -1420,35 +1458,15 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer,
> > /* Update change date */
> > dentry_set_time(retdent);
> > } else {
> >- /* Create a new file */
> >- char shortname[SHORT_NAME_SIZE];
> >- int ndent;
> >-
> > if (pos) {
> > /* No hole allowed */
>
> This is another deficiency of our FAT driver worth looking into in future.
>
> Best regards
>
> Heinrich
>
> > ret = -EINVAL;
> > goto exit;
> > }
> >
> >- /* Check if long name is needed */
> >- ndent = set_name(itr, basename, shortname);
> >- if (ndent < 0) {
> >- ret = ndent;
> >- goto exit;
> >- }
> >- ret = fat_find_empty_dentries(itr, ndent);
> >+ ret = create_link(itr, basename, 0, size, 0);
> > if (ret)
> > goto exit;
> >- if (ndent > 1) {
> >- /* Set long name entries */
> >- ret = fill_dir_slot(itr, basename, shortname);
> >- if (ret)
> >- goto exit;
> >- }
> >-
> >- /* Set short name entry */
> >- fill_dentry(itr->fsdata, itr->dent, shortname, 0, size,
> >- ATTR_ARCH);
> >
> > retdent = itr->dent;
> > }
> >@@ -1564,6 +1582,31 @@ static int delete_long_name(fat_itr *itr)
> > return 0;
> > }
> >
> >+/**
> >+ * delete_dentry_link() - deletes a directory entry, but not the cluster chain
> >+ * it points to
> >+ *
> >+ * @itr: the first directory entry (if a longname) to remove
> >+ * Return: 0 for success
> >+ */
> >+static int delete_dentry_link(fat_itr *itr)
> >+{
> >+ itr->dent = itr->dent_start;
> >+ itr->remaining = itr->dent_rem;
> >+ /* Delete long name */
> >+ if ((itr->dent->attr & ATTR_VFAT) == ATTR_VFAT &&
> >+ (itr->dent->nameext.name[0] & LAST_LONG_ENTRY_MASK)) {
> >+ int ret;
> >+
> >+ ret = delete_long_name(itr);
> >+ if (ret)
> >+ return ret;
> >+ }
> >+ /* Delete short name */
> >+ delete_single_dentry(itr);
> >+ return flush_dir(itr);
> >+}
> >+
> > /**
> > * delete_dentry_long() - remove directory entry
> > *
> >@@ -1589,21 +1632,7 @@ static int delete_dentry_long(fat_itr *itr)
> > if (ret)
> > return ret;
> > }
> >- itr->dent = itr->dent_start;
> >- itr->remaining = itr->dent_rem;
> >- dent = itr->dent_start;
> >- /* Delete long name */
> >- if ((dent->attr & ATTR_VFAT) == ATTR_VFAT &&
> >- (dent->nameext.name[0] & LAST_LONG_ENTRY_MASK)) {
> >- int ret;
> >-
> >- ret = delete_long_name(itr);
> >- if (ret)
> >- return ret;
> >- }
> >- /* Delete short name */
> >- delete_single_dentry(itr);
> >- return flush_dir(itr);
> >+ return delete_dentry_link(itr);
> > }
> >
> > int fat_unlink(const char *filename)
> >@@ -1725,9 +1754,6 @@ int fat_mkdir(const char *dirname)
> > ret = -EEXIST;
> > goto exit;
> > } else {
> >- char shortname[SHORT_NAME_SIZE];
> >- int ndent;
> >-
> > if (itr->is_root) {
> > /* root dir cannot have "." or ".." */
> > if (!strcmp(l_dirname, ".") ||
> >@@ -1737,25 +1763,9 @@ int fat_mkdir(const char *dirname)
> > }
> > }
> >
> >- /* Check if long name is needed */
> >- ndent = set_name(itr, basename, shortname);
> >- if (ndent < 0) {
> >- ret = ndent;
> >- goto exit;
> >- }
> >- ret = fat_find_empty_dentries(itr, ndent);
> >+ ret = create_link(itr, basename, 0, 0, ATTR_DIR);
> > if (ret)
> > goto exit;
> >- if (ndent > 1) {
> >- /* Set long name entries */
> >- ret = fill_dir_slot(itr, basename, shortname);
> >- if (ret)
> >- goto exit;
> >- }
> >-
> >- /* Set attribute as archive for regular file */
> >- fill_dentry(itr->fsdata, itr->dent, shortname, 0, 0,
> >- ATTR_DIR | ATTR_ARCH);
> >
> > retdent = itr->dent;
> > }
>
More information about the U-Boot
mailing list