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

Fall in assert when write file size bigger than block size. #6

Closed
zhabl opened this issue Jan 5, 2018 · 12 comments
Closed

Fall in assert when write file size bigger than block size. #6

zhabl opened this issue Jan 5, 2018 · 12 comments

Comments

@zhabl
Copy link

zhabl commented Jan 5, 2018

Hi:
It seems that file data has been read out and copied to 'head'.

line 1162, lfs.c
assert(head >= 2 && head <= lfs->cfg->block_count);

@rocuh
Copy link

rocuh commented Jan 5, 2018

I had a similar issue when the block_size value was not a multiple of the prog_size.

@geky
Copy link
Member

geky commented Jan 11, 2018

Large files should be checked by this test (block size = 512B, file size = 262144B). I think @Roceh is right, block_size needs to be a multiple of the prog_size. I should probably add an assert for this. EDIT: I've added an assert here.

What is the geometry of your block device (read size, prog size, erase size)?

Out of curiousity, what form of storage are you guys running into where the block size isn't a multiple of the program size?

@zhabl
Copy link
Author

zhabl commented Jan 15, 2018

Hi,
This is my config struct,is it right? The device is a W25Q16.

#define LFS_RBUF_SIZE 128 // Read bufffer size
#define LFS_PBUF_SIZE 128 // prog bufer size
#define LFS_LAHD_SIZE 128 // lookahead size

static uint8_t read_buf[LFS_RBUF_SIZE];
static uint8_t prog_buf[LFS_PBUF_SIZE];
static uint8_t lookahead_buf[LFS_LAHD_SIZE/8];
static uint8_t file_buf[LFS_PBUF_SIZE];

static const struct lfs_config cfg =
{
.read = block_read,
.prog = block_prog,
.erase = block_erase,
.sync = device_sync,

.read_size = LFS_RBUF_SIZE,
.prog_size = LFS_PBUF_SIZE,
.block_size = 4096,
.block_count = 512,
.lookahead = LFS_LAHD_SIZE,

.read_buffer = read_buf,
.prog_buffer = prog_buf,
.lookahead_buffer = lookahead_buf,
.file_buffer = file_buf,
};

@geky
Copy link
Member

geky commented Jan 15, 2018

Hi @zhabl, that looks correct.

Unfortunately, I can't seem to reproduce the assert. Running the tests seem happy enough with your config, even after modifying the tests to use static memory:

CFLAGS="-DLFS_READ_SIZE=128 -DLFS_PROG_SIZE=128 -DLFS_BLOCK_SIZE=4096 -DLFS_BLOCK_COUNT=512 -DLFS_LOOKAHEAD=128" make test_dirs test_files

What filesystem operations did you run before you hit the assert? Do you have a code snippet?


That specific assert could be caused by malformed data on the disk. littlefs would notice if data was corrupted during a write, but afterwards littlefs wouldn't notice if something else corrupts the data (such as another program or a rogue filesystem structure).

Have you been able to read/write/erase directly to the W25Q16 chip without successfully?

If may be also worthwhile to try reformatting the disk with lfs_format to get rid of any old filesystem structures left over from previous configurations.

@zhabl
Copy link
Author

zhabl commented Jan 16, 2018

Hi geky
It works ok when file size <= block size. I dump the memory data,and i found that when the file contain
only one block, all data in the block are user data, without any lfs-meta-data(pointers to other block.)
and ,when writing to file , lfs_ctz_extend() is called to alloc a new block, run to this line 1155:

now, head = first block number of the file.
if (i != skips-1) {
err = lfs_cache_read(lfs, rcache, NULL,
head, 4*i, &head, 4);
if (err) {
return err;
}
}

I am not sure what this dose, lfs_cache_read() read the first 4 bytes of the block, then hit the assert.
because this block contian only user data.

Here are part of memory dump of w25q16.

Block 2.
06 00 00 00 23 00 00 00 ff ff ff ff ff ff ff ff
11 08 00 03 07 00 00 00 fc 0f 00 00 31 2e 66 97
8a e1 5f 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff

Block 7.
00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
30 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f
40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f
50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f
60 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e 6f
70 71 72 73 74 75 76 77 78 79 7a 7b 7c 7d 7e 7f

@geky
Copy link
Member

geky commented Jan 18, 2018

Hmm, I'm still not able to reproduce the bug. I put together a quick test that writes a single byte every mount, but it seems to work fine on the setup I have.

I'm wondering if this is a compiler/architecture issue, maybe around the ctz or popcount instructions. What compiler and architecture are you using?

Also what is the hash of littlefs that you are using?

If it's an issue with the ctz or popcount instructions, you could try replacing the the lfs_ctz, lfs_npw2 and lfs_popc functions in lfs_util.h with their software alternatives:

static inline uint32_t lfs_ctz(uint32_t a) {
    uint32_t r = 32;
    a &= -a;
    if (a) r -= 1;
    if (a & 0x0000ffff) r -= 16;
    if (a & 0x00ff00ff) r -= 8;
    if (a & 0x0f0f0f0f) r -= 4;
    if (a & 0x33333333) r -= 2;
    if (a & 0x55555555) r -= 1;
    return r;
}

static inline uint32_t lfs_npw2(uint32_t a) {
    uint32_t r = 0;
    uint32_t s;
    s = (a > 0xffff) << 4; a >>= s; r |= s;
    s = (a > 0xff  ) << 3; a >>= s; r |= s;
    s = (a > 0xf   ) << 2; a >>= s; r |= s;
    s = (a > 0x3   ) << 1; a >>= s; r |= s;
    return r | (a >> 1);
}

static inline uint32_t lfs_popc(uint32_t a) {
    a = a - ((a >> 1) & 0x55555555);
    a = (a & 0x33333333) + ((a >> 2) & 0x33333333);
    return (((a + (a >> 4)) & 0xf0f0f0f) * 0x1010101) >> 24;
}

Here's some explanation on what's going on.

The ctz skip-list looks like this:

.--------.  .--------.  .--------.  .--------.  .--------.  .--------.
| data 0 |<-| data 1 |<-| data 2 |<-| data 3 |<-| data 4 |<-| data 5 |<-file
|        |<-|        |--|        |<-|        |--|        |  |        |
|        |<-|        |--|        |--|        |--|        |  |        |
'--------'  '--------'  '--------'  '--------'  '--------'  '--------'

The number of pointers is based on the equation ctz(block number) + 1 here. Notice the first block has no pointers. If you can get the value of skips here in a debugger that may help.

In the code snippet you posted, we should be appending block 1, so skips should be the value 1. i should be 0, so that if statement should not execute. If it is executing on block 1 that is a bug.

Since the logic is working on a different setup, I suspect it may be an issue with the compiler/architecture specific instructions and I'd be interested to know which compiler/architecture you're on.

@zhabl
Copy link
Author

zhabl commented Jan 22, 2018

Yes!,it works after i replaced that three functions. You are right,this is an issue of compiler.
The compiler is Keil MDK-ARM V5, the mcu is STM32F103 with Cortex-M3 core.
Next i will do some power loss test . Thank you geky.

@geky
Copy link
Member

geky commented Jan 22, 2018

Oh that's interesting. Did you have to change the implementations of lfs_ctz/lfs_popc before? Or did Keil compile with __builtin_ctz and __builtin_popcount?

@zhabl
Copy link
Author

zhabl commented Jan 23, 2018

I have not change the implementations of lfs_ctz/lfs_popc, Keil compiler provide compatibility of GNU build-in functions, may be not fully compatible. A software implementation of lfs_ctz/lfs_popc is needed.
and re-definations of malloc/free/assert functions for other platform.
#define LFS_MALLOC
#define LFS_FREE
#define LFS_ASSERT

@geky
Copy link
Member

geky commented Jan 23, 2018

Could you not provide malloc/free/assert through their standard names?

@zhabl
Copy link
Author

zhabl commented Jan 23, 2018

Yes i can provide these functions throuth standard names , but they should have 'private' names , easy to turn off assertion when release.

@geky
Copy link
Member

geky commented Jan 25, 2018

That's a good point. I will add wrappers for assert/malloc/free when I get a chance.

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

No branches or pull requests

3 participants