Re: [LTP] [PATCH] fs_fill: Fix test when running on a 256 CPU+ machine

Richard Palethorpe
2023-03-07T12:48:46Z

Hello,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Andrei,
>
>> The fs_fill test runs a fill test on all the supported filesystems. One
>> of them being vfat. This filesystem is configured dynamically or through
>> flags/arguments for its file allocation table type (12/16/32).
>
>> The size of the test device (which is a loop-mounted fs) is 300MB. When not
>> instructed, mkfs will "automatically select between 12, 16 and 32 bit,
>> whatever fits better for the filesystem size"[1]. In the case of a 300Mb that
>> would end up as FAT16.
> Interesting. BTW we plan to change 300 MB to minimal filesystem which would fit
> to all existing tests (255 MB was for Btrfs, 300 MB was for XFS, but there might
> be minimal systems which can use vfat, ext4, ... with smaller resources, e.g.
> 16 MB for filesystem). Therefore I wonder what is minimal reasonable required
> size for vfat. i.e. what MB is required for FAT32? (I guess we don't want to
> check FAT12 or FAT16).
>
>> This is linked with another configuration that is the actual impact on
>> this issue: the maximum number of directories in the root of the
>> filesystem. FAT12 and FAT16 use a special system region: "Root Directory
>> Region". And by default (there is also an argument to configure this at
>> mkfs-time) this ends up being 256 when no custom arguments are provided.
>
>> On the other hand, the test sets up the filesystem with a
>> "tst_ncpus_conf + 2" number of directories in the test filesystem's root
>> which can break the limit explained above on systems with a number of
>> CPUs that would amount above the limit.
>
>> This change addresses this situation by using a subdirectory in the test
>> filesystem which is not subject to this limit.
>
>> Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com>
>> ---
>> testcases/kernel/fs/fs_fill/fs_fill.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>> diff --git a/testcases/kernel/fs/fs_fill/fs_fill.c b/testcases/kernel/fs/fs_fill/fs_fill.c
>> index 2027b6df1..dd8ad2935 100644
>> --- a/testcases/kernel/fs/fs_fill/fs_fill.c
>> +++ b/testcases/kernel/fs/fs_fill/fs_fill.c
>> @@ -18,6 +18,7 @@
>> #include "tst_test.h"
>
>> #define MNTPOINT "mntpoint"
>> +#define THREADS_SUBDIR "subdir"
>
>> static volatile int run;
>> static unsigned int nthreads;
>> @@ -99,9 +100,13 @@ static void setup(void)
>> nthreads = tst_ncpus_conf() + 2;
>> workers = SAFE_MALLOC(sizeof(struct worker) * nthreads);
>
>> + // Avoid creating the thread directories in the root of the filesystem
>> + // to not hit the root entries limit on a FAT16 filesystem.
>> + SAFE_MKDIR(MNTPOINT "/" THREADS_SUBDIR, 0700);
>> +
>> for (i = 0; i < nthreads; i++) {
>> snprintf(workers[i].dir, sizeof(workers[i].dir),
>> - MNTPOINT "/thread%i", i + 1);
>> + MNTPOINT "/" THREADS_SUBDIR "/thread%i", i + 1);
>> SAFE_MKDIR(workers[i].dir, 0700);
>> }
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
> If you don't mind, I suggest to merge slightly changed patch.
>
> Kind regards,
> Petr
>
> +++ testcases/kernel/fs/fs_fill/fs_fill.c
> @@ -18,6 +18,7 @@
> #include "tst_test.h"
>
> #define MNTPOINT "mntpoint"
> +#define THREADS_DIR MNTPOINT "/subdir"
>
> static volatile int run;
> static unsigned int nthreads;
> @@ -99,9 +100,15 @@ static void setup(void)
> nthreads = tst_ncpus_conf() + 2;
> workers = SAFE_MALLOC(sizeof(struct worker) * nthreads);
>
> + /*
> + * Avoid creating the thread directories in the root of the filesystem
> + * to not hit the root entries limit on a FAT16 filesystem.
> + */
> + SAFE_MKDIR(THREADS_DIR, 0700);
> +
> for (i = 0; i < nthreads; i++) {
> snprintf(workers[i].dir, sizeof(workers[i].dir),
> - MNTPOINT "/thread%i", i + 1);
> + THREADS_DIR "/thread%i", i + 1);
> SAFE_MKDIR(workers[i].dir, 0700);
> }

Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>

FWIW I think you could have merged this after a few days or even
straight away.

--
Thank you,
Richard.