fs_mark dummyfs support #68

adamdebek · 2023-10-05T14:27:21Z

Description

Motivation and Context

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: (list targets here).

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
I will merge this PR by myself when appropriate.

fs_mark/patch/makefile.patch

github-actions · 2023-10-05T14:45:19Z

Unit Test Results

7 958 tests ±0 7 416 ✅ ±0 41m 5s ⏱️ - 2m 3s
470 suites ±0 542 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 6c64d4e. ± Comparison against base commit 1994dc6.

♻️ This comment has been updated with latest results.

adamdebek · 2023-11-21T17:29:27Z

To see tests results go to: phoenix-rtos/phoenix-rtos-project#802

fs_mark/patch/makefile.patch

JIRA: RTOS-997

JIRA: CI-315

build.sh

Darchiv

I have left a few comments. Passing CPPFLAGS and differentiating targets by the NOMMU define seems unnecessary. There are many fine-grained POSIX defines to use and maybe feature test macros could be employed in the future.

Darchiv · 2025-02-13T16:52:30Z

fs_mark/build.sh

-FS_MARK_VER="3.3"
-FS_MARK=fs_mark-${FS_MARK_VER}
+VER="3.3"
+FS_MARK="fs_mark-$VER"
 FS_MARK_COMMIT="2628be58146de63a13260ff64550f84275556c0e"
 PKG_URL="https://github.com/josefbacik/fs_mark/archive/${FS_MARK_COMMIT}.tar.gz"
 PKG_MIRROR_URL="https://files.phoesys.com/ports/${FS_MARK}.tar.gz"


There are a few utility functions (and variables from build.sh) available for ports. Take a look:
https://github.com/phoenix-rtos/phoenix-rtos-ports/blob/7c6793d75d5b3e92e309a232c7d37ca893035aec/build.subr

Example usage:

phoenix-rtos-ports/lua/build.sh

Line 13 in 7c6793d

b_port_download "https://www.lua.org/ftp/" "${archive_filename}"

You can use them now if you want or leave that for later (all ports will be refactored some day).

Darchiv · 2025-02-13T17:11:47Z

fs_mark/patches/fs_mark.patch

+ 	do_run(my_pid);
+
+#ifdef NOMMU
+        memset(names, 0, sizeof(struct name_entry) * num_files);


Why is this needed on NOMMU targets? Doesn't the "work thread" exit after returning from thread_work()?

fs_mark can run multiple loops with each calling thread_work(), so I had to reinitialize this array back to zero's after each loop

Darchiv · 2025-02-13T17:13:05Z

fs_mark/patches/fs_mark.patch

+ 	 */
+ 	df_full = get_df_full(child_tasks[0].test_dir);
+
+	usleep(100000);


Why is this needed? Is this related to console output being interleaved with something else?

I had a problem with parts of logs being lost, so I used this sleep just for now to make it work.

Darchiv · 2025-02-13T17:23:26Z

fs_mark/patches/fs_mark_h.patch

+	#undef MAX_NAME_PATH
+
+	#define MAX_IO_BUFFER_SIZE (1024 * 32)     /* Max write buffer size is 32KB */
+	#define MAX_NAME_PATH      (200)           /* Length of the pathname before the leaf */


I guess that you are trying to avoid huge static allocations on low-RAM targets. NOMMU option itself doesn't imply that the target device has a tiny pool of RAM.

I think the most flexible solution is to use calloc(), as is the case with struct name_entry *names. The behavior would remain the same (use DEFAULT_IO_SIZE==(16 * 1024) bytes), but the problem of static allocation vanishes. Also, it seems to be a simple fix: change io_buffer into a pointer and replace memset(io_buffer, 0, io_buffer_size); with a call to calloc(). Freeing the memory is not of concern, because the author already ignores that part (it would be important if "worker threads" used threads instead of forked children).

When it comes to MAX_NAME_PATH and FILENAME_SIZE, you can use defines from <limits.h>: PATH_MAX and NAME_MAX. They are defined per-architecture and seem to be perfect for this use case. (See: libphoenix/include/arch/*/limits.h)

Darchiv · 2025-02-13T20:23:52Z

fs_mark/patches/fs_mark_h.patch

+ int	do_fill_fs = 0;				/* Run until the file system is full  */
+ int	verbose_stats = 0;		    	/* Print complete stats for each system call */
+-char 	log_file_name[PATH_MAX] = "fs_log.txt"; /* Log file name for run */
+char 	log_file_name[PATH_MAX - 16] = "fs_log.txt"; /* Log file name for run, reserve space for suffix */


You subtract 16 to accommodate for PID suffix appended in a few places in the source file? If so, please include that information in the comment to make it clear why it is changed (i.e. that fs_mark is flaky and you fix their own bugs) and note that there is
strncpy(log_file_name, optarg, PATH_MAX);
in -l option handling code which also has to be changed.

Darchiv · 2025-02-13T20:32:26Z

fs_mark/patches/fs_mark.patch

+
+#ifdef NOMMU
+	#define FORK vfork
+	file_count = 0;


Why is file_count = 0 needed on NOMMU targets?

names and file_count are two variables that I need to reset after each loop, because NOMMU use vfork() to run thread_work()

Darchiv · 2025-02-13T21:03:54Z

fs_mark/patches/fs_mark.patch

+
+ 	for (i = 0; i < num_threads; i++) {
+-		if ((child_tasks[i].child_pid = fork()) == -1) {
+		if ((child_tasks[i].child_pid = FORK()) == -1) {


Using vfork() actually defeats the whole purpose of running fs_mark with multiple "child tasks", i.e. concurrent operations on files. vfork(), as opposed to fork(), causes the parent to suspend execution until its vforked child exits or execs, so every "child task" is executed sequentially.

I don't see any test commands (phoenix-rtos/phoenix-rtos-tests#233) to use more than one thread, so maybe this can be left unimplemented for now on NOMMU targets. fs_mark could use actual threads (via pthreads) instead of forking, but there are so many globals that refactoring would be quite cumbersome.

Make it work with threads instead of processes on NOMMU can be a tedious task, so I wanted to make it run with just a 1 thread.

github-actions bot reviewed Oct 5, 2023

View reviewed changes

fs_mark/patch/makefile.patch Outdated Show resolved Hide resolved

adamdebek force-pushed the adamdebek/fs_mark branch from 47236c3 to 737efe4 Compare October 6, 2023 11:07

adamdebek force-pushed the adamdebek/fs_mark branch 5 times, most recently from c31e844 to 9ca9a4a Compare October 31, 2023 13:36

adamdebek force-pushed the adamdebek/fs_mark branch 10 times, most recently from c67a862 to c7349d8 Compare November 7, 2023 14:15

adamdebek force-pushed the adamdebek/fs_mark branch 2 times, most recently from 8ba5992 to 62d697a Compare November 21, 2023 17:17

adamdebek changed the title ~~fs_mark fixes~~ fs_mark dummyfs support Nov 21, 2023

adamdebek force-pushed the adamdebek/fs_mark branch from 62d697a to f84af5d Compare November 21, 2023 17:36

github-actions bot reviewed Nov 21, 2023

View reviewed changes

fs_mark/patch/makefile.patch Outdated Show resolved Hide resolved

adamdebek force-pushed the adamdebek/fs_mark branch from f84af5d to e153701 Compare November 21, 2023 17:38

adamdebek marked this pull request as ready for review November 22, 2023 09:16

adamdebek requested review from nalajcie and damianloew November 22, 2023 09:16

adamdebek added 2 commits January 24, 2025 14:14

build.sh: export CPPFLAGS taken from make to ports

26295ff

JIRA: RTOS-997

fs_mark: enable on NOMMU architectures

6c64d4e

JIRA: CI-315

adamdebek force-pushed the adamdebek/fs_mark branch from e153701 to 6c64d4e Compare January 24, 2025 13:20

github-actions bot reviewed Jan 24, 2025

View reviewed changes

build.sh Show resolved Hide resolved

Darchiv reviewed Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs_mark dummyfs support #68

fs_mark dummyfs support #68

adamdebek commented Oct 5, 2023 •

edited

Loading

github-actions bot commented Oct 5, 2023 •

edited

Loading

adamdebek commented Nov 21, 2023

Darchiv left a comment

Darchiv Feb 13, 2025

Darchiv Feb 13, 2025

adamdebek Feb 14, 2025

Darchiv Feb 13, 2025

adamdebek Feb 14, 2025

Darchiv Feb 13, 2025

Darchiv Feb 13, 2025

Darchiv Feb 13, 2025

adamdebek Feb 14, 2025

Darchiv Feb 13, 2025

adamdebek Feb 14, 2025

fs_mark dummyfs support #68

Are you sure you want to change the base?

fs_mark dummyfs support #68

Conversation

adamdebek commented Oct 5, 2023 • edited Loading

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

github-actions bot commented Oct 5, 2023 • edited Loading

Unit Test Results

adamdebek commented Nov 21, 2023

Darchiv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdebek commented Oct 5, 2023 •

edited

Loading

github-actions bot commented Oct 5, 2023 •

edited

Loading