littlefs: Add lfs_file_getattr and lfs_file_setattr

[xiaoxiang781216]

which are counterpart of lfs_getattr and lfs_setattr but work with lfs_file_t not a file path

[xiaoxiang781216]

@geky I think the ci error isn't related to this patch:

Secret source: None
Prepare workflow directory
Prepare all required actions
Getting action download info
Error: This request has been automatically failed because it uses a deprecated version of `actions/upload-artifact: v2`. Learn more: https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/

[geky]

Hi @xiaoxiang781216, thanks for creating a PR.

@geky I think the ci error isn't related to this patch:

You are correct, sorry about that. GitHub introduced some breaking changes somewhat recently and it's made things a bit of a mess. This would need to be rebased on to the devel branch to fix CI.

---

This isn't the first time lfs_file_getattr and lfs_file_setattr have been proposed (#580, #636). I understand the motivation for it, but unfortunately I don't see how it can fit in littlefs's data model.

In littlefs, writes to a file are not committed until lfs_file_sync or lfs_file_close. The data may be written to disk, but the metadata is not updated until one of these functions is called. This prevents partially written data from appearing on disk after a powerloss, and helps write safe powerloss-resilient applications.

This gets tricky for metadata, where we can't rely on on-disk copy-on-write data structures. The current assumption is that a file's metadata fits in RAM, with custom attributes being backed by the user-provided lfs_file_config.attrs array.

Calling lfs_dir_commit in lfs_file_setattr is enticing, but unfortunately breaks this model by writing attrs to disk immediately, potentially breaking applications if power is lost.

The only way I could see lfs_file_setattr working in this model is if it appended to a RAM-backed array, but this would require either dynamic memory or the user to providing lfs_file_config.attrs. And if users are already providing lfs_file_config.attrs, I think it's easier to manipulate custom attrs through the array directly...

[crafcat7]

Hello, I'm glad to see you reply to this PR. I have two questions about it:

1. Suppose we distinguish the problem between lfs_file_getattr and lfs_file_setattr. I understand that lfs_file_getattr is reasonable at present, but the implementation of lfs_file_setattr does not meet the design expectations of littlefs. In this case, can we first introduce lfs_file_getattr as a new interface to littlefs as another way to obtain file attributes?

Calling lfs_dir_commit in lfs_file_setattr is enticing, but unfortunately breaks this model by writing attrs to disk immediately, potentially breaking applications if power is lost.

2. Combining the first point, can we adjust the implementation of lfs_file_setattr to implement a RAM array to store this attribute field, and let it actually save during sync / close to solve the problem?

[geky]

1. Suppose we distinguish the problem between lfs_file_getattr and lfs_file_setattr. I understand that lfs_file_getattr is reasonable at present, but the implementation of lfs_file_setattr does not meet the design expectations of littlefs. In this case, can we first introduce lfs_file_getattr as a new interface to littlefs as another way to obtain file attributes?

Yes! I'm not opposed to it. lfs_file_getattr may be quite useful on its own.

I've considered adding it myself, but there is one extremely niche corner case that has made me hesitate. Consider removing a file with attrs:

lfs_file_open(&lfs, &file, "file.txt", LFS_O_RDONLY) => 0;
lfs_remove(&lfs, "file.txt") => 0;
lfs_file_getattr(&lfs, &file, 't', buffer, sizeof(buffer)) => ?;

If these attrs live on disk, lfs_file_getattr will fail. The concerning thing is that this is different from POSIX filesystems, where getxattr would succeed up until you close the file. This is because most POSIX filesystems use inodes and littlefs doesn't, which is a complicated design tradeoff that creates a lot of problems.

Is this practically an issue? To be honest I'm not sure.

On one hand it risks burning users with niche POSIX incompatibilities. On the other hand lfs_file_getattr may be useful enough that this unexpected behavior is worth it. It's hard to know.

So, I'm not opposed to it, but that's why I've held off so far.

--

Though before bringing it in, lfs_file_getattr would need some work:

• lfs_file_getattr should also check lfs_file_config.attrs.

  We probably want user-specified attrs to take priority over on-disk attrs.

• If we add lfs_file_getattr, we should probably also add lfs_dir_getattr.

  On one hand, lfs_dir_getattr is much less useful. On the other hand, it should be basically the same function.

• New APIs need new tests.

  See tests/test_attrs.toml, adding tests here may be a good start.

  This can be extremely tedious, but is also extremely valuable.

Calling lfs_dir_commit in lfs_file_setattr is enticing, but unfortunately breaks this model by writing attrs to disk immediately, potentially breaking applications if power is lost.

2. Combining the first point, can we adjust the implementation of lfs_file_setattr to implement a RAM array to store this attribute field, and let it actually save during sync / close to solve the problem?

Yes, this would satisfy littlefs's powerloss model. But it would the be the only API that requires malloc to work.

My original thinking was that lfs_file_config.attrs would be sufficient to allow this to be done on the user's side of things, but thinking about it now this doesn't allow dynamically adding new attrs. I'm not sure such an API is possible but I'll have to think about it...

At least lfs_file_config.attrs should be sufficient if you have a known-set of system-wide attrs.

If we do add a malloc-backed lfs_file_setattr, I think it would at least need to be opt-in instead of opt-out. Requiring some define like LFS_FILE_SETATTR or LFS_DYN before being available.

A malloc-backed lfs_file_setattr would be pretty bad in terms of heap fragmentation, though I realize most of the systems that would use this probably wouldn't really care.

[crafcat7]

Though before bringing it in, lfs_file_getattr would need some work:

• lfs_file_getattr should also check lfs_file_config.attrs.
  We probably want user-specified attrs to take priority over on-disk attrs.

Thanks for your comments. I split out lfs_file_getattr into #1047 and improved it based on your suggestion.

I've considered adding it myself, but there is one extremely niche corner case that has made me hesitate. Consider removing a file with attrs:

lfs_file_open(&lfs, &file, "file.txt", LFS_O_RDONLY) => 0;
lfs_remove(&lfs, "file.txt") => 0;
lfs_file_getattr(&lfs, &file, 't', buffer, sizeof(buffer)) => ?;

If these attrs live on disk, lfs_file_getattr will fail. The concerning thing is that this is different from POSIX filesystems, where getxattr would succeed up until you close the file. This is because most POSIX filesystems use inodes and littlefs doesn't, which is a complicated design tradeoff that creates a lot of problems.

Is this practically an issue? To be honest I'm not sure.

On one hand it risks burning users with niche POSIX incompatibilities. On the other hand lfs_file_getattr may be useful enough that this unexpected behavior is worth it. It's hard to know.

So, I'm not opposed to it, but that's why I've held off so far.

I also found this problem during testing. Are there differences in the way different file systems handle some details? Perhaps this behavior is acceptable?🤔

• If we add lfs_file_getattr, we should probably also add lfs_dir_getattr.
  On one hand, lfs_dir_getattr is much less useful. On the other hand, it should be basically the same function.

As for the implementation of lfs_dir_getattr, I don't have a good idea to implement it yet. Maybe I will have inspiration in the future, or someone will implement it.😢

Calling lfs_dir_commit in lfs_file_setattr is enticing, but unfortunately breaks this model by writing attrs to disk immediately, potentially breaking applications if power is lost.

2. Combining the first point, can we adjust the implementation of lfs_file_setattr to implement a RAM array to store this attribute field, and let it actually save during sync / close to solve the problem?

Yes, this would satisfy littlefs's powerloss model. But it would the be the only API that requires malloc to work.

My original thinking was that lfs_file_config.attrs would be sufficient to allow this to be done on the user's side of things, but thinking about it now this doesn't allow dynamically adding new attrs. I'm not sure such an API is possible but I'll have to think about it...

At least lfs_file_config.attrs should be sufficient if you have a known-set of system-wide attrs.

If we do add a malloc-backed lfs_file_setattr, I think it would at least need to be opt-in instead of opt-out. Requiring some define like LFS_FILE_SETATTR or LFS_DYN before being available.

A malloc-backed lfs_file_setattr would be pretty bad in terms of heap fragmentation, though I realize most of the systems that would use this probably wouldn't really care.

Thank you for your idea. I will consider whether there is a simpler and more efficient way to implement the modification of lfs_file_setattr.