| Message ID | 20221029071344.45447-2-tsahu@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Hugetlb:Migrating the libhugetlbfs tests | expand |
Hi Tarun, This version is much better, comments are inline below. On Sat, Oct 29, 2022 at 3:14 PM Tarun Sahu <tsahu@linux.ibm.com> wrote: > Most of libhugetlbfs test require mounted hugetlbfs and random opened > unlinked file for follow-up test actions. > > Here, this patch adds two new field in tst_test struct(include/tst_test.h) > which user can set if the test requires mounted hugetlbfs and other one > for if test requires opened unlinked file. > > Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com> > --- > include/tst_test.h | 20 +++++++++++++++++- > lib/tst_test.c | 51 +++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 67 insertions(+), 4 deletions(-) > > diff --git a/include/tst_test.h b/include/tst_test.h > index a69965b95..f36382ae9 100644 > --- a/include/tst_test.h > +++ b/include/tst_test.h > @@ -131,7 +131,7 @@ struct tst_tag { > }; > > extern unsigned int tst_variant; > - > +extern int tst_hugetlb_fd; > #define TST_NO_HUGEPAGES ((unsigned long)-1) > > #define TST_UNLIMITED_RUNTIME (-1) > @@ -176,6 +176,18 @@ struct tst_test { > int all_filesystems:1; > int skip_in_lockdown:1; > int skip_in_compat:1; > + /* > + * If set, the test function will create a hugetlbfs mount point > + * at /tmp/xxxxxx, where xxxxxx is a random string. > + */ > + int needs_hugetlbfs:1; > + /* > + * If set, the test function will create and open a random file > + * under mounted hugetlbfs. To use this option, needs_hugetlbfs > must > + * be set. The file descriptior will be set in tst_hugetlb_fd. > + * The close(tst_hugetlb_fd) will be called on test exit(cleanup). > + */ > + int needs_unlinked_hugetlb_file:1; > Why not consider encapsulating these two new fields in 'struct tst_hugepage' ? Then the tst_test in the case can simply initialize to: .... static struct tst_test test = { .needs_root = 1, .taint_check = TST_TAINT_D | TST_TAINT_W, .setup = setup, .test_all = run_test, .hugepages = {1, TST_NEEDS, 1, 1}, }; > > /* > * The skip_filesystems is a NULL terminated list of filesystems > the > @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void); > */ > void tst_set_max_runtime(int max_runtime); > > +/* > + * Create and open a random file inside the given dir path. > + * It unlinks the file after opening and return file descriptor. > + */ > +int tst_create_unlinked_file(const char *path); > what about renaming this function to tst_'get|create'_unlinked_file_fd? I guess the 'fd' part should be emphasized here. > + > /* > * Returns path to the test temporary directory in a newly allocated > buffer. > */ > diff --git a/lib/tst_test.c b/lib/tst_test.c > index 8ccde1629..43cba1004 100644 > --- a/lib/tst_test.c > +++ b/lib/tst_test.c > @@ -925,7 +925,8 @@ static int needs_tmpdir(void) > tst_test->needs_device || > tst_test->mntpoint || > tst_test->resource_files || > - tst_test->needs_checkpoints; > + tst_test->needs_checkpoints || > + tst_test->needs_hugetlbfs; > } > > static void copy_resources(void) > @@ -1021,6 +1022,30 @@ static void prepare_and_mount_dev_fs(const char > *mntpoint) > } > } > > +static void prepare_and_mount_hugetlb_fs(void) > +{ > + tst_test->mntpoint = tst_get_tmpdir(); > + SAFE_MOUNT("none", tst_test->mntpoint, "hugetlbfs", 0, NULL); > + mntpoint_mounted = 1; > +} > + > +int tst_create_unlinked_file(const char *path) > +{ > + char template[PATH_MAX]; > + int fd; > + > + snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX", > + path, TCID); > + > + fd = mkstemp(template); > + if (fd < 0) > + tst_brk(TBROK | TERRNO, > + "%s: mkstemp(%s) failed", __func__, template); > + > + SAFE_UNLINK(template); > + return fd; > +} > + > static const char *limit_tmpfs_mount_size(const char *mnt_data, > char *buf, size_t buf_size, const char *fs_type) > { > @@ -1094,6 +1119,8 @@ static void do_cgroup_requires(void) > tst_cg_init(); > } > > +int tst_hugetlb_fd = -1; > + > static void do_setup(int argc, char *argv[]) > { > if (!tst_test) > @@ -1217,6 +1244,17 @@ static void do_setup(int argc, char *argv[]) > } > } > > + if (tst_test->needs_hugetlbfs) > + prepare_and_mount_hugetlb_fs(); > + > + if (tst_test->needs_unlinked_hugetlb_file) { > + if (!(tst_test->needs_hugetlbfs)) { > + tst_brk(TBROK, "Option needs_unlinked_hugetlb_file > " > + "requires option needs_hugetlbfs"); > + } > + tst_hugetlb_fd = > tst_create_unlinked_file(tst_test->mntpoint); > + } > + > Seems we have to add a confliction check[1] to avoid multiple mounting at 'tst_test->mntpoint'. Or maybe go another method to move all the hugetlbfs operations into tst_hugetlb.c to isolate details from the tst_test library, but this will require more changes for all preexisting hugetlb tests. [1] something like this: @@ -1224,9 +1224,9 @@ static void do_setup(int argc, char *argv[]) } if (!!tst_test->needs_rofs + !!tst_test->needs_devfs + - !!tst_test->needs_device > 1) { + !!tst_test->needs_device + !!tst_test->needs_hugetlbfs > 1) { tst_brk(TBROK, - "Two or more of needs_{rofs, devfs, device} are set"); + "Two or more of needs_{rofs, devfs, device, hugetlb} are set"); } if (tst_test->needs_device && !mntpoint_mounted) { > tdev.dev = tst_acquire_device_(NULL, > tst_test->dev_min_size); > @@ -1299,8 +1337,15 @@ static void do_cleanup(void) > if (ovl_mounted) > SAFE_UMOUNT(OVL_MNT); > > - if (mntpoint_mounted) > - tst_umount(tst_test->mntpoint); > + if (tst_hugetlb_fd >= 0) > + SAFE_CLOSE(tst_hugetlb_fd); > + > + if (mntpoint_mounted) { > + if (tst_test->needs_hugetlbfs) > + SAFE_UMOUNT(tst_test->mntpoint); > + else > + tst_umount(tst_test->mntpoint); > + } > > if (tst_test->needs_device && tdev.dev) > tst_release_device(tdev.dev); > -- > 2.31.1 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > >
Hello, Thanks for reviewing patch Li, On Oct 31 2022, Li Wang wrote: > Hi Tarun, > > This version is much better, comments are inline below. > > On Sat, Oct 29, 2022 at 3:14 PM Tarun Sahu <tsahu@linux.ibm.com> wrote: > > > Most of libhugetlbfs test require mounted hugetlbfs and random opened > > unlinked file for follow-up test actions. > > > > Here, this patch adds two new field in tst_test struct(include/tst_test.h) > > which user can set if the test requires mounted hugetlbfs and other one > > for if test requires opened unlinked file. > > > > Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com> > > --- > > include/tst_test.h | 20 +++++++++++++++++- > > lib/tst_test.c | 51 +++++++++++++++++++++++++++++++++++++++++++--- > > 2 files changed, 67 insertions(+), 4 deletions(-) > > > > diff --git a/include/tst_test.h b/include/tst_test.h > > index a69965b95..f36382ae9 100644 > > --- a/include/tst_test.h > > +++ b/include/tst_test.h > > @@ -131,7 +131,7 @@ struct tst_tag { > > }; > > > > extern unsigned int tst_variant; > > - > > +extern int tst_hugetlb_fd; > > #define TST_NO_HUGEPAGES ((unsigned long)-1) > > > > #define TST_UNLIMITED_RUNTIME (-1) > > @@ -176,6 +176,18 @@ struct tst_test { > > int all_filesystems:1; > > int skip_in_lockdown:1; > > int skip_in_compat:1; > > + /* > > + * If set, the test function will create a hugetlbfs mount point > > + * at /tmp/xxxxxx, where xxxxxx is a random string. > > + */ > > + int needs_hugetlbfs:1; > > + /* > > + * If set, the test function will create and open a random file > > + * under mounted hugetlbfs. To use this option, needs_hugetlbfs > > must > > + * be set. The file descriptior will be set in tst_hugetlb_fd. > > + * The close(tst_hugetlb_fd) will be called on test exit(cleanup). > > + */ > > + int needs_unlinked_hugetlb_file:1; > > > > Why not consider encapsulating these two new fields in 'struct > tst_hugepage' ? > > Then the tst_test in the case can simply initialize to: > > .... > static struct tst_test test = { > .needs_root = 1, > .taint_check = TST_TAINT_D | TST_TAINT_W, > .setup = setup, > .test_all = run_test, > .hugepages = {1, TST_NEEDS, 1, 1}, > }; > Ok, I will move these fields in tst_hugepages struct. > > > > > > /* > > * The skip_filesystems is a NULL terminated list of filesystems > > the > > @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void); > > */ > > void tst_set_max_runtime(int max_runtime); > > > > +/* > > + * Create and open a random file inside the given dir path. > > + * It unlinks the file after opening and return file descriptor. > > + */ > > +int tst_create_unlinked_file(const char *path); > > > > what about renaming this function to tst_'get|create'_unlinked_file_fd? > I guess the 'fd' part should be emphasized here. > OK. Will update it. > > > > + > > /* > > * Returns path to the test temporary directory in a newly allocated > > buffer. > > */ > > diff --git a/lib/tst_test.c b/lib/tst_test.c > > index 8ccde1629..43cba1004 100644 > > --- a/lib/tst_test.c > > +++ b/lib/tst_test.c > > @@ -925,7 +925,8 @@ static int needs_tmpdir(void) > > tst_test->needs_device || > > tst_test->mntpoint || > > tst_test->resource_files || > > - tst_test->needs_checkpoints; > > + tst_test->needs_checkpoints || > > + tst_test->needs_hugetlbfs; > > } > > > > static void copy_resources(void) > > @@ -1021,6 +1022,30 @@ static void prepare_and_mount_dev_fs(const char > > *mntpoint) > > } > > } > > > > +static void prepare_and_mount_hugetlb_fs(void) > > +{ > > + tst_test->mntpoint = tst_get_tmpdir(); > > + SAFE_MOUNT("none", tst_test->mntpoint, "hugetlbfs", 0, NULL); > > + mntpoint_mounted = 1; > > +} > > + > > +int tst_create_unlinked_file(const char *path) > > +{ > > + char template[PATH_MAX]; > > + int fd; > > + > > + snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX", > > + path, TCID); > > + > > + fd = mkstemp(template); > > + if (fd < 0) > > + tst_brk(TBROK | TERRNO, > > + "%s: mkstemp(%s) failed", __func__, template); > > + > > + SAFE_UNLINK(template); > > + return fd; > > +} > > + > > static const char *limit_tmpfs_mount_size(const char *mnt_data, > > char *buf, size_t buf_size, const char *fs_type) > > { > > @@ -1094,6 +1119,8 @@ static void do_cgroup_requires(void) > > tst_cg_init(); > > } > > > > +int tst_hugetlb_fd = -1; > > + > > static void do_setup(int argc, char *argv[]) > > { > > if (!tst_test) > > @@ -1217,6 +1244,17 @@ static void do_setup(int argc, char *argv[]) > > } > > } > > > > + if (tst_test->needs_hugetlbfs) > > + prepare_and_mount_hugetlb_fs(); > > + > > + if (tst_test->needs_unlinked_hugetlb_file) { > > + if (!(tst_test->needs_hugetlbfs)) { > > + tst_brk(TBROK, "Option needs_unlinked_hugetlb_file > > " > > + "requires option needs_hugetlbfs"); > > + } > > + tst_hugetlb_fd = > > tst_create_unlinked_file(tst_test->mntpoint); > > + } > > + > > > > Seems we have to add a confliction check[1] to avoid multiple mounting > at 'tst_test->mntpoint'. Or maybe go another method to move all the > hugetlbfs > operations into tst_hugetlb.c to isolate details from the tst_test library, > but > this will require more changes for all preexisting hugetlb tests. > > > [1] something like this: > > @@ -1224,9 +1224,9 @@ static void do_setup(int argc, char *argv[]) > } > > if (!!tst_test->needs_rofs + !!tst_test->needs_devfs + > - !!tst_test->needs_device > 1) { > + !!tst_test->needs_device + !!tst_test->needs_hugetlbfs > 1) { > tst_brk(TBROK, > - "Two or more of needs_{rofs, devfs, device} are > set"); > + "Two or more of needs_{rofs, devfs, device, > hugetlb} are set"); > } > OK. Will update it. > > if (tst_test->needs_device && !mntpoint_mounted) { > > tdev.dev = tst_acquire_device_(NULL, > > tst_test->dev_min_size); > > > > @@ -1299,8 +1337,15 @@ static void do_cleanup(void) > > if (ovl_mounted) > > SAFE_UMOUNT(OVL_MNT); > > > > - if (mntpoint_mounted) > > - tst_umount(tst_test->mntpoint); > > + if (tst_hugetlb_fd >= 0) > > + SAFE_CLOSE(tst_hugetlb_fd); > > + > > + if (mntpoint_mounted) { > > + if (tst_test->needs_hugetlbfs) > > + SAFE_UMOUNT(tst_test->mntpoint); > > + else > > + tst_umount(tst_test->mntpoint); > > + } > > > > if (tst_test->needs_device && tdev.dev) > > tst_release_device(tdev.dev); > > -- > > 2.31.1 > > > > > > -- > > Mailing list info: https://lists.linux.it/listinfo/ltp > > > > > > -- > Regards, > Li Wang > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi! > extern unsigned int tst_variant; > - > +extern int tst_hugetlb_fd; > #define TST_NO_HUGEPAGES ((unsigned long)-1) > > #define TST_UNLIMITED_RUNTIME (-1) > @@ -176,6 +176,18 @@ struct tst_test { > int all_filesystems:1; > int skip_in_lockdown:1; > int skip_in_compat:1; > + /* > + * If set, the test function will create a hugetlbfs mount point > + * at /tmp/xxxxxx, where xxxxxx is a random string. > + */ > + int needs_hugetlbfs:1; > + /* > + * If set, the test function will create and open a random file > + * under mounted hugetlbfs. To use this option, needs_hugetlbfs must > + * be set. The file descriptior will be set in tst_hugetlb_fd. > + * The close(tst_hugetlb_fd) will be called on test exit(cleanup). > + */ > + int needs_unlinked_hugetlb_file:1; > > /* > * The skip_filesystems is a NULL terminated list of filesystems the > @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void); > */ > void tst_set_max_runtime(int max_runtime); > > +/* > + * Create and open a random file inside the given dir path. > + * It unlinks the file after opening and return file descriptor. > + */ > +int tst_create_unlinked_file(const char *path); > + > /* > * Returns path to the test temporary directory in a newly allocated buffer. > */ > diff --git a/lib/tst_test.c b/lib/tst_test.c > index 8ccde1629..43cba1004 100644 > --- a/lib/tst_test.c > +++ b/lib/tst_test.c > @@ -925,7 +925,8 @@ static int needs_tmpdir(void) > tst_test->needs_device || > tst_test->mntpoint || > tst_test->resource_files || > - tst_test->needs_checkpoints; > + tst_test->needs_checkpoints || > + tst_test->needs_hugetlbfs; > } > > static void copy_resources(void) > @@ -1021,6 +1022,30 @@ static void prepare_and_mount_dev_fs(const char *mntpoint) > } > } > > +static void prepare_and_mount_hugetlb_fs(void) > +{ > + tst_test->mntpoint = tst_get_tmpdir(); > + SAFE_MOUNT("none", tst_test->mntpoint, "hugetlbfs", 0, NULL); > + mntpoint_mounted = 1; > +} > + > +int tst_create_unlinked_file(const char *path) > +{ > + char template[PATH_MAX]; > + int fd; > + > + snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX", > + path, TCID); > + > + fd = mkstemp(template); > + if (fd < 0) > + tst_brk(TBROK | TERRNO, > + "%s: mkstemp(%s) failed", __func__, template); > + > + SAFE_UNLINK(template); > + return fd; > +} > + > static const char *limit_tmpfs_mount_size(const char *mnt_data, > char *buf, size_t buf_size, const char *fs_type) > { > @@ -1094,6 +1119,8 @@ static void do_cgroup_requires(void) > tst_cg_init(); > } > > +int tst_hugetlb_fd = -1; > + > static void do_setup(int argc, char *argv[]) > { > if (!tst_test) > @@ -1217,6 +1244,17 @@ static void do_setup(int argc, char *argv[]) > } > } > > + if (tst_test->needs_hugetlbfs) > + prepare_and_mount_hugetlb_fs(); > + > + if (tst_test->needs_unlinked_hugetlb_file) { > + if (!(tst_test->needs_hugetlbfs)) { > + tst_brk(TBROK, "Option needs_unlinked_hugetlb_file " > + "requires option needs_hugetlbfs"); > + } > + tst_hugetlb_fd = tst_create_unlinked_file(tst_test->mntpoint); > + } The function tst_create_unlinked_file() looks useful, but I do not think that adding the needs_unlinked_hugetlb_file flag simplifies things that much. Also this will not scale well when we would need two filedescripors like that. Maybe we it would be cleaner to add only the mount/umount functionality to the test library and call the tst_create_unlinked_file() in the test setup in the testcases. > if (tst_test->needs_device && !mntpoint_mounted) { > tdev.dev = tst_acquire_device_(NULL, tst_test->dev_min_size); > > @@ -1299,8 +1337,15 @@ static void do_cleanup(void) > if (ovl_mounted) > SAFE_UMOUNT(OVL_MNT); > > - if (mntpoint_mounted) > - tst_umount(tst_test->mntpoint); > + if (tst_hugetlb_fd >= 0) > + SAFE_CLOSE(tst_hugetlb_fd); > + > + if (mntpoint_mounted) { > + if (tst_test->needs_hugetlbfs) > + SAFE_UMOUNT(tst_test->mntpoint); > + else > + tst_umount(tst_test->mntpoint); > + } Is there a good reason for this, why can't we call tst_umount() for hugetlbfs?
Hi! > > @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void); > > */ > > void tst_set_max_runtime(int max_runtime); > > > > +/* > > + * Create and open a random file inside the given dir path. > > + * It unlinks the file after opening and return file descriptor. > > + */ > > +int tst_create_unlinked_file(const char *path); > > > > what about renaming this function to tst_'get|create'_unlinked_file_fd? > I guess the 'fd' part should be emphasized here. It has create in name and in UNIX creat() creates file and returns a file descriptor so I think it's fine. Maybe we can be even shorted with something as tst_creat_unlinked(const char *path).
Hi! > Why not consider encapsulating these two new fields in 'struct > tst_hugepage' ? > > Then the tst_test in the case can simply initialize to: > > .... > static struct tst_test test = { > .needs_root = 1, > .taint_check = TST_TAINT_D | TST_TAINT_W, > .setup = setup, > .test_all = run_test, > .hugepages = {1, TST_NEEDS, 1, 1}, > }; I do not like that we have magic constants in the .hugepages that are not self describing. I would treat the hugetltbfs just as we treat devfs, that would be: #define MNTPOINT "hugetlbfs/" #define HUGEFILE MNTPOINT "hugefile" static int huge_fd; static void setup(void) { huge_fd = tst_creat_unlinked(HUGEFILE); ... } static void cleanup(void) { if (huge_fd > 0) SAFE_CLOSE(huge_fd); } static struct tst_test test = { ... .mntpoint = MNTPOINT, .needs_hugetlbfs = 1, .setup = setup, .cleanup = cleanup, ... } What do you think?
Hi Cyril, Please find my comments inline. On Oct 31 2022, Cyril Hrubis wrote: > Hi! > > Why not consider encapsulating these two new fields in 'struct > > tst_hugepage' ? > > > > Then the tst_test in the case can simply initialize to: > > > > .... > > static struct tst_test test = { > > .needs_root = 1, > > .taint_check = TST_TAINT_D | TST_TAINT_W, > > .setup = setup, > > .test_all = run_test, > > .hugepages = {1, TST_NEEDS, 1, 1}, > > }; > > I do not like that we have magic constants in the .hugepages that are > not self describing. I would treat the hugetltbfs just as we treat > devfs, that would be: > > #define MNTPOINT "hugetlbfs/" > #define HUGEFILE MNTPOINT "hugefile" > > static int huge_fd; > > static void setup(void) > { > huge_fd = tst_creat_unlinked(HUGEFILE); > ... > } > > static void cleanup(void) > { > if (huge_fd > 0) > SAFE_CLOSE(huge_fd); > } > > static struct tst_test test = { > ... > .mntpoint = MNTPOINT, > .needs_hugetlbfs = 1, > .setup = setup, > .cleanup = cleanup, > ... > } > > > What do you think? > My original idea behind putting it in tst_test struct instead of tst_hugepages was based on below two reasoning- 1. tst_hugepages seems to have only hugepages related info. It would be better to rename it to tst_hugetlb and rename <hugepages> field in tst_test struct to <hugetlb>. If we rename it which will require changes in already existing tests. and Moreover, there were similar field like needs_tmpdir in tst_test so I put it(needs_unlinked_hugetlb_file) in tst_test. 2. There was already related fields in tst_test for mounting fs, like needs_devfs, needs_rofs, So keeping all the needs_ABCfs at same structure. > -- > Cyril Hrubis > chrubis@suse.cz > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
On Oct 31 2022, Cyril Hrubis wrote: > Hi! > > extern unsigned int tst_variant; > > - > > +extern int tst_hugetlb_fd; > > #define TST_NO_HUGEPAGES ((unsigned long)-1) > > > > #define TST_UNLIMITED_RUNTIME (-1) > > @@ -176,6 +176,18 @@ struct tst_test { > > int all_filesystems:1; > > int skip_in_lockdown:1; > > int skip_in_compat:1; > > + /* > > + * If set, the test function will create a hugetlbfs mount point > > + * at /tmp/xxxxxx, where xxxxxx is a random string. > > + */ > > + int needs_hugetlbfs:1; > > + /* > > + * If set, the test function will create and open a random file > > + * under mounted hugetlbfs. To use this option, needs_hugetlbfs must > > + * be set. The file descriptior will be set in tst_hugetlb_fd. > > + * The close(tst_hugetlb_fd) will be called on test exit(cleanup). > > + */ > > + int needs_unlinked_hugetlb_file:1; > > > > /* > > * The skip_filesystems is a NULL terminated list of filesystems the > > @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void); > > */ > > void tst_set_max_runtime(int max_runtime); > > > > +/* > > + * Create and open a random file inside the given dir path. > > + * It unlinks the file after opening and return file descriptor. > > + */ > > +int tst_create_unlinked_file(const char *path); > > + > > /* > > * Returns path to the test temporary directory in a newly allocated buffer. > > */ > > diff --git a/lib/tst_test.c b/lib/tst_test.c > > index 8ccde1629..43cba1004 100644 > > --- a/lib/tst_test.c > > +++ b/lib/tst_test.c > > @@ -925,7 +925,8 @@ static int needs_tmpdir(void) > > tst_test->needs_device || > > tst_test->mntpoint || > > tst_test->resource_files || > > - tst_test->needs_checkpoints; > > + tst_test->needs_checkpoints || > > + tst_test->needs_hugetlbfs; > > } > > > > static void copy_resources(void) > > @@ -1021,6 +1022,30 @@ static void prepare_and_mount_dev_fs(const char *mntpoint) > > } > > } > > > > +static void prepare_and_mount_hugetlb_fs(void) > > +{ > > + tst_test->mntpoint = tst_get_tmpdir(); > > + SAFE_MOUNT("none", tst_test->mntpoint, "hugetlbfs", 0, NULL); > > + mntpoint_mounted = 1; > > +} > > + > > +int tst_create_unlinked_file(const char *path) > > +{ > > + char template[PATH_MAX]; > > + int fd; > > + > > + snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX", > > + path, TCID); > > + > > + fd = mkstemp(template); > > + if (fd < 0) > > + tst_brk(TBROK | TERRNO, > > + "%s: mkstemp(%s) failed", __func__, template); > > + > > + SAFE_UNLINK(template); > > + return fd; > > +} > > + > > static const char *limit_tmpfs_mount_size(const char *mnt_data, > > char *buf, size_t buf_size, const char *fs_type) > > { > > @@ -1094,6 +1119,8 @@ static void do_cgroup_requires(void) > > tst_cg_init(); > > } > > > > +int tst_hugetlb_fd = -1; > > + > > static void do_setup(int argc, char *argv[]) > > { > > if (!tst_test) > > @@ -1217,6 +1244,17 @@ static void do_setup(int argc, char *argv[]) > > } > > } > > > > + if (tst_test->needs_hugetlbfs) > > + prepare_and_mount_hugetlb_fs(); > > + > > + if (tst_test->needs_unlinked_hugetlb_file) { > > + if (!(tst_test->needs_hugetlbfs)) { > > + tst_brk(TBROK, "Option needs_unlinked_hugetlb_file " > > + "requires option needs_hugetlbfs"); > > + } > > + tst_hugetlb_fd = tst_create_unlinked_file(tst_test->mntpoint); > > + } > > The function tst_create_unlinked_file() looks useful, but I do not think > that adding the needs_unlinked_hugetlb_file flag simplifies things that > much. Also this will not scale well when we would need two > filedescripors like that. Maybe we it would be cleaner to add only the > mount/umount functionality to the test library and call the > tst_create_unlinked_file() in the test setup in the testcases. > yes I agree, There is a test that requires two such unlinked file, there is one that requires one hugetlb and one normal such unlinked file. > > if (tst_test->needs_device && !mntpoint_mounted) { > > tdev.dev = tst_acquire_device_(NULL, tst_test->dev_min_size); > > > > @@ -1299,8 +1337,15 @@ static void do_cleanup(void) > > if (ovl_mounted) > > SAFE_UMOUNT(OVL_MNT); > > > > - if (mntpoint_mounted) > > - tst_umount(tst_test->mntpoint); > > + if (tst_hugetlb_fd >= 0) > > + SAFE_CLOSE(tst_hugetlb_fd); > > + > > + if (mntpoint_mounted) { > > + if (tst_test->needs_hugetlbfs) > > + SAFE_UMOUNT(tst_test->mntpoint); > > + else > > + tst_umount(tst_test->mntpoint); > > + } > > Is there a good reason for this, why can't we call tst_umount() for > hugetlbfs? > My reason behind this is, tst_umount doesnt break the test, if umount fails. One of the cause of failure can be that test left the mounted fs busy, which is not expected. > -- > Cyril Hrubis > chrubis@suse.cz > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
On Mon, Oct 31, 2022 at 10:54 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > Why not consider encapsulating these two new fields in 'struct > > tst_hugepage' ? > > > > Then the tst_test in the case can simply initialize to: > > > > .... > > static struct tst_test test = { > > .needs_root = 1, > > .taint_check = TST_TAINT_D | TST_TAINT_W, > > .setup = setup, > > .test_all = run_test, > > .hugepages = {1, TST_NEEDS, 1, 1}, > > }; > > I do not like that we have magic constants in the .hugepages that are > not self describing. I would treat the hugetltbfs just as we treat > devfs, that would be: > > #define MNTPOINT "hugetlbfs/" > #define HUGEFILE MNTPOINT "hugefile" > > static int huge_fd; > > static void setup(void) > { > huge_fd = tst_creat_unlinked(HUGEFILE); > ... > } > > static void cleanup(void) > { > if (huge_fd > 0) > SAFE_CLOSE(huge_fd); > } > > static struct tst_test test = { > ... > .mntpoint = MNTPOINT, > .needs_hugetlbfs = 1, > .setup = setup, > .cleanup = cleanup, > ... > } > > > What do you think? > +1 Looks good, this treats it as an FS and is separated from the hugepage usage.
Hi! > > Is there a good reason for this, why can't we call tst_umount() for > > hugetlbfs? > > > > My reason behind this is, > tst_umount doesnt break the test, if umount fails. One of the cause > of failure can be that test left the mounted fs busy, which is not > expected. This does not matter, because the do_cleanup() is called only once at the end of the test. The test exists once the function does finish anyways.
diff --git a/include/tst_test.h b/include/tst_test.h index a69965b95..f36382ae9 100644 --- a/include/tst_test.h +++ b/include/tst_test.h @@ -131,7 +131,7 @@ struct tst_tag { }; extern unsigned int tst_variant; - +extern int tst_hugetlb_fd; #define TST_NO_HUGEPAGES ((unsigned long)-1) #define TST_UNLIMITED_RUNTIME (-1) @@ -176,6 +176,18 @@ struct tst_test { int all_filesystems:1; int skip_in_lockdown:1; int skip_in_compat:1; + /* + * If set, the test function will create a hugetlbfs mount point + * at /tmp/xxxxxx, where xxxxxx is a random string. + */ + int needs_hugetlbfs:1; + /* + * If set, the test function will create and open a random file + * under mounted hugetlbfs. To use this option, needs_hugetlbfs must + * be set. The file descriptior will be set in tst_hugetlb_fd. + * The close(tst_hugetlb_fd) will be called on test exit(cleanup). + */ + int needs_unlinked_hugetlb_file:1; /* * The skip_filesystems is a NULL terminated list of filesystems the @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void); */ void tst_set_max_runtime(int max_runtime); +/* + * Create and open a random file inside the given dir path. + * It unlinks the file after opening and return file descriptor. + */ +int tst_create_unlinked_file(const char *path); + /* * Returns path to the test temporary directory in a newly allocated buffer. */ diff --git a/lib/tst_test.c b/lib/tst_test.c index 8ccde1629..43cba1004 100644 --- a/lib/tst_test.c +++ b/lib/tst_test.c @@ -925,7 +925,8 @@ static int needs_tmpdir(void) tst_test->needs_device || tst_test->mntpoint || tst_test->resource_files || - tst_test->needs_checkpoints; + tst_test->needs_checkpoints || + tst_test->needs_hugetlbfs; } static void copy_resources(void) @@ -1021,6 +1022,30 @@ static void prepare_and_mount_dev_fs(const char *mntpoint) } } +static void prepare_and_mount_hugetlb_fs(void) +{ + tst_test->mntpoint = tst_get_tmpdir(); + SAFE_MOUNT("none", tst_test->mntpoint, "hugetlbfs", 0, NULL); + mntpoint_mounted = 1; +} + +int tst_create_unlinked_file(const char *path) +{ + char template[PATH_MAX]; + int fd; + + snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX", + path, TCID); + + fd = mkstemp(template); + if (fd < 0) + tst_brk(TBROK | TERRNO, + "%s: mkstemp(%s) failed", __func__, template); + + SAFE_UNLINK(template); + return fd; +} + static const char *limit_tmpfs_mount_size(const char *mnt_data, char *buf, size_t buf_size, const char *fs_type) { @@ -1094,6 +1119,8 @@ static void do_cgroup_requires(void) tst_cg_init(); } +int tst_hugetlb_fd = -1; + static void do_setup(int argc, char *argv[]) { if (!tst_test) @@ -1217,6 +1244,17 @@ static void do_setup(int argc, char *argv[]) } } + if (tst_test->needs_hugetlbfs) + prepare_and_mount_hugetlb_fs(); + + if (tst_test->needs_unlinked_hugetlb_file) { + if (!(tst_test->needs_hugetlbfs)) { + tst_brk(TBROK, "Option needs_unlinked_hugetlb_file " + "requires option needs_hugetlbfs"); + } + tst_hugetlb_fd = tst_create_unlinked_file(tst_test->mntpoint); + } + if (tst_test->needs_device && !mntpoint_mounted) { tdev.dev = tst_acquire_device_(NULL, tst_test->dev_min_size); @@ -1299,8 +1337,15 @@ static void do_cleanup(void) if (ovl_mounted) SAFE_UMOUNT(OVL_MNT); - if (mntpoint_mounted) - tst_umount(tst_test->mntpoint); + if (tst_hugetlb_fd >= 0) + SAFE_CLOSE(tst_hugetlb_fd); + + if (mntpoint_mounted) { + if (tst_test->needs_hugetlbfs) + SAFE_UMOUNT(tst_test->mntpoint); + else + tst_umount(tst_test->mntpoint); + } if (tst_test->needs_device && tdev.dev) tst_release_device(tdev.dev);
Most of libhugetlbfs test require mounted hugetlbfs and random opened unlinked file for follow-up test actions. Here, this patch adds two new field in tst_test struct(include/tst_test.h) which user can set if the test requires mounted hugetlbfs and other one for if test requires opened unlinked file. Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com> --- include/tst_test.h | 20 +++++++++++++++++- lib/tst_test.c | 51 +++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 67 insertions(+), 4 deletions(-)