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

Provide a function for probing for LFS #947

Open
rojer opened this issue Feb 27, 2024 · 12 comments
Open

Provide a function for probing for LFS #947

rojer opened this issue Feb 27, 2024 · 12 comments

Comments

@rojer
Copy link
Contributor

rojer commented Feb 27, 2024

It would be nice to have a function that would probe a device for LFS and return the parameters if found (the lfs_superblock_t structure).

@rojer
Copy link
Contributor Author

rojer commented Feb 27, 2024

naturally, we must at least know the block size.
here's my quick and dirty take: https://gist.github.com/rojer/cf1a99497c1f1e807fe0e44c059daa17
pulled out the relevant part of lfs_rawmount

@geky
Copy link
Member

geky commented Feb 28, 2024

I'm curious, what would this gain over lfs_mount + lfs_fs_stat?

Unless I'm missing something, everything that is in the superblock should be accessible via lfs_fs_stat.

@rojer
Copy link
Contributor Author

rojer commented Feb 28, 2024

is lfs_mount guaranteed to not modify the fs?

the reason for probing is that we want to provide exact disk_version to lfs_mount to avoid upgrading the disk format (we want to preserve compatibility with older firmware for now).

@rojer
Copy link
Contributor Author

rojer commented Feb 28, 2024

there is no runtime read-only mount option, only LFS_READONLY. i guess lfs_mount ro + lfs_fs_stat would be a way.
but also, probing, for example, should be quiet about finding gibberish, since finding something else is a valid expected result.
we also probe to determine which filesystem should be used - if device is upgrading from spiffs, we need to be able to detect it.
so overall, a separate probe api would be useful, i think,

@geky
Copy link
Member

geky commented Feb 28, 2024

is lfs_mount guaranteed to not modify the fs?

Ah yes, lfs_mount involves no erase/prog operations.

In general littlefs has some pretty strong guarantees around not mutating the disk until an actual mutation API is called (lfs_file_open, lfs_remove, lfs_fs_mkconsistent, etc). Though I realize this isn't really documented.

there is no runtime read-only mount option, only LFS_READONLY

I've been thinking of adding a runtime LFS_M_READONLY flag to lfs_mount, but it's low priority since the best API would require a breaking change, and it technically doesn't add any functionality given the above guarantees.

But there's other mount flags that could be useful in the future.

but also, probing, for example, should be quiet about finding gibberish

This is a valid point. We should probably make a failing lfs_mount not log any errors, though this is a bit tricky in practice.

The notorious "Corrupted dir pair" log (here) can probably be downgraded to LFS_DEBUG, though I'm not sure if there's other error logs that can be hit.


I do like the idea and design of the lfs_probe API, but by default I need to be against new APIs unless there are strong reasons for their inclusion, to combat API complexity, bug surface-area, maintenance overhead, etc.

Since this can be implemented on the user's side:

int lfs_probe(lfs_t *lfs, const struct lfs_config *cfg,
        struct lfs_fs_stat *fsstat) {
    int err = lfs_mount(lfs, cfg);
    if (err) {
        return err;
    }

    err = lfs_fs_stat(lfs, fsstat);
    if (err) {
        return err;
    }

    err = lfs_unmount(lfs);
    if (err) {
        return err;
    }

    return 0;
}

I'm also curious if there's any prior art. From what I understand most filesystems rely on lfs_mount failure for querying if a filesystem exists, for similar reasons to why there is no fexists like function`.


the reason for probing is that we want to provide exact disk_version to lfs_mount to avoid upgrading the disk format (we want to preserve compatibility with older firmware for now).

I'm probably missing something, but would this be more easily accomplished by setting cfg.disk_version = 0x00020000?

If configured this way, littlefs should error with LFS_ERR_INVAL if it encounters an lfs2.1 disk version (here, oh look another error log)

@rojer
Copy link
Contributor Author

rojer commented Feb 28, 2024

setting cfg.disk_version = 0x00020000

yes, but that would hard-code the version and won't be as flexible during migration.
right now we mount with cfg.disk_version = probed_version.
you may or may not remember that we ship new filesystem images with our fw, that on its first boot mounts the old fs and copies over the volatile state. thus new code must support old disk format, obviously.
however, in order to support rollback, for the transitional period old code must also be able to mount new fs: to perform reverse migration when old code boots with old initial fs image and mounts used-to-be-new fs to copy over the state.

thus our disk_ver migration plan is:

  1. start shipping code that supports 2.1 but ship 2.0 filesystem images.
  2. start shipping 2.1 images. at this point we won't be able to roll back anymore.

we implemented (1) about 6 months ago but haven't done (2) yet, so even our latest firmware still comes with disk_ver 2.0 images. when we update out image generation to 2.1 (probably in a month or so), we'll still maintain 6+ month rollback compatibility window because of the version probing behavior. had we hard-coded cfg.disk_version = 0x00020000 in (1), we'd have a hard cut-off in terms of rollback since they'd refuse to mount 2.1 images.

@geky
Copy link
Member

geky commented Feb 29, 2024

Ah I see, thanks for clarifying.

Yeah, I can't think of a better solution with littlefs's current API.

If/when we have mount flags, adding LFS_M_NOUPGRADE (or more likely requiring LFS_M_UPGRADE to upgrade) would make a lot of sense.

@rojer
Copy link
Contributor Author

rojer commented Feb 29, 2024

can't mount flags be added to struct lfs_config?

@geky
Copy link
Member

geky commented Feb 29, 2024

That's a good point, it would be inconsistent with lfs_file_opencfg, but there's nothing stopping us from adding flags to struct lfs_config.

I was also considering adding lfs_mountcfg (with flags) and deprecating lfs_mount, which would be a bit more disruptive but at least not an API breakage.

But there are some... more disruptive changes bubbling down the pipeline. If we're going to have major API breakages somewhat soon I think trying to add flags to lfs_mount is the way to go.

If things take longer than expected (as they usually do) I'll consider adding LFS_M_NOUPGRADE to struct lfs_config. It is a nice way to move things in the right direction.

@geky
Copy link
Member

geky commented Feb 29, 2024

thus our disk_ver migration plan is:

  1. start shipping code that supports 2.1 but ship 2.0 filesystem images.
  2. start shipping 2.1 images. at this point we won't be able to roll back anymore.

Just thinking out loud. Another option might be to try mounting first with disk_version=0x00020000, and if that fails try mounting with disk_version=0x00020001.

Not sure if this is better or worse than mount->fs_stat->unmount->mount.

@rojer
Copy link
Contributor Author

rojer commented Feb 29, 2024

trying to add flags to lfs_mount is the way to go.

but we'd need to store them anyway and presumably check? e.g. LFS_M_READONLY would need to be consulted on every mutating operation. might as well just add them to struct lfs_config. but anyway, up to you.

so, my wishlist for flags:

  • LFS_M_READONLY - extra safety when mounting old filesystem to migrate from, we don't want to mutate anything.
  • LFS_M_NOUPGRADE - we'd use this in general, to preserve whatever disk version the image was created with.
  • LFS_M_SILENT - silence errors, perhaps? when using mount to probe an unknown device, we don't want any screams in the logs.

@geky
Copy link
Member

geky commented Mar 8, 2024

but we'd need to store them anyway and presumably check? e.g. LFS_M_READONLY would need to be consulted on every mutating operation. might as well just add them to struct lfs_config.

Ah sorry, this is in the context of some other changes that move away from needing struct lfs_config to be long-lived. The theory being cases where you can benefit from struct lfs_config being readonly are better handled with compile-time configuration. Otherwise struct lfs_config usually ends up in RAM anyways.

LFS_M_SILENT - silence errors, perhaps? when using mount to probe an unknown device, we don't want any screams in the logs.

It's not very clear what the best solution is, but I think we may just want to treat errors during lfs_mount as unexceptional. Similar to lfs_file_open. So downgrade them to LFS_DEBUG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants