Re: [LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Cyril Hrubis <chrubis@suse.cz>]

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?

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp