littlefs-project / littlefs

A little fail-safe filesystem designed for microcontrollers
BSD 3-Clause "New" or "Revised" License
5.16k stars 793 forks source link

File creation freezes #717

Open UfukAKKAYA opened 2 years ago

UfukAKKAYA commented 2 years ago

Hi dear littlefs developers.

I am creating a file in a folder. I am writing data to this file and I am able to close the file successfully. But my code sometimes freezes on "lfs_dir_orphaningcommit" function while creating the file.


uint8_t  lfs_read_buf[256];
uint8_t  lfs_prog_buf[256];

// configuration of the filesystem is provided by this struct
const struct lfs_config cfg = {
    // block device operations
    .read  = user_provided_block_device_read,
    .prog  = user_provided_block_device_prog,
    .erase = user_provided_block_device_erase,
    .sync  = user_provided_block_device_sync,

    // block device configuration
    .read_size = 256,
    .prog_size = 256,
    .block_size = 4096,
    .block_count = 8192,
    .cache_size = 256,
    .lookahead_size = 256,
    .block_cycles = 500,

   .read_buffer = lfs_read_buf,
   .write_buffer = lfs_prog_buf,

};
int fileCreate(char *fileName, uint8_t *data, int len)
{

    int err = lfs_file_open(&lfs, &file, "../123456789/file1234.f", LFS_O_RDWR | LFS_O_CREAT);
    if(err) 
        return err;

    err = lfs_file_rewind(&lfs, &file); 
    if(err) 
        return err;

    err = lfs_file_write(&lfs, &file, data, len);
    if(err) 
        return err;

    return lfs_file_close(&lfs, &file);
}

File Create


fileCreate("../123456789/file1234.f", writeBuf, strlen(writeBuf));

Code in infinite loop


static int lfs_dir_orphaningcommit(lfs_t *lfs, lfs_mdir_t *dir,
        const struct lfs_mattr *attrs, int attrcount) {
    // check for any inline files that aren't RAM backed and
    // forcefully evict them, needed for filesystem consistency
    for (lfs_file_t *f = (lfs_file_t*)lfs->mlist; f; f = f->next) {
        if (dir != &f->m && lfs_pair_cmp(f->m.pair, dir->pair) == 0 &&
                f->type == LFS_TYPE_REG && (f->flags & LFS_F_INLINE) &&
                f->ctz.size > lfs->cfg->cache_size) {
            int err = lfs_file_outline(lfs, f);
            if (err) {
                return err;
            }

            err = lfs_file_flush(lfs, f);
            if (err) {
                return err;
            }
        }
    }
    .
    . 
    .

}

I will be glad if you help.

UfukAKKAYA commented 2 years ago

Debug View

Capture

thuan1091996 commented 2 years ago

I'm having the same issue

midiwidi commented 1 year ago

I'm having the same problem.

static int lfs_dir_orphaningcommit(lfs_t *lfs, lfs_mdir_t *dir,
        const struct lfs_mattr *attrs, int attrcount) {
    // check for any inline files that aren't RAM backed and
    // forcefully evict them, needed for filesystem consistency
    for (lfs_file_t *f = (lfs_file_t*)lfs->mlist; f; f = f->next)

f->next points back to f and therefore we are stuck in an endless loop.

thuan1091996 commented 1 year ago

I'm having the same problem.

static int lfs_dir_orphaningcommit(lfs_t *lfs, lfs_mdir_t *dir,
        const struct lfs_mattr *attrs, int attrcount) {
    // check for any inline files that aren't RAM backed and
    // forcefully evict them, needed for filesystem consistency
    for (lfs_file_t *f = (lfs_file_t*)lfs->mlist; f; f = f->next)

f->next points back to f and therefore we are stuck in an endless loop.

I spotted that when opening the same file multiple times, then this issue came in

midiwidi commented 1 year ago

I came back here to report exactly that. Debugging the problem today revealed that I had synced the file but forgot to close it. When I opened it again I got "Assertion at ../littlefs/lfs.c:5478"

image

and on the next sync I ran into the problem with the endless loop described above.

demetriopitasi commented 9 months ago

We are too experiencing issues with lfs_dir_orphaningcommit too.

We're still analyzing the issue, but from a preliminary analysis it seems to be possible to call the lfs_pair_cmp within lfs_dir_orphaningcommit with f->m.pair set to NULL resulting in an hard fault.

static int lfs_dir_orphaningcommit(lfs_t *lfs, lfs_mdir_t *dir,
        const struct lfs_mattr *attrs, int attrcount) {
    // check for any inline files that aren't RAM backed and
    // forcefully evict them, needed for filesystem consistency
    for (lfs_file_t *f = (lfs_file_t*)lfs->mlist; f; f = f->next) {
        if (dir != &f->m && lfs_pair_cmp(f->m.pair, dir->pair) == 0 &&

define LFS_VERSION 0x00020005

define LFS_VERSION_MAJOR (0xffff & (LFS_VERSION >> 16))

define LFS_VERSION_MINOR (0xffff & (LFS_VERSION >> 0))

We're currently testing the following workaround

static inline int lfs_pair_cmp(
        const lfs_block_t paira[2],
        const lfs_block_t pairb[2]) {

    if(paira==NULL||pairb==NULL)
        return 0;

    return !(paira[0] == pairb[0] || paira[1] == pairb[1] ||
             paira[0] == pairb[1] || paira[1] == pairb[0]);
}

#ifndef LFS_READONLY
static inline bool lfs_pair_sync(
        const lfs_block_t paira[2],
        const lfs_block_t pairb[2]) {

    if(paira==NULL||pairb==NULL)
        return 0;

    return (paira[0] == pairb[0] && paira[1] == pairb[1]) ||
           (paira[0] == pairb[1] && paira[1] == pairb[0]);
}
#endif
geky commented 9 months ago

Hi all, thanks for opening/appending this issue.

This is a symptom of unbalanced open/close calls. Though it should be caught earlier than lfs_dir_orphaningcommit if you have asserts enabled.

littlefs uses an invasive linked-list to keep track of open files. Anytime open is called twice on the same file, littlefs tries to enroll the file in the linked-list twice, but this doesn't work since there's only one next pointer. Things break, and usually the linked-list ends up pointing to itself, creating an infinite loop.

The key thing is that every successful open call needs to be followed by a close call. This is to keep the linked-list up to date, but also for freeing any malloced resources.

One tricky thing is that this is a requirement even if a later file operation fails. So the example posted by @UfukAKKAYA should probably be written like the following:

int fileCreate(char *fileName, uint8_t *data, int len)
{
    int err = lfs_file_open(&lfs, &file, "../123456789/file1234.f", LFS_O_RDWR | LFS_O_CREAT);
    if(err)    
        return err;

    err = lfs_file_rewind(&lfs, &file);    
    if(err)    
        goto failed;

    err = lfs_file_write(&lfs, &file, data, len);
    if(err)    
        goto failed;

    return lfs_file_close(&lfs, &file);

failed:;
    lfs_file_close(&lfs, &file);
    return err;
}

Though it's unclear if this is actually @UfukAKKAYA's issue, since an unrelated error is required for the original code to break.

When an error occurs during a file operation a flag is set internally so that lfs_file_close will not touch disk and will not error.


I've created a PR to tighten the bounds of the open-list assert, in case any functions are missing this assert: https://github.com/littlefs-project/littlefs/pull/921, though this still requires asserts to be enabled to do anything.


EDIT: Corrected error handling when lfs_file_open itself errors. If lfs_file_open errors, the file is not "open" and should not be closed.

geky commented 9 months ago

I just realized open-list related issues could also be caused by moving the lfs_file_t struct.

A side-effect of the invasive linked-list is that the lfs_file_t struct can't be moved. Moving the struct causes the linked-list to point to now-invalid memory.

I don't think this will often lead to an infinite loop, but it could cause other problems.

I'm not sure there's a way to check for this in an assert...

Alvipe commented 8 months ago

Hi @geky. First of all, I want to thank you for your amazing work. Coming from SPIFFS, which gets corrupted so easily in the event of sudden power losses, the robustness of LittleFS is truly a blessing.

I am not sure about what you mention that every open call must be followed by a close call, even in case of error. I've tested this, trying to open a non-existent file (which returned a -2 error core, as expected) and then closing it. This has caused the following assert in line 6080 of lfs.c (in lfs_file_close) to fail (my code has the LFS_ASSERT macro defined):

LFS_ASSERT(lfs_mlist_isopen(lfs->mlist, (struct lfs_mlist*)file));

I have tested this with version 2.9.

geky commented 8 months ago

Oh, good catch! I made a mistake.

I am not sure about what you mention that every open call must be followed by a close call, even in case of error.

This should have been phrased as "Every successful open call must be followed by a close call, even if a later file operation errors."

Given that an open call can fail to even allocate resources (LFS_ERR_NOMEM), this is necessary behavior for the API.

Coming from SPIFFS

To be fair, it's a hard problem, and every layer needs to be power-loss aware, both up into the application and down into the bd driver.

geky commented 8 months ago

I've edited the above comment so it should now be correct.