[PATCH] fsck: implement fsck -r {fd}

[PATCH] fsck: implement fsck -r {fd}

[Stanislav Brabec]

Make possible sending of statistics to a dedicated file descriptor.

Rationale: When UI is calling fsck from a remote terminal, fsck progress
needs to be sent to stdout. It is mixed there with output of statistics,
and it is impossible to parse the output to get the statistics.

Now it will be possible e. g. with "fsck -C -r 3 /dev/sda1"

Note: Code in if and else is intentionally partially duplicated. Current
human readable output of floats does not conform to locale conventions,
and may be changed in future. But we want to keep machine readable output
exactly same as it is now.

Signed-off-by: Stanislav Brabec <sbrabec@suse.cz>
---
 disk-utils/fsck.8 | 13 ++++++++++--
 disk-utils/fsck.c | 63 ++++++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/disk-utils/fsck.8 b/disk-utils/fsck.8
index c2bb7bf..df7d095 100644
--- a/disk-utils/fsck.8
+++ b/disk-utils/fsck.8
@@ -6,7 +6,9 @@
 fsck \- check and repair a Linux filesystem
 .SH SYNOPSIS
 .B fsck
-.RB [ \-lrsAVRTMNP ]
+.RB [ \-lsAVRTMNP ]
+.RB [ \-r
+.RI [ fd ]]
 .RB [ \-C
 .RI [ fd ]]
 .RB [ \-t
@@ -100,13 +102,20 @@ multiple devices or for non-rotating disks.  \fBfsck\fR does not lock underlying
 devices when executed to check stacked devices (e.g.\& MD or DM) \(en this feature is
 not implemented yet.
 .TP
-.B \-r
+.BR \-r \ [ \fIfd\fR ]
 Report certain statistics for each fsck when it completes.  These statistics
 include the exit status, the maximum run set size (in kilobytes), the elapsed
 all-clock time and the user and system CPU time used by the fsck run.  For
 example:
 
 /dev/sda1: status 0, rss 92828, real 4.002804, user 2.677592, sys 0.86186
+
+GUI front-ends may specify a file descriptor
+.IR fd ,
+in which case the progress bar information will be sent to that file descriptor
+in a machine parseable format.  For example:
+
+/dev/sda1 0 92828 4.002804 2.677592 0.86186
 .TP
 .B \-s
 Serialize
diff --git a/disk-utils/fsck.c b/disk-utils/fsck.c
index 299a775..27ffad3 100644
--- a/disk-utils/fsck.c
+++ b/disk-utils/fsck.c
@@ -141,6 +141,8 @@ static int progress;
 static int progress_fd;
 static int force_all_parallel;
 static int report_stats;
+static int report_stats_fd;
+static FILE *report_stats_file;
 
 static int num_running;
 static int max_running;
@@ -586,16 +588,28 @@ static void print_stats(struct fsck_instance *inst)
 
 	timersub(&inst->end_time, &inst->start_time, &delta);
 
-	fprintf(stdout, "%s: status %d, rss %ld, "
-			"real %ld.%06ld, user %d.%06d, sys %d.%06d\n",
-		fs_get_device(inst->fs),
-		inst->exit_status,
-		inst->rusage.ru_maxrss,
-		delta.tv_sec, delta.tv_usec,
-		(int)inst->rusage.ru_utime.tv_sec,
-		(int)inst->rusage.ru_utime.tv_usec,
-		(int)inst->rusage.ru_stime.tv_sec,
-		(int)inst->rusage.ru_stime.tv_usec);
+	if (report_stats_fd)
+		fprintf(report_stats_file, "%s %d %ld "
+					   "%ld.%06ld %d.%06d %d.%06d\n",
+			fs_get_device(inst->fs),
+			inst->exit_status,
+			inst->rusage.ru_maxrss,
+			delta.tv_sec, delta.tv_usec,
+			(int)inst->rusage.ru_utime.tv_sec,
+			(int)inst->rusage.ru_utime.tv_usec,
+			(int)inst->rusage.ru_stime.tv_sec,
+			(int)inst->rusage.ru_stime.tv_usec);
+	else
+		fprintf(stdout, "%s: status %d, rss %ld, "
+				"real %ld.%06ld, user %d.%06d, sys %d.%06d\n",
+			fs_get_device(inst->fs),
+			inst->exit_status,
+			inst->rusage.ru_maxrss,
+			delta.tv_sec, delta.tv_usec,
+			(int)inst->rusage.ru_utime.tv_sec,
+			(int)inst->rusage.ru_utime.tv_usec,
+			(int)inst->rusage.ru_stime.tv_sec,
+			(int)inst->rusage.ru_stime.tv_usec);
 }
 
 /*
@@ -1371,11 +1385,12 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
 	fputs(_(" -N         do not execute, just show what would be done\n"), out);
 	fputs(_(" -P         check filesystems in parallel, including root\n"), out);
 	fputs(_(" -R         skip root filesystem; useful only with '-A'\n"), out);
-	fputs(_(" -r         report statistics for each device checked\n"), out);
+	fputs(_(" -r [<fd>]  report statistics for each device checked;\n"
+		"            file descriptor is for GUIs\n"), out);
 	fputs(_(" -s         serialize the checking operations\n"), out);
 	fputs(_(" -T         do not show the title on startup\n"), out);
 	fputs(_(" -t <type>  specify filesystem types to be checked;\n"
-		"             <type> is allowed to be a comma-separated list\n"), out);
+		"            <type> is allowed to be a comma-separated list\n"), out);
 	fputs(_(" -V         explain what is being done\n"), out);
 	fputs(_(" -?         display this help and exit\n"), out);
 
@@ -1504,6 +1519,21 @@ static void parse_argv(int argc, char *argv[])
 				break;
 			case 'r':
 				report_stats = 1;
+				if (arg[j+1]) {					/* -r<fd> */
+					report_stats_fd = string_to_int(arg+j+1);
+					if (report_stats_fd < 0)
+						report_stats_fd = 0;
+					else
+						goto next_arg;
+				} else if (i+1 < argc && *argv[i+1] != '-') {	/* -r <fd> */
+					report_stats_fd = string_to_int(argv[i+1]);
+					if (report_stats_fd < 0)
+						report_stats_fd = 0;
+					else {
+						++i;
+						goto next_arg;
+					}
+				}
 				break;
 			case 's':
 				serialize = 1;
@@ -1569,6 +1599,15 @@ int main(int argc, char *argv[])
 
 	parse_argv(argc, argv);
 
+	/* Validate the report stats file descriptor to avoid disasters */
+	if (report_stats_fd) {
+		report_stats_file = fdopen(report_stats_fd, "w");
+		if (!report_stats_file)
+			err(FSCK_EX_ERROR,
+				_("invalid argument -r %d"),
+				report_stats_fd);
+	}
+
 	if (!notitle)
 		printf(UTIL_LINUX_VERSION);
 
-- 
2.3.5

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec@suse.cz
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] fsck: implement fsck -r {fd}

[Karel Zak]

 Applied, thanks. See notes below.

On Mon, Apr 27, 2015 at 05:20:55PM +0200, Stanislav Brabec wrote:
> Note: Code in if and else is intentionally partially duplicated. Current
> human readable output of floats does not conform to locale conventions,
> and may be changed in future. But we want to keep machine readable output
> exactly same as it is now.

The question is if we can change the human readable output. It was
requested by Google and I guess they parse the output.

> +static int report_stats_fd;
> +static FILE *report_stats_file;

It seems that report_stats_fd does not have to be global variable,
code depends on report_stats_file. Fixed.

>  			case 'r':
>  				report_stats = 1;
> +				if (arg[j+1]) {					/* -r<fd> */
> +					report_stats_fd = string_to_int(arg+j+1);
> +					if (report_stats_fd < 0)
> +						report_stats_fd = 0;
> +					else
> +						goto next_arg;
> +				} else if (i+1 < argc && *argv[i+1] != '-') {	/* -r <fd> */
> +					report_stats_fd = string_to_int(argv[i+1]);
> +					if (report_stats_fd < 0)
> +						report_stats_fd = 0;
> +					else {
> +						++i;
> +						goto next_arg;
> +					}
> +				}
>  				break;

...you have copied code from -C, right?

The -C code interprets errors or missing fd as 0, it's probably
because fsck.extN requires file descriptor for -C, but fsck(8) has the
fd as optional argument.

I don't think we need this behavior for -r too. It would be better to
report all possible mistakes. Fixed.

> +	/* Validate the report stats file descriptor to avoid disasters */
> +	if (report_stats_fd) {
> +		report_stats_file = fdopen(report_stats_fd, "w");
> +		if (!report_stats_file)
> +			err(FSCK_EX_ERROR,
> +				_("invalid argument -r %d"),
> +				report_stats_fd);
> +	}
> +

It means that "-r 0" is unsupported, why? I have fixed this.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] fsck: implement fsck -r {fd}

[Theodore Ts'o]

On Tue, Apr 28, 2015 at 01:52:10PM +0200, Karel Zak wrote:
> 
> On Mon, Apr 27, 2015 at 05:20:55PM +0200, Stanislav Brabec wrote:
> > Note: Code in if and else is intentionally partially duplicated. Current
> > human readable output of floats does not conform to locale conventions,
> > and may be changed in future. But we want to keep machine readable output
> > exactly same as it is now.
> 
> The question is if we can change the human readable output. It was
> requested by Google and I guess they parse the output.

We're not parsing the output; it's something that we look at in the
log files manually (i.e., by humans when trying to debug a problem.)

The bigger deal for us not just mixing the stats and the rest of the
fsck output, but if you have dozen(s) of disks after a power failure,
and you're running without a journal, multiple drives will have
inconsistencies that need fixing, and so we divert the output for each
fsck process to a separate log file for each fsck process / file
system.

The changes to separate out the fsck output into separate files is a
change which I don't think ever made it upstream.  IIRC, we submitted
the patches upstream, there were some objections, and I never had time
to get back to trying to make something that would work for us and
acceptable upstream; my bad.  If you want I can try to dig it up and
try resubmitting it.

Alternatively, upstream e2fsck also has a way of having e2fsck direct
its output to individual files, which is a bit more flexible and IMO a
bit cleaner than the fsck changes.  I suspect the right answer is to
get something like this into fsck, but the problem is where to put the
formatting information, since fsck doesn't a configuration file ala
/etc/e2fsck.conf, where something like this can be dropped:

[options]
    log_dir = /var/log/fsck
    log_filename = e2fsck-%N.%h.%D-%T
    log_dir_wait = true
    report_time = true
    report_verbose = true
    report_features = true

See the man page for e2fsck.conf, looking at the LOGGING section and
the definition of log_* in the options configuration stanza for more
details.

If I recall correctly, it was the fact that fsck didn't have a
configuration file which made the fsck patches a bit grotty, since
there was no place to put the information to customize the logging
output, and it's highly likely that what would work for Google might
not be all that acceptable for other sites.  That's why I ended up
coding the e2fsck changes, but we never got around to switching over,
since our out-of-tree fsck patches worked well enough, even if they
could never go upstream.  I can try resending the fsck patches, but I
wouldn't blame you if you considered them too ugly to live.

Let me know if you have preferences of how you would like to proceed.

Cheers,

					- Ted

Re: [PATCH] fsck: implement fsck -r {fd}

[Stanislav Brabec]

Karel Zak wrote:
>
>   Applied, thanks. See notes below.
>
> On Mon, Apr 27, 2015 at 05:20:55PM +0200, Stanislav Brabec wrote:
>> Note: Code in if and else is intentionally partially duplicated. Current
>> human readable output of floats does not conform to locale conventions,
>> and may be changed in future. But we want to keep machine readable output
>> exactly same as it is now.
>
> The question is if we can change the human readable output. It was
> requested by Google and I guess they parse the output.
>
I created the alternative output format just for purpose of machine 
parsing: No need for scanf(), just space separated values.

I was also thinking about possibility to switch between human-readable 
and easily parseable output even to a specified FD, but I did not 
implement it.

I could be easy: Positive number would mean easily parseable output, 
negative number human readable. Just zero will be a problem (but we can 
still parse it on ASCII level and allow -r-0.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec@suse.cz
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] fsck: implement fsck -r {fd}

[Stanislav Brabec]

Theodore Ts'o wrote:

> We're not parsing the output; it's something that we look at in the
> log files manually (i.e., by humans when trying to debug a problem.)

SUSE boot scripts parse this output (well, it uses just a part of the 
information) to synchronize fsck and quota activation. Passing an opened 
file descriptor is a good solution for these purposes.

This needs just the one-line statistics generated by fsck, underlying FS 
specific statistics are not needed.

> The bigger deal for us not just mixing the stats and the rest of the
> fsck output, but if you have dozen(s) of disks after a power failure,
> and you're running without a journal, multiple drives will have
> inconsistencies that need fixing, and so we divert the output for each
> fsck process to a separate log file for each fsck process / file
> system.

My patch just allows separating of the progress and statistics, but not 
splitting it per volume. (If you want human readable output, please see 
my reply to this note. (e. g. use -r -3).

Splitting per volumen is another issue, and use of -C {fd} is 
insufficient for it. But -r {fd} is probably sufficient for all use 
cases of the one-line-per-volume summary statistics.

fsck can now use only a subset of e2fsck features.

> The changes to separate out the fsck output into separate files is a
> change which I don't think ever made it upstream.  IIRC, we submitted
> the patches upstream, there were some objections, and I never had time
> to get back to trying to make something that would work for us and
> acceptable upstream; my bad.  If you want I can try to dig it up and
> try resubmitting it.

The format of the machine parseable output of -C {fd} allows to separate 
progress for particular discs, but yes, if you want them separated and 
human readable, it is a bad approach to do it that way.

I can imagine better configurability of fsck, and maybe seamless 
integration with udev/systemd. But I have no idea what is the best way.

Keeping a possibility to send progress to an opened FD is required by 
GUI frontends, so it needs to be kept.

  I suspect the right answer is to
> get something like this into fsck, but the problem is where to put the
> formatting information, since fsck doesn't a configuration file ala
> /etc/e2fsck.conf, where something like this can be dropped:

Configuration file could be a solution. SUSE could easily live even 
without it, so I cannot give an advice how to do it best.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec@suse.cz
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] fsck: implement fsck -r {fd}

[Theodore Ts'o]

On Tue, Apr 28, 2015 at 05:56:08PM +0200, Stanislav Brabec wrote:
> 
> >We're not parsing the output; it's something that we look at in the
> >log files manually (i.e., by humans when trying to debug a problem.)
> 
> SUSE boot scripts parse this output (well, it uses just a part of the
> information) to synchronize fsck and quota activation. Passing an opened
> file descriptor is a good solution for these purposes.
> 
> This needs just the one-line statistics generated by fsck, underlying FS
> specific statistics are not needed.

Actually, what of the one-line statistics do you need?  Is it just
"we're done checking /dev/sdXX", so you can activate quotas for
/dev/sdXX?  If so, one of the other patches which Frank Mayhar (one of
my collegaues) implemented the ability for fsck to run a "completion
handler" which was a program/shell script that would get executed for
each file system the check was completed -- and which would pass the
exit status for each specific file system along to the completion
handler.  That allowed us to take specific action on a per-file system
basis if a file system couldn't be repaired, without having to parse
any of the log outputs to determine which file system had
uncorrectable file system problems.

I wonder if something like that would be useful for SuSE?

Regards,

						- Ted
						

Re: [PATCH] fsck: implement fsck -r {fd}

[Stanislav Brabec]

Theodore Ts'o wrote:

> Actually, what of the one-line statistics do you need?  Is it just
> "we're done checking /dev/sdXX", so you can activate quotas for
> /dev/sdXX?

Yes, I think that it is only the device name.

> If so, one of the other patches which Frank Mayhar (one of
> my collegaues) implemented the ability for fsck to run a "completion
> handler" which was a program/shell script that would get executed for
> each file system the check was completed -- and which would pass the
> exit status for each specific file system along to the completion
> handler.  That allowed us to take specific action on a per-file system
> basis if a file system couldn't be repaired, without having to parse
> any of the log outputs to determine which file system had
> uncorrectable file system problems.
>
> I wonder if something like that would be useful for SuSE?
>

This sounds interesting.


>From the systemd perspective, I can imagine a completely different 
solution. systemd does not use a completion handlers, but it uses 
dependencies: When one task has fulfilled all its dependencies, then it 
could be run.

- fsck depends on presence of device
- mount depends on presence of the device, presence of mount point and
   fsck being complete.


There can be one fsck service per volume instead of one central fsck 
service.

But it is not a task for util-linux developers.


-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec@suse.cz
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76