Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support attribute enumeration for files and directories #636

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Jan 29, 2022

This PR provides some additional attribute enumeration features.

  • Add lfs_statcfg, lfs_dir_readcfg functions

    Extends regular lfs_stat by querying set of attribute at the same time.
    Improves performance when generating directory listings.

  • Add lfs_file_getattr, lfs_file_setattr, lfs_file_removeattr

    Supports attribute manipulation on open files.
    Important for updating things like security descriptors, timestamps, etc.

    Note: Includes checks for registered attributes.
    Attempting to remove registered attributes fails.
    This could be handled by marking registered attribute as dirty.

  • Add lfs_file_enumattr() function

    Allows file attributes to be efficiently enumerated,
    required for filesystem backup, etc.

  • Support opening handle to directory

    Allows attributes to be accessed more efficiently, and enumerated

@mikee47
Copy link
Contributor Author

mikee47 commented Feb 19, 2022

Note: These changes are part of the Sming LittleFS library, used by Sming.

@geky geky added enhancement needs minor version new functionality only allowed in minor versions labels Mar 21, 2022
@xiaoxiang781216
Copy link

look like some feature is also implemented in: #636

@geky
Copy link
Member

geky commented Apr 14, 2022

Hi, yes, sorry this PR did not go in this release, due to time I was only looking at PRs with smaller scopes this release. I'm slow to bring in new APIs since they need to be supported for quite a while for backwards compatibility reasons.

This looks like a very interesting PR, thank @mikee47 for putting it together.

In particular I will need to see how it interacts with #580 and #513 (though #513 may come in much later due to being an API breaking change)

mikee47 and others added 4 commits May 15, 2024 07:57
Extends regular `lfs_stat` by querying set of attribute at the same time.
Improves performance of directory listings.
Supports attribute manipulation on open files.
Important for updating things like security descriptors, timestamps, etc.

Note: Attempting to remove registered attributes fails.
This could be handled by marking registered attribute as dirty.
Allows file attributes to be efficiently enumerated,
required for filesystem backup, etc.
Allows attributes to be accessed more efficiently, and enumerated
@mikee47 mikee47 force-pushed the feature/upstream-enum-attr branch from caa0b9d to a146db8 Compare May 15, 2024 09:13
@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 18212 B (+6.7%), Stack: 1440 B (+0.0%), Structs: 832 B (+2.5%)
Code Stack Structs Coverage
Default 18212 B (+6.7%) 1440 B (+0.0%) 832 B (+2.5%) Lines 2410/2721 lines (-4.4%)
Readonly 7014 B (+13.2%) 448 B (+0.0%) 832 B (+2.5%) Branches 1266/1682 branches (-3.3%)
Threadsafe 19156 B (+6.9%) 1440 B (+0.0%) 840 B (+2.4%) Benchmarks
Multiversion 18268 B (+6.7%) 1440 B (+0.0%) 836 B (+2.5%) Readed 29369693876 B (+0.0%)
Migrate 19904 B (+6.1%) 1744 B (+0.0%) 836 B (+2.5%) Proged 1482874766 B (+0.0%)
Error-asserts 18960 B (+6.8%) 1432 B (+0.0%) 832 B (+2.5%) Erased 1568888832 B (+0.0%)

slaff pushed a commit to SmingHub/Sming that referenced this pull request May 27, 2024
This PR updates the [Sming-LittleFS](https://github.com/mikee47/Sming-LittleFS) library, which implements IFS for the [littlefs](https://github.com/littlefs-project/littlefs) file system.

The library still uses a fork of littlefs because it adds attribute enumeration support - see upstream [PR request](littlefs-project/littlefs#636). This has now been rebased on version 2.9.3 - see [releases](https://github.com/littlefs-project/littlefs/releases) for details of all the changes. Here's a (non-exhaustive) summary:

- Improved RAM usage
- Improved performance
- v2.5.0 removed all recursion
- v2.6.0 bump the on-disk minor version of littlefs from lfs2.0 -> lfs2.1. "This change is backwards-compatible, but after the first write with the new version, the image on disk will no longer be mountable by older versions of littlefs."
- v2.7.0 Add `lfs_fs_stat` as an analog for the POSIX stavfs. This isn't currently used by Sming-LittleFS.
- v2.9.0 `rename` now returns LFS_ERR_NOTDIR if the source is a regular file and the destination is a directory. This better aligns with POSIX.

There are no functional changes to the library, but the samples have been simplified. An additional sample has been added to assess wear levelling behaviour (as a result of discussion #2771).
@geky
Copy link
Member

geky commented Jun 25, 2024

Hi @mikee47, sorry about the late response and lack of feedback. This is some really interesting work that deserves attention.

Unfortunately there is also work going on in the background to make attrs behave more reasonably in littlefs's CoW model (as well as a bunch of unrelated file work). My current thinking is the best route forward will be to hold off on bringing these in until other changes land, though this may create challenges for maintaining this PR...

Other requested new-API features are also in a holding pattern at the moment for similar reasons.


That being said, some high-level thoughts:

Add lfs_statcfg, lfs_dir_readcfg functions

These are a clever. These would also probably make integration layers quite a bit easier when populating time/permission/etc attrs.

The only though was do you think these need a full cfg object? Maybe we can get away with lfs_statattrs/lfs_stata/lfs_statx/?, etc, that take an attr list directly?

int lfs_stat(lfs_t *lfs, const char *name, struct lfs_info *info);
int lfs_stata(lfs_t *lfs, const char *name, struct lfs_info *info,
        struct lfs_attr *attrs, lfs_size_t attr_count);

I'm not sure what other configuration would be useful here.

Add lfs_file_getattr, lfs_file_setattr, lfs_file_removeattr

I realize these are appealing, but they are problematic with littlefs's CoW.

In littlefs, writes are not committed to mdirs until lfs_file_sync or lfs_file_close. This has the effect of making file writes an atomic "either-or" operation, which is helpful for implementing power-loss safe systems. But since attrs are stored directly in the mdir, we can't really save them anywhere unless they are backed by lfs_attr structs in lfs_file_config.

lfs_file_getattr would at least work.

Maybe lfs_file_setattr returning LFS_ERR_NOMEM if there is no backing lfs_attr struct associated with the specific attr would work? I'm not sure how useful this would be...

Add lfs_file_enumattr() function

This is a reasonable solution. It's at least better than Linux's listxattr returning one big string.

Though some of the background work includes moving lfs_fs_traverse away from callbacks and towards a lfs_dir_read-esque state machine.

If applied the same transformation here, this might look like:

int lfs_attrs_open(lfs_t *lfs, lfs_attrs_t *attrs, const char *path);
int lfs_attrs_read(lfs_t *lfs, lfs_attrs_t *attrs, struct lfs_attr *attr);
int lfs_attrs_close(lfs_t *lfs, lfs_attrs_t *attrs);
int lfs_attrs_rewind(lfs_t *lfs, lfs_attrs_t *attrs);

Thoughts?

This might lend itself towards other ways of opening the lfs_attrs_t object, such as lfs_attrs_openat, lfs_attrs_openmid, lfs_attrs_openfile, etc.

Support opening handle to directory

We don't really want to do this, since it loses the type info that makes it impossible to "write" to a dir object at compile time.

I'm curious if the hypothetical dir-relative openat APIs would solve most of the cases where path-based lfs_setattr/lfs_getattr are unwieldy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs minor version new functionality only allowed in minor versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants