fstore: hold more than 64 files #4680

matthewfala · 2022-01-25T02:00:43Z

Signed-off-by: Matthew Fala falamatt@amazon.com

FStore - Store More Than 64 Files

FStore is used to store data locally in files. There is a limitation of 64 files which FStore can store. This is due to a configuration in chunkio that limits memory mapped files to 64. It would be nice to have more than 64 files held/tracked in FStore at a given time.

The easier way to do this would be to increasing the chunkio memory map limit to some arbitrary large number. However this seems to be against the design put forth by chunkio where a small number of files should be memory mapped (or up) and let down when not needed

This PR explores the alternate route of adding and removing mapped files strategically to keep the number of memory mapped files always <= 64

This PR implements a caching system for memory mapped files. Files that are least frequently accessed through fstore are let down after the 64 file limit has been reached. These files are brought up by fstore when demanded.

The cache is a linked list of memory mapped "up" files which, on initialization, is initialized to a list of files brought up by chunkio. This list is maintained throughout fstore lifecycle. On file access, the linked list is rearranged to reference the newly accessed file first. On loading the 65th file, the last file in the cache list is put down and the 65th file is brought up, keeping 64 files in memory.

Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

Example configuration file for the change
Debug log output from testing the change

Attached Valgrind output that shows no leaks or memory corruption was found

This problem is impacting clients of S3 plugin who have more than 64 log files that will eventually upload.

Steps to reproduce problem and view solution:

Use fluent-bit.conf

[SERVICE]
    Flush        1
    Grace        30
    Log_Level    info

[INPUT]
    Name             tail
    Path             /container/path/*.txt
    Exclude_Path     *.gz
    Rotate_Wait      15
    Buffer_Chunk_Size 1MB
    Buffer_Max_Size   5MB
    Mem_Buf_Limit     200MB
    Read_from_Head    True
    DB               /container/path/tail-containers-state.db
    DB.Sync          Normal
    Tag_Regex        (?<one>\S*)\/(?<two>\S*)\/(?<three>\S*)
    Tag              <one>/<two>/<three>

[OUTPUT]
    Name                         s3
    Match                        *
    bucket                      my-bucket
    region                       eu-central-1
    total_file_size              1M
    upload_timeout               1m
    s3_key_format                /log-upload-repro$TAG/$UUID
    use_put_object               On

Add 1000 .txt files to /container/path/ directory

[root@ip-10-1-145-174 mnt]# touch my-log-file-0.txt
[root@ip-10-1-145-174 mnt]# for i in {1..1000}; do cp my-log-file-0.txt "my-log-file-$i.txt"; done

Add a line of log in each file

#!/bin/bash  
for filename in *.txt; do
    echo "Logging file name - $filename" >> $filename
done

Before the patch: Errors should be available in fluent-bit logs. S3 will only be able to upload 64 files. The remaining 1000-64 files are not uploaded due to fstore limitations.
After the patch: No errors are shown in the logs. S3 will be able to upload 65+ files. Tested with 1000 and 5000 files. Upload successful. No errors.

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

Attached local packaging test output showing all targets (including any new ones) build.

Documentation

Documentation required for this feature

Backporting

Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

edsiper · 2022-01-26T14:57:29Z

src/flb_fstore.c

    if (delete == FLB_TRUE) {
        cio_stream_delete(stream->stream);
+
+        /* remove stream files from fstore files_up up list */
+        mk_list_foreach(head, &stream->files) {


since the entry fsf will be removed from the list, it will generate a corruption with other linked elements, you have you use mk_list_foreach_safe() instead mk_list_foreach()

files_up_remove removes file from files_up list in fstore. mk_list_foreach enumerates files list in stream. I think mk_list_foreach is okay in this case because files list not modified, though can replace with mk_list_foreach_safe()

https://github.com/fluent/fluent-bit/pull/4680/files/9b3fea6edf755786e2e80311020d5d3a829317d7#diff-d774a9676f644aabb1f4cd06bc9860f329e7505dbc6125bc3df5214448598e84R59

Oh, is this because of multithreading, @edsiper? I didn't consider that.

src/flb_fstore.c

edsiper · 2022-01-26T14:59:40Z

tests/internal/fstore.c

@@ -64,7 +64,7 @@ void cb_all()
    ret = stat(FSF_STORE_PATH "/abc/example.txt", &st_data);
    TEST_CHECK(ret == 0);

-    ret = flb_fstore_file_append(fsf, "fluent-bit\n", 11);
+    ret = flb_fstore_file_append(fs, fsf, "fluent-bit\n", 11);
    TEST_CHECK(ret == 0);



Please extend the fstore.c unit test to try to reproduce the issue you were facing initially

This sound good. Will add.

Added a test!
Get the following errors without the patch:

[2022/02/11 04:47:22] [error] [fstore] [cio file] file is not mmap()ed: abc:files-round_2-65.txt [2022/02/11 04:47:22] [error] [fstore] could not write data to file files-round_2-65.txt fstore.c:172: Check ret == 0... failed fstore.c:177: Check memcmp(out_buf, "fluent-bit-3\n", 13) == 0... failed fstore.c:178: Check out_size == 13... failed fstore.c:169: Check ret == 0... failed [2022/02/11 04:47:22] [error] [fstore] [cio file] file is not mmap()ed: abc:files-round_2-66.txt [2022/02/11 04:47:22] [error] [fstore] could not write data to file files-round_2-66.txt fstore.c:172: Check ret == 0... failed fstore.c:177: Check memcmp(out_buf, "fluent-bit-3\n", 13) == 0... failed fstore.c:178: Check out_size == 13... failed fstore.c:169: Check ret == 0... failed [2022/02/11 04:47:22] [error] [fstore] [cio file] file is not mmap()ed: abc:files-round_2-67.txt [2022/02/11 04:47:22] [error] [fstore] could not write data to file files-round_2-67.txt ... fstore.c:193: Check out_size == 11+13... failed FAILED: 1 of 2 unit tests has failed.

With the patch, the following is output showing success:

[ec2-user@ip-172-31-30-69 bin]$ /home/ec2-user/fala-forks/fluent-bit/build/bin/flb-it-fstore Test all... [2022/02/11 04:50:17] [ info] [fstore] created root path /tmp/flb-fstore ===== FSTORE DUMP ===== - stream: abc abc/example.txt (up) [ OK ] Test many_files... [2022/02/11 04:50:17] [ info] [fstore] created root path /tmp/flb-fstore [2022/02/11 04:50:17] [ info] [fstore] created root path /tmp/flb-fstore [2022/02/11 04:50:17] [ info] [fstore] created root path /tmp/flb-fstore [2022/02/11 04:50:17] [ info] [fstore] created root path /tmp/flb-fstore [ OK ] SUCCESS: All unit tests have passed.

src/flb_fstore.c

matthewfala · 2022-01-27T20:59:55Z

~~@nokute78 Could you take a look? Resolved merge conflicts.~~ Sorry this comment was meant for the base64 pr...

Signed-off-by: Matthew Fala <falamatt@amazon.com>

matthewfala · 2022-02-11T04:41:42Z

lib/chunkio/src/cio_file.c

@@ -358,6 +358,7 @@ static int mmap_file(struct cio_ctx *ctx, struct cio_chunk *ch, size_t size)

        cio_log_debug(ctx, "%s:%s adjusting size OK", ch->st->name, ch->name);
    }
+    cf->alloc_size = size;


@edsiper alloc_size may need to reflect the true size of the file here, not the size rounded up. Trying to append to file does not write through when alloc_size misrepresents the true filesize because file resize not detected.

File resize needed on chunk_down -> chunk_up -> append because chunk_down truncates file extra space, chunk_up restores file with alloc_size misrepresenting true filesize (rounded non-truncated filesize) and append expects the file to be non-truncated size and the write fails.

There are several solution options I tried which all work (the tests fail if none of the following are implemented)

Round up only mmap (implemented)

alloc_size not rounded up and represents true file size

Move alloc_size = size above round up. This keeps mmap length still a multiple of page size, but larger than true filesize

Don't round up alloc_size or mmap (another option -- Preferred but needs confirmation)

Just delete the following ROUND_UP(size, ctx->page_size). mmap length is now the true length of the file.

This seems the most straightforward but I don't know if you want mmap to always allocate in increments of page size for some reason.

Actually resize the file (another option)

A file resize needed at least once before append since file truncated on let down.

Not needed if alloc_size is set since append would detect resize needed and will resize and remap.

Lazy may be better for cases with only read and no append

/* add the following to the if (fs_size > 0) { ... } block */ /* Round up filesize to allow for appends */ size = ROUND_UP(size, ctx->page_size); ret = cio_file_fs_size_change(cf, size);

matthewfala · 2022-02-11T04:57:59Z

Added tests! Merge requires change to chunkio dep. Please see notes above.

Signed-off-by: falamatt@amazon.com <falamatt@amazon.com>

matthewfala · 2022-02-11T18:59:06Z

tests/internal/fstore.c

+    return 0;
+}
+
+void test_chunkio_up_down_up_append()


This test case displays the issue with chunk io append after up > down > up > append.

edsiper · 2022-02-23T00:24:02Z

in order to merge this, please submit the chunkio fix/PR to https://github.com/edsiper/chunkio first, then we do the sync here.

matthewfala · 2022-02-23T23:56:24Z

PR'd to chunkio: fluent/chunkio#80.
Would you please take a look?

matthewfala · 2022-04-25T19:32:03Z

@edsiper, what threadsafety concerns do I need to take into account with fstore? Should we expect multiple threads to simultaneously append/destroy/(other fstore api)? If so, it may help to have the async lock designed here: #5255, so we can lock and unlock fstore instance on usage.

matthewfala · 2022-04-25T19:33:16Z

Please hold off on merging till the conversation on thread safety is settled.

edsiper · 2022-06-20T18:46:23Z

re: threading... each context creator is responsible to protect multiple readers/writers.

edsiper · 2022-06-20T18:58:30Z

@matthewfala is this ready to go besides the threading concerns ?

matthewfala · 2022-08-12T22:56:39Z

Hi @edsiper,

This PR was heavily tested a while ago, however it looks like Leonardo and the team resolved this issues in the following PR using a somewhat simpler method.
https://github.com/fluent/fluent-bit/pull/5469/files

If there are more than 64 files, Leonardo's pr will just open a 65th file via a force, do the operation and close the file.

My PR closes the least recently used file, and opens a new 64th file.

I think we should keep this PR open and switch to it if you find a common use case where file access patterns are biased toward a small set of files, with many infrequently accessed files. In this case, my solution will perform better. In the case where there's just a bunch of infrequently accessed files (only written to once), both solutions perform equally poorly.

matthewfala · 2022-08-12T22:57:30Z

@PettitWesley for visibility

matthewfala requested review from edsiper, fujimotos, koleini and PettitWesley as code owners January 25, 2022 02:00

github-actions bot added the docs-required label Jan 25, 2022

edsiper requested changes Jan 26, 2022

View reviewed changes

edsiper added waiting-for-user Waiting for more information, tests or requested changes and removed docs-required labels Jan 26, 2022

edsiper self-assigned this Jan 26, 2022

matthewfala commented Jan 26, 2022

View reviewed changes

src/flb_fstore.c Outdated Show resolved Hide resolved

matthewfala force-pushed the master-plus-files-fstore branch from 9b3fea6 to 2fd4384 Compare February 10, 2022 01:06

fstore: hold more than 64 files

3a5106d

Signed-off-by: Matthew Fala <falamatt@amazon.com>

matthewfala force-pushed the master-plus-files-fstore branch from 2fd4384 to c01c60a Compare February 11, 2022 04:02

matthewfala commented Feb 11, 2022

View reviewed changes

matthewfala added 2 commits February 11, 2022 18:57

fstore: add tests for many files

d408800

Signed-off-by: falamatt@amazon.com <falamatt@amazon.com>

chunkio: resolve incorrect alloc size on file up

0df0300

Signed-off-by: falamatt@amazon.com <falamatt@amazon.com>

matthewfala force-pushed the master-plus-files-fstore branch from c01c60a to 0df0300 Compare February 11, 2022 18:57

matthewfala commented Feb 11, 2022

View reviewed changes

matthewfala mentioned this pull request Mar 21, 2022

file: allocation size adjustment fluent/chunkio#80

Merged

This was referenced May 17, 2022

1.8: fstore: ensure the chunk is up while modifying it #5468

Closed

fstore: ensure the chunk is up while modifying it #5469

Merged

edsiper requested a review from leonardo-albertovich as a code owner August 14, 2024 17:58

patrick-stephens requested review from pwhelan, patrick-stephens and niedbalski as code owners March 20, 2025 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fstore: hold more than 64 files #4680

fstore: hold more than 64 files #4680

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fstore: hold more than 64 files #4680

Are you sure you want to change the base?

fstore: hold more than 64 files #4680

Uh oh!

Conversation

FStore - Store More Than 64 Files

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!