Updating how we file datapatterns work (#35)

Previously when the option -datapattern file/wholefile was used, XDD
would open the file for every target and keep it open till the run was
over. This is really unnecessary as the file only needs to be opened to
read the datapattern in to set the datapattern in the target. Once that
has happen, the file can immediately be closed. This is a good idea as
there are limits to how many files can be opened in Linux (ulimit). XDD
should not tie up the number of avialable open file descriptors just to
set a datapattern over an entire XDD run.

The code has been updated to open the file, set the target data_pattern
buffer, and then immediately close the file. I have added a new
functional test case test_xdd_file_datapattern.sh to test out that these
this modification did not break the expected functionality of
-datapattern file/wholefile.

There was also just some general cleanup of the code that was done as
part of this PR.

Signed-off-by: Brian Atkinson <batkinson@lanl.gov>

src/base/target_cleanup.c

@@ -69,7 +69,6 @@ xdd_target_thread_cleanup(target_data_t *tdp) {
             /* If the file target opend a file for the data pattern we must close
             * the files and clean up the data_pattern buffer
             */
-            close(tdp->dpp_fd);
             free(tdp->td_dpp->data_pattern);
         }
         free(tdp->td_dpp);

src/client/parse_func.c

@@ -555,6 +555,8 @@ xddfunc_datapattern(xdd_plan_t *planp, int32_t argc, char *argv[], uint32_t flag
 		}
 	} else if (strcmp(pattern_type, "file") == 0 ||  strcmp(pattern_type, "wholefile") == 0) {
 		retval++;
+		int dp_fd = -1;
+
 		if (argc <= args+2) {
 			fprintf(xgp->errout, "%s: not enough arguments specified for the option '-datapattern'\n", xgp->progname);
 			return(0);
@@ -566,61 +568,48 @@ xddfunc_datapattern(xdd_plan_t *planp, int32_t argc, char *argv[], uint32_t flag
 					(char *)argv[args+2]);
 				return(0);
 			}
-			pattern_length = stat_buf.st_size;
+
 		}
 		if (tdp) { /* set option for specific target */
 			if (strcmp(pattern_type, "file") == 0) {
 				tdp->td_dpp->data_pattern_options |= DP_FILE_PATTERN;
 			} else {// wholefile
 				tdp->td_dpp->data_pattern_options |= DP_WHOLEFILE_PATTERN;
+				if (!xdd_datapattern_wholefile_enough_ram(tdp, argv[args+2])) {
+					return(0);
+				}
 			}
-			tdp->td_dpp->data_pattern_filename = (char *)argv[args+2];
-			tdp->td_dpp->data_pattern_length = pattern_length;
-			if (!xdd_datapattern_wholefile_enough_ram(tdp)) {
-				fprintf(xgp->errout, "%s: file %s can not be loaded into memory because 50%% of RAM is "
-					"currently unavailable\n",
-					xgp->progname,
-					(char *)argv[args+2]);
-				return(0);
-			}
-			tdp->dpp_fd = open(tdp->td_dpp->data_pattern_filename, O_RDONLY);
-			if (tdp->dpp_fd < 0) {
-				fprintf(xgp->errout, "%s: could not open %s\n", xgp->progname, tdp->td_dpp->data_pattern_filename);
+			tdp->td_dpp->data_pattern_length = stat_buf.st_size;
+			if(xdd_set_datapattern_from_filename(tdp, argv[args+2])) {
 				return(0);
 			}
-			tdp->td_dpp->data_pattern = (unsigned char *)malloc(sizeof(unsigned char) * (tdp->td_dpp->data_pattern_length + 1));
-			memset(tdp->td_dpp->data_pattern, '\0', sizeof(unsigned char) * (tdp->td_dpp->data_pattern_length + 1));
-			pread(tdp->dpp_fd, tdp->td_dpp->data_pattern, tdp->td_dpp->data_pattern_length, 0);
 		} else {// Put this option into all Targets
 			if (flags & XDD_PARSE_PHASE2) {
 				tdp = planp->target_datap[0];
+				tdp->td_dpp->data_pattern_length = stat_buf.st_size;
 				i = 0;
-				while (tdp) {
-					if (strcmp(pattern_type, "file") == 0) {
-						tdp->td_dpp->data_pattern_options |= DP_FILE_PATTERN;
-					} else {//wholefile
-						tdp->td_dpp->data_pattern_options |= DP_WHOLEFILE_PATTERN;
-					}
-					tdp->td_dpp->data_pattern_filename = (char *)argv[args+2];
-					tdp->td_dpp->data_pattern_length = pattern_length;
-					if (!xdd_datapattern_wholefile_enough_ram(tdp)) {
-						fprintf(xgp->errout, "%s: the file %s is larger than 50%% of total RAM\n",
-							xgp->progname,
-							(char *)argv[args+2]);
+				dp_fd = open(argv[args+2], O_RDONLY);
+				if (dp_fd < 0) {
+					fprintf(xgp->errout, "%s: could not open %s\n", xgp->progname, argv[args+2]);
+					return(0);
+				}
+				if (strcmp(pattern_type, "file") == 0) {
+					tdp->td_dpp->data_pattern_options |= DP_FILE_PATTERN;
+				} else {//wholefile
+					tdp->td_dpp->data_pattern_options |= DP_WHOLEFILE_PATTERN;
+					if (!xdd_datapattern_wholefile_enough_ram(tdp, argv[args+2])) {
 						return(0);
 					}
-					tdp->dpp_fd = open(tdp->td_dpp->data_pattern_filename, O_RDONLY);
-					if (tdp->dpp_fd < 0) {
-						fprintf(xgp->errout, "%s: could not open %s\n", xgp->progname, tdp->td_dpp->data_pattern_filename);
+				}
+				while (tdp) {
+					if (xdd_set_datapattern_from_file_descriptor(tdp, dp_fd, argv[args+2])) {
+						close(dp_fd);
 						return(0);
 					}
-					tdp->td_dpp->data_pattern_length = pattern_length;
-					tdp->td_dpp->data_pattern = (unsigned char *)malloc(sizeof(unsigned char) * (tdp->td_dpp->data_pattern_length + 1));
-					memset(tdp->td_dpp->data_pattern, '\0', sizeof(unsigned char) * (tdp->td_dpp->data_pattern_length + 1));
-					pread(tdp->dpp_fd, tdp->td_dpp->data_pattern, tdp->td_dpp->data_pattern_length, 0);
 					i++;
 					tdp = planp->target_datap[i];
 				}
+				close(dp_fd);
 			}
 		}
 	} else if (strcmp(pattern_type, "sequenced") == 0) {

src/common/datapatterns.c

@@ -257,22 +257,92 @@ xdd_datapattern_fill(worker_data_t *wdp) {
  * WHOLEFILE_MAX_SIZE_RAM.
  */
 int
-xdd_datapattern_wholefile_enough_ram(target_data_t *tdp) {
-	int ret = TRUE;
+xdd_datapattern_wholefile_enough_ram(target_data_t *tdp, const char *filename) {
 	struct sysinfo info;
 	size_t file_size = tdp->td_dpp->data_pattern_length;
 
+	assert(tdp->td_dpp->data_pattern_options & DP_WHOLEFILE_PATTERN);
+
 	// Short circuit empty files
 	if (!file_size)
-		return FALSE;
+		goto error;
 
 	sysinfo(&info);
 	if (file_size > (info.freeram * WHOLEFILE_MAX_SIZE_RAM))
-		ret = FALSE;
+		goto error;;
 
-	return ret;
+	return TRUE;
+
+error:
+	fprintf(xgp->errout, "%s: file %s can not be loaded into memory because 50%% of RAM is "
+		"currently unavailable\n",
+		xgp->progname,
+		filename);
+
+	return FALSE;
 } // End of xdd_datapattern_wholefile_enough_ram() 
 
+/*----------------------------------------------------------------------------*/
+/* xdd_set_datapattern_from_filename() - This subroutine will set the targets
+ * data_pattern from a file. The targets td_dpp->data_pattern_length must be
+ * set before calling this. On error returns 1, and on success returns 0.
+ */
+int
+xdd_set_datapattern_from_filename(target_data_t *tdp, char *filename) {
+	struct xint_data_pattern *dp = tdp->td_dpp;
+	int ret = 0;
+
+	int fd = open(filename, O_RDONLY);
+	if (fd < 0 ) {
+		fprintf(xgp->errout, "%s, could not open %s\n", xgp->progname, filename);
+		return  1;
+	}
+
+	dp->data_pattern_filename = filename;
+	ret = xdd_set_datapattern_from_file_descriptor(tdp, fd, filename);
+	close(fd);
+
+	return ret;
+} // End of xdd_set_datapattern_from_filename() 
+
+/*----------------------------------------------------------------------------*/
+/* xdd_set_datapattern_from_file_descriptor() - This subroutine will set the
+ * targets data_pattern from an open file descriptor. The targets
+ * td_dpp->data_pattern_length must be set before calling this. On error returns 1,
+ * and on success returns 0.
+ */
+int
+xdd_set_datapattern_from_file_descriptor(target_data_t *tdp, int fd, char *filename) {
+	struct xint_data_pattern *dp = tdp->td_dpp;
+
+	// Short circuit if the file is not open
+	if (fd < 0) {
+		return 1;
+	}
+
+	dp->data_pattern_filename = filename;
+	dp->data_pattern = (unsigned char *)malloc(sizeof(unsigned char) * dp->data_pattern_length + 1)	;
+	if (!dp->data_pattern) {
+		fprintf(xgp->errout, "%s: could not allocate datapattern buffer for target %d",
+			xgp->progname, tdp->td_target_number);
+		goto error;
+	}
+
+	size_t bytes = pread(fd, dp->data_pattern, dp->data_pattern_length, 0);
+	if (bytes != dp->data_pattern_length) {
+		fprintf(xgp->errout, "%s: short read from file %s when setting target datapattern. "
+			"Espected %zu bytes but got %zu",
+			xgp->progname, filename, dp->data_pattern_length, bytes);
+			goto error;
+	}
+
+	return 0;
+
+error:
+	free(dp->data_pattern);
+	return 1;
+} // End of xdd_set_datapattern_from_file_descriptor() 
+
 /*----------------------------------------------------------------------------*/
 /* xdd_datapattern_get_datap_from_offset() - This subroutine will return the
  * beginning of a the buffer from the target's tp_dpp->data_pattern based on

src/common/xint_prototypes.h

@@ -35,7 +35,9 @@ int32_t	xdd_barrier(struct xdd_barrier *bp, xdd_occupant_t *occupantp, char owne
 // datapatterns.c
 void	xdd_datapattern_buffer_init(worker_data_t *wdp);
 void	xdd_datapattern_fill(worker_data_t *wdp);
-int		xdd_datapattern_wholefile_enough_ram(target_data_t *tdp);
+int	xdd_datapattern_wholefile_enough_ram(target_data_t *tdp, const char *filename);
+int	xdd_set_datapattern_from_filename(target_data_t *tdp, char *filename);
+int	xdd_set_datapattern_from_file_descriptor(target_data_t *tdp, int fd, char *filename);
 unsigned char *xdd_datapattern_get_datap_from_offset(worker_data_t *wdp);
 
 // debug.c

src/common/xint_td.h

@@ -156,7 +156,6 @@ struct xint_target_data {
 	struct xint_e2e				*td_e2ep;			// Pointer to the e2e struct when needed
 	struct xint_extended_stats	*td_esp;			// Extended Stats Structure Pointer
 	struct xint_triggers		*td_trigp;			// Triggers Structure Pointer
-	int                         dpp_fd;             // File descriptor for file data pattern
 	struct xint_data_pattern	*td_dpp;			// Data Pattern Structure Pointer
 	struct xint_raw				*td_rawp;          	// RAW Data Structure Pointer
 	struct lockstep				*td_lsp;			// Pointer to the lockstep structure used by the lockstep option

tests/functional/CMakeLists.txt

@@ -2,6 +2,7 @@ set(FUNCTIONAL
     test_xdd_createnewfiles.sh
     test_xdd_createnewfiles2.sh
     test_xdd_dryrun.sh
+    test_xdd_file_datapattern.sh
     test_xdd_heartbeat_byte.sh
     test_xdd_heartbeat_elapsed.sh
     test_xdd_heartbeat_lf.sh

tests/functional/test_xdd_file_datapattern.sh

@@ -0,0 +1,56 @@
+#!/usr/bin/env bash
+#
+# Acceptance test for XDD.
+#
+# Validate the funtionality of -datapattern file/wholefile option by creating a file, setting the datapattern
+# from that file and then verifying the contents match
+#
+# Description - terminates XDD after a given amount of seconds have passed
+#
+# Get absolute path to script
+SCRIPT=${BASH_SOURCE[0]}
+SCRIPTPATH=$(dirname "${SCRIPT}")
+
+# Source the test configuration environment
+source "${SCRIPTPATH}"/../test_config
+source "${SCRIPTPATH}"/../common.sh 
+
+# Perform pre-test
+initialize_test
+test_dir="${XDDTEST_LOCAL_MOUNT}/${TESTNAME}"
+log_file="$(get_log_file)"
+
+input_file="${test_dir}/data1.dat"
+output_file="${test_dir}/data2.dat"
+
+# Create datapattern input file using dd
+dd if=/dev/urandom of="${input_file}" bs=4k count=2
+
+# Write out file using xdd with -datapattern file
+"${XDDTEST_XDD_EXE}" -op write -reqsize 8 -numreqs 1 -datapattern file "${input_file}" -targets 1 "${output_file}" -qd 1 -passes 1
+# Verify the contents of the file match
+if ! cmp "${input_file}" "${output_file}"
+then
+    echo "Error when comparing files ${input_file} and ${output_file} with -datapattern file" > "${log_file}"
+    cmp "${input_file}" "${output_file}" >> "${log_file}"
+    rm "${input_file}" "${output_file}"
+    finalize_test 1 "Issue with setting datapattern from file with -datapattern file. Contents don't match."
+fi
+
+rm "${output_file}"
+# Write out file using xdd with -datapattern wholefile
+# In this caes we will use two threads to write out the data in serial ordering as the
+# contents of the file will be distributed evenly between both threads.
+"${XDDTEST_XDD_EXE}" -op write -reqsize 4 -numreqs 2 -datapattern wholefile "${input_file}" -targets 1 "${output_file}" -serialordering -qd 2 -passes 1
+# Verify the contents of the file match
+if ! cmp "${input_file}" "${output_file}"
+then
+    echo "Error when comparing files ${input_file} and ${output_file} with -datapattern wholefile" > "${log_file}"
+    cmp "${input_file}" "${output_file}" >> "${log_file}"
+    rm "${input_file}" "${output_file}"
+    finalize_test 1 "Issue with setting datapattern from file with -datapattern file. Contents don't match."
+fi
+rm "${input_file}" "${output_file}"
+
+# Test passed
+finalize_test 0