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

[Tarun Sahu <tsahu@linux.ibm.com>]

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

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