Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43b354b
Allow the user to specify several devices as arguments to dasdfmt.
markkp Jun 29, 2021
14d0278
Allow dasdfmt to run in parallel when several devices are specified.
markkp Jun 29, 2021
2bde08f
Update the boot menu message to show that kernel parameters can also …
markkp Jun 29, 2021
72e6104
Merge branch 'ibm-s390-linux:master' into master
markkp Oct 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions dasdfmt/dasdfmt.8
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
dasdfmt \- formatting of DASD (ECKD) disk drives.

.SH SYNOPSIS
\fBdasdfmt\fR [-h] [-t] [-v] [-y] [-p] [-P] [-m \fIstep\fR]
\fBdasdfmt\fR [-h] [-t] [-v] [-y] [-p] [-Q] [-P] [-m \fIstep\fR]
.br
[-r \fIcylinder\fR] [-b \fIblksize\fR] [-l \fIvolser\fR] [-d \fIlayout\fR]
.br
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Print one line for each formatted cylinder showing the number of the
cylinder and percentage of formatting process.
Intended to be used by higher level interfaces.
Expand Down Expand Up @@ -158,6 +158,18 @@ devices, counting the base device and all alias devices.
Specify blocksize to be used. \fIblksize\fR must be a positive integer
and always be a power of two. The recommended blocksize is 4096 bytes.

.TP
\fB-P\fR \fInumdisks\fR or \fB--max_parallel\fR=\fInumdisks\fR
markkp marked this conversation as resolved.
Show resolved Hide resolved
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.br
Using this option can decrease overall processing time when formatting
several disks. Please note that the I/O throughput will dramatically
increase when using this option. Use with care.
.br

.TP
\fB-l\fR \fIvolser\fR or \fB--label\fR=\fIvolser\fR
Specify the volume serial number or volume identifier to be written
Expand Down
58 changes: 46 additions & 12 deletions dasdfmt/dasdfmt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <sys/sysmacros.h>
#include <sys/time.h>
#include <sys/utsname.h>
#include <sys/wait.h>

#include "lib/dasd_base.h"
#include "lib/dasd_sys.h"
Expand Down Expand Up @@ -81,6 +82,7 @@ static struct dasdfmt_globals {
int mode_specified;
int ese;
int no_discard;
int procnum;
} g = {
.dasd_info = { 0 },
};
Expand All @@ -105,6 +107,11 @@ static struct util_opt opt_vec[] = {
.desc = "Perform complete format check on device",
.flags = UTIL_OPT_FLAG_NOSHORT,
},
{
.option = { "max_parallel", required_argument, NULL, 'P' },
.desc = "Format devices in parallel",
.flags = UTIL_OPT_FLAG_NOLONG,
},
UTIL_OPT_SECTION("FORMAT OPTIONS"),
{
.option = { "blocksize", required_argument, NULL, 'b' },
Expand Down Expand Up @@ -162,7 +169,7 @@ static struct util_opt opt_vec[] = {
.desc = "Show a progressbar",
},
{
.option = { "percentage", no_argument, NULL, 'P' },
.option = { "percentage", no_argument, NULL, 'Q' },
.desc = "Show progress in percent",
},
UTIL_OPT_SECTION("MISC"),
Expand Down Expand Up @@ -311,7 +318,7 @@ static void draw_progress(int cyl, unsigned int cylinders, int aborted)
}

if (g.print_hashmarks && (cyl / g.hashstep - hashcount) != 0) {
printf("#");
printf("%d|", g.procnum);
fflush(stdout);
hashcount++;
}
Expand Down Expand Up @@ -1560,7 +1567,11 @@ int main(int argc, char *argv[])
char *reqsize_param_str = NULL;
char *hashstep_str = NULL;

int rc, numdev = 0, i;
int rc, numdev = 0, numproc = 0, status;
int max_parallel =1 ;
int running = 0;
int chpid;
int tmp;

/* Establish a handler for interrupt signals. */
signal(SIGTERM, program_interrupt_signal);
Expand Down Expand Up @@ -1623,7 +1634,7 @@ int main(int argc, char *argv[])
g.print_hashmarks = 1;
}
break;
case 'P':
case 'Q':
if (!(g.print_hashmarks || g.print_progressbar))
g.print_percentage = 1;
break;
Expand Down Expand Up @@ -1682,6 +1693,9 @@ int main(int argc, char *argv[])
case OPT_NODISCARD:
g.no_discard = 1;
break;
case 'P':
max_parallel = atoi(optarg);
break;
case OPT_CHECK:
g.check = 1;
break;
Expand Down Expand Up @@ -1733,15 +1747,35 @@ int main(int argc, char *argv[])
if (numdev > 1 && g.labelspec)
error("Specifying a volser to be written doesn't make sense when formatting multiple DASD volumes.");

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);
process_dasd(&vlabel, format_params);
for (numproc = 0; numproc < numdev; numproc++) {
chpid = fork();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@markkp markkp Oct 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (chpid == -1 )
ERRMSG_EXIT(EXIT_FAILURE,
"%s: Unable to create child process: %s\n",
prog_name, strerror(errno));
if (!chpid) {
g.procnum = numproc;
strncpy(g.dev_path, g.dev_path_array[numproc], strlen(g.dev_path_array[numproc])+1);
strncpy(g.dev_node, g.dev_node_array[numproc], strlen(g.dev_node_array[numproc])+1);
process_dasd(&vlabel, format_params);

free(g.dev_path);
free(g.dev_node);
exit(0);
} else {
running++;
if (running >= max_parallel) {
if (wait(&tmp) > 0 && WEXITSTATUS(tmp))
rc = WEXITSTATUS(tmp);
running--;
}
}
}

free(g.dev_path);
free(g.dev_node);
/* wait until all formatting children have finished */
while(wait(&status) > 0)
if (WEXITSTATUS(status))
rc = WEXITSTATUS(status);

return 0;
return rc;
}