Allow multiple device arguments to dasdfmt, and allow them to be formatted in parallel. #115

markkp · 2021-06-29T19:29:22Z

No description provided.

Signed-off-by: Hannes Reinecke <[email protected]> Signed-off-by: Mark Post <[email protected]>

…be entered via the 'vi vmsg' CP command. Signed-off-by: Mark Post <[email protected]>

frank-heimes · 2021-06-29T19:48:54Z

That's a great enhancement!

markkp · 2021-06-29T19:57:52Z

Hmm. I did not intend for the boot menu message commit to be part of this pull request. I guess I need to learn how to use github better. :(

markkp · 2021-08-03T15:34:38Z

On 8/3/21 11:27 AM, Marc Hartmayer wrote: This change seems to be unrelated.

That's because it is unrelated. As I commented after creating the pull request. Mark Post

hoeppnerj

Please add a more descriptive commit message as well.

hoeppnerj · 2021-10-15T10:01:24Z

dasdfmt/dasdfmt.c

-	}
+	if (!numdev)
+		error("%s: No device specified!\n",
+			prog_name);


This could be written in one line.
Also, there is a check in get_device_name()

s390-tools/dasdfmt/dasdfmt.c

Lines 518 to 519 in 895dc80

if (optind >= argc)

error("No device specified!");

that checks for "No device" already. You're calling get_device_name() in line 1721 in the loop. The check could be moved out before entering the loop and then removed here.

I'm not quite sure what you mean. If you're referring to the "while (optind < argc)" as the loop, then are you saying move the call to get_device_name on line 1735 to just before "while (optind < argc)", or something else.

Yes, within the while (optind < argc) loop your first call is get_device_name(). Within that function the first check
is the one I referenced above (optind >= argc). You are essentially doing the exact same check after the loop
with if (!numdev). That check however, is pretty much a dead code path as get_device_name() would fail first.

With moving the check out of the loop, I mean that the check could be done right before while (optind < argc)
and can then be removed from get_device_name().
I also just figured, that the check for the label if (numdev > 1 && g.labelspec) could also be moved before
the loop.

It would look like this:

if (optind >= argc) error("No device specified!"); if ((argc - optind) > 1 && g.labelspec) error("Specifying a volser to be written doesn't make sense when formatting multiple DASD volumes."); while (optind < argc) { get_device_name(optind, argc, argv); strncpy(g.dev_path_array[numdev], g.dev_path, strlen(g.dev_path)); strncpy(g.dev_node_array[numdev], g.dev_node, strlen(g.dev_node)); optind++; numdev++; }

hoeppnerj · 2021-10-15T10:02:10Z

dasdfmt/dasdfmt.c

@@ -1644,6 +1696,9 @@ int main(int argc, char *argv[])
 			break; /* exit loop if finished */
 	}

+	/* Reset the value of rc since we're going to use it again later. */
+	rc = 0;
+


This change should go into the second patch. It's not relevant here.

I'll double check that to be sure I didn't get something else wrong.

hoeppnerj · 2021-10-15T12:14:22Z

dasdfmt/dasdfmt.c

+	for (i = 0; i < numdev; i++)
+	{
+		strncpy(g.dev_path, g.dev_path_array[i], strlen(g.dev_path_array[i])+1);
+		strncpy(g.dev_node, g.dev_node_array[i], strlen(g.dev_node_array[i])+1);	


These two lines produce some warnings and I think the string operations can be avoided.
See below for a different approach.

In function ‘strncpy’, inlined from ‘main’ at dasdfmt.c:1742:3: /usr/include/s390x-linux-gnu/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound depends on the length of the source argument [-Wstringop-overflow=] 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ dasdfmt.c: In function ‘main’: dasdfmt.c:1742:44: note: length computed here 1742 | strncpy(g.dev_path, g.dev_path_array[i], strlen(g.dev_path_array[i])+1); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~

char *tmp_path = g.dev_path; char *tmp_node = g.dev_node; for (i = 0; i < numdev; i++) { g.dev_path = g.dev_path_array[i]; g.dev_node = g.dev_node_array[i]; process_dasd(&vlabel, format_params); } free(tmp_path); free(tmp_node);

Hmm. I don't get that warning, or I would have worked to eliminate it. What version of gcc are you using?
Not being particularly strong in C, I don't understand what you're doing here. How do the *tmp_ variables come into play here?

hoeppnerj · 2021-10-15T13:19:28Z

dasdfmt/dasdfmt.c

+	while (optind < argc) {
+		get_device_name(optind, argc, argv);
+		strncpy(g.dev_path_array[numdev], g.dev_path, strlen(g.dev_path));
+		strncpy(g.dev_node_array[numdev], g.dev_node, strlen(g.dev_node));


strncpy() is tricky here. As it is, the following warning is produced:

In function ‘strncpy’, inlined from ‘main’ at dasdfmt.c:1722:3: /usr/include/s390x-linux-gnu/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation] 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ dasdfmt.c: In function ‘main’: dasdfmt.c:1722:3: note: length computed here 1722 | strncpy(g.dev_path_array[numdev], g.dev_path, strlen(g.dev_path)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Adding +1 to make sure the terminating nul is copied as well will produce another warning:

In function ‘strncpy’, inlined from ‘main’ at dasdfmt.c:1722:3: /usr/include/s390x-linux-gnu/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound depends on the length of the source argument [-Wstringop-overflow=] 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ dasdfmt.c: In function ‘main’: dasdfmt.c:1722:49: note: length computed here 1722 | strncpy(g.dev_path_array[numdev], g.dev_path, strlen(g.dev_path) + 1); | ^~~~~~~~~~~~~~~~~~

In this scenario I think strcpy() should suffice. However, we have util_strlcpy() which is probably the even better solution to provide some safety. It would look like this and the warnings would be gone:

util_strlcpy(g.dev_path_array[numdev], g.dev_path, MAX_LENGTH); util_strlcpy(g.dev_node_array[numdev], g.dev_node, MAX_LENGTH);

I'll take a look at your util function.

hoeppnerj · 2021-10-15T13:24:26Z

dasdfmt/dasdfmt.c

 {
 	volume_label_t vlabel;
 	char old_volser[7];
-
 	char str[ERR_LENGTH];


While you're touching this area anyway, would be nice to format it in reverse xmas tree here.

I don't understand what you mean by that. Format what? and what is "reverse xmas tree?"

It's a format style that we try to follow where the order of the variable declarations is determined by the
length starting with the longest line at the top. In this example it would look like this:

volume_label_t vlabel; char str[ERR_LENGTH]; char old_volser[7];

hoeppnerj · 2021-10-15T13:29:42Z

dasdfmt/dasdfmt.c

@@ -25,6 +25,8 @@

 #include "dasdfmt.h"

+#define MAX_DEVICES 512
+#define MAX_LENGTH 256


There is a check in get_device_name() that should probably now check against that value:

s390-tools/dasdfmt/dasdfmt.c

Lines 521 to 522 in 895dc80

if (strlen(argv[optind]) >= PATH_MAX)

error("device name too long!");

hoeppnerj · 2021-10-15T13:40:13Z

dasdfmt/dasdfmt.c

+	eval_format_mode();
+
+	/* Not sure this next line is needed in the new version of the code. */
+	memcpy(&vlabel, orig_vlabel, sizeof(vlabel));


I don't think is line is relevant here. Just use orig_vlabel (and call it vlabel)

I'll give it a try.

hoeppnerj · 2021-10-15T14:02:02Z

dasdfmt/dasdfmt.8

@@ -96,7 +96,7 @@ Do not use this option if you are using a 3270 console,
 running in background or redirecting the output to a file.

 .TP
-\fB-P\fR or \fB--percentage\fR
+\fB-Q\fR or \fB--percentage\fR


I know it's sad that both p and P are taken, but please don't change the behaviour of an established command line option.

I already warned you about this, so no surprise here.

Sure, but still wanted to mentioned that we shouldn't change it.

dasdfmt/dasdfmt.8

hoeppnerj · 2021-10-15T14:15:36Z

dasdfmt/dasdfmt.c

-		strncpy(g.dev_node, g.dev_node_array[i], strlen(g.dev_node_array[i])+1);	
-		process_dasd(&vlabel, format_params);
+	for (numproc = 0; numproc < numdev; numproc++) {
+		chpid = fork();


I have one major issue with the fork() approach. The output is very clutttered:

[root@m83lp46 ~]# ./dasdfmt -y -p -b 4096 -P 4 /dev/dasdb /dev/dasdc /dev/dasdd /dev/dasde WARNING: Disk /dev/dasdd is online on operating system instances in 58 different LPARs. Ensure that the disk is not being used by a system outside your LPAR. Note: Your installation might include z/VM systems that are configured to automatically vary on disks, regardless of whether they are subsequently used. WARNING: Disk /dev/dasdb is online on operating system instances in 58 different LPARs. Ensure that the disk is not being used by a system outside your LPAR. Note: Your installation might include z/VM systems that are configured to automatically vary on disks, regardless of whether they are subsequently used. WARNING: Disk /dev/dasde is online on operating system instances in 58 different LPARs. Ensure that the disk is not being used by a system outside your LPAR. Note: Your installation might include z/VM systems that are configured to automatically vary on disks, regardless of whether they are subsequently used. WARNING: Disk /dev/dasdc is online on operating system instances in 58 different LPARs. Ensure that the disk is not being used by a system outside your LPAR. Note: Your installation might include z/VM systems that are configured to automatically vary on disks, regardless of whether they are subsequently used. cyl 1113 of 1113 |#################################|100% [5s] cyl 1113 of 1113 |#################################|100% [5s] Finished formatting the /dev/dasdb device. Finished formatting the /dev/dasdd device. cyl 1113 of 1113 |#################################|100% [5s] Rereading the partition table for /dev/dasdb... ok Rereading the partition table for /dev/dasdd... ok Finished formatting the /dev/dasdc device. Rereading the partition table for /dev/dasdc... ok cyl 1113 of 1113 |#################################|100% [5s] Finished formatting the /dev/dasde device. Rereading the partition table for /dev/dasde... ok

This is hard to consume and very confusing (and this is for only 4 devices).
This lies in the nature of fork() as the process is just cloned and all the output
is directed to the same terminal as it comes in.
I'd rather appreciate an approach using pthreads, which allows to track the progress of each
formatting run. Also, it should allow for more fine grained control over each formatting process
and outputs could then be displayed as required.

I would not be capable of writing anything with pthreads. Sorry. I wouldn't be opposed to someone else who knows that they're doing in that regard taking on that task.

We would still need a solution to aggregate the output. If pthreads are to complicated at this point, one solution
that came to mind whilst discussing this with my team was pipe() (See man 2 pipe). Which seems rather
easy to implement and you could collect the status of each forked process and work with that in the main
process. That way, output could be reduced to a meaningful and easy to consume minimum.

hoeppnerj · 2021-10-15T14:19:45Z

dasdfmt/dasdfmt.8

+Specify the number of disks to be formatted in parallel.
+\fInumdisks\fR specifies the number of formatting processed,
+independent on the overall number of disks to be formatted.
+The maximum value for \fInumdisks\fR is 512. Default is 1.


I think the default value should be numdev and the number the user can set should
be an optional parameter.

I'll have to think about that. I think that might be an unexpected change for some people.

I have in mind that at one point we could allow the user to specify ranges of devices. Just like for other tools
like zdev. So, one could just do dasdfmt -b 4096 -y -p -n 0.0.9300-0.9330 and all devices would
automatically be formatted in parallel.
The number to specify the amount of devices that should be formatted in parallel would be optional.
That way the user doesn't need to bother with counting the amount of devices but still has the option to limit
it if resources are a constraint.

markkp

Please add a more descriptive commit message as well.

Just what are you looking for here?

markkp · 2021-10-19T18:33:05Z

dasdfmt/dasdfmt.c

+	for (i = 0; i < numdev; i++)
+	{
+		strncpy(g.dev_path, g.dev_path_array[i], strlen(g.dev_path_array[i])+1);
+		strncpy(g.dev_node, g.dev_node_array[i], strlen(g.dev_node_array[i])+1);	


Hmm. I don't get that warning, or I would have worked to eliminate it. What version of gcc are you using?
Not being particularly strong in C, I don't understand what you're doing here. How do the *tmp_ variables come into play here?

markkp · 2021-10-19T18:33:41Z

dasdfmt/dasdfmt.c

+	while (optind < argc) {
+		get_device_name(optind, argc, argv);
+		strncpy(g.dev_path_array[numdev], g.dev_path, strlen(g.dev_path));
+		strncpy(g.dev_node_array[numdev], g.dev_node, strlen(g.dev_node));


I'll take a look at your util function.

markkp · 2021-10-19T18:34:13Z

dasdfmt/dasdfmt.c

@@ -1644,6 +1696,9 @@ int main(int argc, char *argv[])
 			break; /* exit loop if finished */
 	}

+	/* Reset the value of rc since we're going to use it again later. */
+	rc = 0;
+


I'll double check that to be sure I didn't get something else wrong.

dasdfmt/dasdfmt.8

markkp · 2021-10-19T18:36:41Z

dasdfmt/dasdfmt.8

+Specify the number of disks to be formatted in parallel.
+\fInumdisks\fR specifies the number of formatting processed,
+independent on the overall number of disks to be formatted.
+The maximum value for \fInumdisks\fR is 512. Default is 1.


I'll have to think about that. I think that might be an unexpected change for some people.

markkp added 3 commits June 29, 2021 15:25

Allow the user to specify several devices as arguments to dasdfmt.

43b354b

Signed-off-by: Hannes Reinecke <[email protected]> Signed-off-by: Mark Post <[email protected]>

Allow dasdfmt to run in parallel when several devices are specified.

14d0278

Signed-off-by: Hannes Reinecke <[email protected]> Signed-off-by: Mark Post <[email protected]>

Update the boot menu message to show that kernel parameters can also …

2bde08f

…be entered via the 'vi vmsg' CP command. Signed-off-by: Mark Post <[email protected]>

hoeppnerj self-assigned this Jul 7, 2021

hoeppnerj added the enhancement label Jul 7, 2021

hoeppnerj requested changes Oct 15, 2021

View reviewed changes

markkp commented Oct 19, 2021

View reviewed changes

Merge branch 'ibm-s390-linux:master' into master

72e6104

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple device arguments to dasdfmt, and allow them to be formatted in parallel. #115

Allow multiple device arguments to dasdfmt, and allow them to be formatted in parallel. #115

markkp commented Jun 29, 2021

frank-heimes commented Jun 29, 2021

markkp commented Jun 29, 2021

markkp commented Aug 3, 2021 via email

hoeppnerj left a comment

hoeppnerj Oct 15, 2021

markkp Oct 19, 2021

hoeppnerj Oct 20, 2021

hoeppnerj Oct 15, 2021

markkp Oct 19, 2021

hoeppnerj Oct 15, 2021

markkp Oct 19, 2021

hoeppnerj Oct 15, 2021

markkp Oct 19, 2021

hoeppnerj Oct 15, 2021

markkp Oct 19, 2021

hoeppnerj Oct 20, 2021

hoeppnerj Oct 15, 2021

hoeppnerj Oct 15, 2021

markkp Oct 19, 2021

hoeppnerj Oct 15, 2021

markkp Oct 15, 2021

hoeppnerj Oct 20, 2021

hoeppnerj Oct 15, 2021

markkp Oct 15, 2021 •

edited

Loading

hoeppnerj Oct 20, 2021

hoeppnerj Oct 15, 2021

markkp Oct 19, 2021

hoeppnerj Oct 20, 2021

markkp left a comment

markkp Oct 19, 2021

markkp Oct 19, 2021

markkp Oct 19, 2021

markkp Oct 19, 2021

	if (strlen(argv[optind]) >= PATH_MAX)
	error("device name too long!");

Allow multiple device arguments to dasdfmt, and allow them to be formatted in parallel. #115

Are you sure you want to change the base?

Allow multiple device arguments to dasdfmt, and allow them to be formatted in parallel. #115

Conversation

markkp commented Jun 29, 2021

frank-heimes commented Jun 29, 2021

markkp commented Jun 29, 2021

markkp commented Aug 3, 2021 via email

hoeppnerj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markkp Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markkp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markkp Oct 15, 2021 •

edited

Loading