| Message ID | 20191121214445.282160-3-preichl@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mkfs: inform during block discarding | expand |
On Thu, Nov 21, 2019 at 10:44:45PM +0100, Pavel Reichl wrote: > Signed-off-by: Pavel Reichl <preichl@redhat.com> > --- > mkfs/xfs_mkfs.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index a02d6f66..07b8bd78 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -1248,6 +1248,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) > const uint64_t step = (uint64_t)2<<30; > /* Sector size is 512 bytes */ > const uint64_t count = nsectors << 9; > + uint64_t prev_done = (uint64_t) ~0; > > fd = libxfs_device_to_fd(dev); > if (fd <= 0) > @@ -1255,6 +1256,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > while (offset < count) { > uint64_t tmp_step = step; > + uint64_t done = offset * 100 / count; > > if ((offset + step) > count) > tmp_step = count - offset; > @@ -1268,7 +1270,13 @@ discard_blocks(dev_t dev, uint64_t nsectors) > return; > > offset += tmp_step; > + > + if (prev_done != done) { Hmm... so this prints the status message every increase percentage point, right? > + prev_done = done; > + fprintf(stderr, _("Discarding: %2lu%% done\n"), done); This isn't an error, so why output to stderr? FWIW if it's a tty you might consider ending that string with \r so the status messages don't scroll off the screen. Or possibly only reporting status if stdout is a tty? --D > + } > } > + fprintf(stderr, _("Discarding is done.\n")); > } > > static __attribute__((noreturn)) void > -- > 2.23.0 >
On Thu, Nov 21, 2019 at 10:44:45PM +0100, Pavel Reichl wrote: > Signed-off-by: Pavel Reichl <preichl@redhat.com> > --- > mkfs/xfs_mkfs.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index a02d6f66..07b8bd78 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -1248,6 +1248,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) > const uint64_t step = (uint64_t)2<<30; > /* Sector size is 512 bytes */ > const uint64_t count = nsectors << 9; > + uint64_t prev_done = (uint64_t) ~0; > > fd = libxfs_device_to_fd(dev); > if (fd <= 0) > @@ -1255,6 +1256,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > while (offset < count) { > uint64_t tmp_step = step; > + uint64_t done = offset * 100 / count; That will overflow on a EB-scale (2^60 bytes) filesystems, won't it? > > if ((offset + step) > count) > tmp_step = count - offset; > @@ -1268,7 +1270,13 @@ discard_blocks(dev_t dev, uint64_t nsectors) > return; > > offset += tmp_step; > + > + if (prev_done != done) { > + prev_done = done; > + fprintf(stderr, _("Discarding: %2lu%% done\n"), done); > + } > } > + fprintf(stderr, _("Discarding is done.\n")); Hmmm - this output doesn't get suppressed when the "quiet" (-q) option is used. mkfs is supposed to be silent when this option is specified. I also suspect that it breaks a few fstests, too, as a some of them capture and filter mkfs output. They'll need filters to drop these new messages. FWIW, a 100 lines of extra mkfs output is going to cause workflow issues. I know it will cause me problems, because I often mkfs 500TB filesystems tens of times a day on a discard enabled device. This extra output will scroll all the context of the previous test run I'm about to compare against off my terminal screen and so now I will have to scroll the terminal to look at the results of back-to-back runs. IOWs, I'm going to immediately want to turn this output off and have it stay off permanently. Hence I think that, by default, just outputting a single "Discard in progress" line before starting the discard would be sufficient indication of what mkfs is currently doing. If someone wants more verbose progress output, then we should probably introduce a "verbose" CLI option to go along with the "quiet" option that suppresses all output. Cheers, Dave.
On Thu, Nov 21, 2019 at 10:59 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Thu, Nov 21, 2019 at 10:44:45PM +0100, Pavel Reichl wrote: > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > > --- > > mkfs/xfs_mkfs.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > index a02d6f66..07b8bd78 100644 > > --- a/mkfs/xfs_mkfs.c > > +++ b/mkfs/xfs_mkfs.c > > @@ -1248,6 +1248,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > const uint64_t step = (uint64_t)2<<30; > > /* Sector size is 512 bytes */ > > const uint64_t count = nsectors << 9; > > + uint64_t prev_done = (uint64_t) ~0; > > > > fd = libxfs_device_to_fd(dev); > > if (fd <= 0) > > @@ -1255,6 +1256,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > > > while (offset < count) { > > uint64_t tmp_step = step; > > + uint64_t done = offset * 100 / count; > > > > if ((offset + step) > count) > > tmp_step = count - offset; > > @@ -1268,7 +1270,13 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > return; > > > > offset += tmp_step; > > + > > + if (prev_done != done) { > > Hmm... so this prints the status message every increase percentage > point, right? Not at all, the 'least change' it prints is one percent but that's the maximum granularity i.e. I tested with 10 GB file and the output was: Discarding: 0% done Discarding: 20% done Discarding: 40% done Discarding: 60% done Discarding: 80% done Discarding is done. So ATM there could be up to 102 lines - please propose a different idea. > > > + prev_done = done; > > + fprintf(stderr, _("Discarding: %2lu%% done\n"), done); > > This isn't an error, so why output to stderr? My bad, sorry. > > FWIW if it's a tty you might consider ending that string with \r so the > status messages don't scroll off the screen. Or possibly only reporting > status if stdout is a tty? Do I get it right that you propose to not flow the terminal with dozens of lines which just update the percentage but instead keep updating the same line? If so, I do like that. > > --D > > > + } > > } > > + fprintf(stderr, _("Discarding is done.\n")); > > } > > > > static __attribute__((noreturn)) void > > -- > > 2.23.0 > > >
On Fri, Nov 22, 2019 at 05:27:42PM +0100, Pavel Reichl wrote: > On Thu, Nov 21, 2019 at 10:59 PM Darrick J. Wong > <darrick.wong@oracle.com> wrote: > > > > On Thu, Nov 21, 2019 at 10:44:45PM +0100, Pavel Reichl wrote: > > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > > > --- > > > mkfs/xfs_mkfs.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > > index a02d6f66..07b8bd78 100644 > > > --- a/mkfs/xfs_mkfs.c > > > +++ b/mkfs/xfs_mkfs.c > > > @@ -1248,6 +1248,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > > const uint64_t step = (uint64_t)2<<30; > > > /* Sector size is 512 bytes */ > > > const uint64_t count = nsectors << 9; > > > + uint64_t prev_done = (uint64_t) ~0; > > > > > > fd = libxfs_device_to_fd(dev); > > > if (fd <= 0) > > > @@ -1255,6 +1256,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > > > > > while (offset < count) { > > > uint64_t tmp_step = step; > > > + uint64_t done = offset * 100 / count; > > > > > > if ((offset + step) > count) > > > tmp_step = count - offset; > > > @@ -1268,7 +1270,13 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > > return; > > > > > > offset += tmp_step; > > > + > > > + if (prev_done != done) { > > > > Hmm... so this prints the status message every increase percentage > > point, right? > > Not at all, the 'least change' it prints is one percent but that's the > maximum granularity i.e. I tested with 10 GB file and the output was: > > Discarding: 0% done > Discarding: 20% done > Discarding: 40% done > Discarding: 60% done > Discarding: 80% done > Discarding is done. > > So ATM there could be up to 102 lines - please propose a different idea. if (device supports discard) { if (!quiet) printf(_("Discarding blocks, this may take some time...")); <discard loop> } <the rest of mkfs> > > > > > > + prev_done = done; > > > + fprintf(stderr, _("Discarding: %2lu%% done\n"), done); > > > > This isn't an error, so why output to stderr? > My bad, sorry. > > > > > FWIW if it's a tty you might consider ending that string with \r so the > > status messages don't scroll off the screen. Or possibly only reporting > > status if stdout is a tty? > > Do I get it right that you propose to not flow the terminal with > dozens of lines which just update the percentage but instead keep > updating the same line? If so, I do like that. Correct. --D > > > > --D > > > > > + } > > > } > > > + fprintf(stderr, _("Discarding is done.\n")); > > > } > > > > > > static __attribute__((noreturn)) void > > > -- > > > 2.23.0 > > > > > >
On Fri, Nov 22, 2019 at 12:42 AM Dave Chinner <david@fromorbit.com> wrote: > > On Thu, Nov 21, 2019 at 10:44:45PM +0100, Pavel Reichl wrote: > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > > --- > > mkfs/xfs_mkfs.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > index a02d6f66..07b8bd78 100644 > > --- a/mkfs/xfs_mkfs.c > > +++ b/mkfs/xfs_mkfs.c > > @@ -1248,6 +1248,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > const uint64_t step = (uint64_t)2<<30; > > /* Sector size is 512 bytes */ > > const uint64_t count = nsectors << 9; > > + uint64_t prev_done = (uint64_t) ~0; > > > > fd = libxfs_device_to_fd(dev); > > if (fd <= 0) > > @@ -1255,6 +1256,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > > > while (offset < count) { > > uint64_t tmp_step = step; > > + uint64_t done = offset * 100 / count; > > That will overflow on a EB-scale (2^60 bytes) filesystems, won't it? I guess that can happen, sorry. I'll try to come out with computation based on a floating point arithmetic. There should not be any performance or actual precision problem. (well actually I'll drop this line completely, no ratio will be computed in the end) > > > > > if ((offset + step) > count) > > tmp_step = count - offset; > > @@ -1268,7 +1270,13 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > return; > > > > offset += tmp_step; > > + > > + if (prev_done != done) { > > + prev_done = done; > > + fprintf(stderr, _("Discarding: %2lu%% done\n"), done); > > + } > > } > > + fprintf(stderr, _("Discarding is done.\n")); > > Hmmm - this output doesn't get suppressed when the "quiet" (-q) > option is used. mkfs is supposed to be silent when this option is > specified. OK, my bad. I'll fix that. > > I also suspect that it breaks a few fstests, too, as a some of them > capture and filter mkfs output. They'll need filters to drop these > new messages. > > FWIW, a 100 lines of extra mkfs output is going to cause workflow > issues. I know it will cause me problems, because I often mkfs 500TB > filesystems tens of times a day on a discard enabled device. This > extra output will scroll all the context of the previous test run > I'm about to compare against off my terminal screen and so now I > will have to scroll the terminal to look at the results of > back-to-back runs. IOWs, I'm going to immediately want to turn this > output off and have it stay off permanently. > > Hence I think that, by default, just outputting a single "Discard in > progress" line before starting the discard would be sufficient OK, maybe just one line "Discard in progress" is actually what users need. The computing of % done was probably just overkill from my side. Sorry about that. > indication of what mkfs is currently doing. If someone wants more > verbose progress output, then we should probably introduce a > "verbose" CLI option to go along with the "quiet" option that > suppresses all output. > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com >
On Fri, Nov 22, 2019 at 05:43:55PM +0100, Pavel Reichl wrote: > On Fri, Nov 22, 2019 at 12:42 AM Dave Chinner <david@fromorbit.com> wrote: > > > > On Thu, Nov 21, 2019 at 10:44:45PM +0100, Pavel Reichl wrote: > > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > > > --- > > > mkfs/xfs_mkfs.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > > index a02d6f66..07b8bd78 100644 > > > --- a/mkfs/xfs_mkfs.c > > > +++ b/mkfs/xfs_mkfs.c > > > @@ -1248,6 +1248,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > > const uint64_t step = (uint64_t)2<<30; > > > /* Sector size is 512 bytes */ > > > const uint64_t count = nsectors << 9; > > > + uint64_t prev_done = (uint64_t) ~0; > > > > > > fd = libxfs_device_to_fd(dev); > > > if (fd <= 0) > > > @@ -1255,6 +1256,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) > > > > > > while (offset < count) { > > > uint64_t tmp_step = step; > > > + uint64_t done = offset * 100 / count; > > > > That will overflow on a EB-scale (2^60 bytes) filesystems, won't it? > > I guess that can happen, sorry. I'll try to come out with computation > based on a floating point arithmetic. There should not be any > performance or actual precision problem. > (well actually I'll drop this line completely, no ratio will be > computed in the end) No need to apologise for not realising huge filesystems need to work. It takes time to get used to having to consider 64 bit overflows everywhere... :) Maybe the easiest way to do this sort of thing is to calculate reporting interval prior to the loop, and every time it is exceeded issue a report and then reset the report counter to zero. No fancy math required there. If we want 1% increments: report_interval = count / 100; while (offset < count) { .... offset += tmp_step; report_offset += tmp_step; if (report_offset > report_interval) { report_offset = 0; /* issue report */ } } And this is easy to adjust the number of reports issued (e.g. every 5% or 10% is just changing the report_interval division constant. Another way of doing it is deciding on the -time- between reports. e.g. issue a progress report every 60s. Then you can just report the percentage done based on offset and count without needing intermediate accounting. > > I also suspect that it breaks a few fstests, too, as a some of them > > capture and filter mkfs output. They'll need filters to drop these > > new messages. > > > > FWIW, a 100 lines of extra mkfs output is going to cause workflow > > issues. I know it will cause me problems, because I often mkfs 500TB > > filesystems tens of times a day on a discard enabled device. This > > extra output will scroll all the context of the previous test run > > I'm about to compare against off my terminal screen and so now I > > will have to scroll the terminal to look at the results of > > back-to-back runs. IOWs, I'm going to immediately want to turn this > > output off and have it stay off permanently. > > > > Hence I think that, by default, just outputting a single "Discard in > > progress" line before starting the discard would be sufficient > > OK, maybe just one line "Discard in progress" is actually what users > need. The computing of % done was probably just overkill from my side. > Sorry about that. Again, no need to apologise because there are different opinions on how something should be done. If you didn't put progress reporting in, I'm sure someone would have suggested it and we'd be having the same discussion anyway. :) Cheers, Dave.
On 11/22/19 10:43 AM, Pavel Reichl wrote: > On Fri, Nov 22, 2019 at 12:42 AM Dave Chinner <david@fromorbit.com> wrote: >> >> On Thu, Nov 21, 2019 at 10:44:45PM +0100, Pavel Reichl wrote: ... >> >> I also suspect that it breaks a few fstests, too, as a some of them >> capture and filter mkfs output. They'll need filters to drop these >> new messages. >> >> FWIW, a 100 lines of extra mkfs output is going to cause workflow >> issues. I know it will cause me problems, because I often mkfs 500TB >> filesystems tens of times a day on a discard enabled device. This >> extra output will scroll all the context of the previous test run >> I'm about to compare against off my terminal screen and so now I >> will have to scroll the terminal to look at the results of >> back-to-back runs. IOWs, I'm going to immediately want to turn this >> output off and have it stay off permanently. >> >> Hence I think that, by default, just outputting a single "Discard in >> progress" line before starting the discard would be sufficient e2fsprogs simply does: Discarding device blocks: done ("done" isn't printed until it's ... done) so that might be a good convention to follow? Though I'd probably do printf("Discarding blocks... "); .... printf("Done.\n"); because the ellipses tend to indicate waiting. :) Even the one line might require filtering-out in xfstests, but luckily we have standard filters and it should be trivial to add. > OK, maybe just one line "Discard in progress" is actually what users > need. The computing of % done was probably just overkill from my side. > Sorry about that. No worries, that's why we discuss stuff. :) Thanks for taking this on, -Eric
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index a02d6f66..07b8bd78 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -1248,6 +1248,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) const uint64_t step = (uint64_t)2<<30; /* Sector size is 512 bytes */ const uint64_t count = nsectors << 9; + uint64_t prev_done = (uint64_t) ~0; fd = libxfs_device_to_fd(dev); if (fd <= 0) @@ -1255,6 +1256,7 @@ discard_blocks(dev_t dev, uint64_t nsectors) while (offset < count) { uint64_t tmp_step = step; + uint64_t done = offset * 100 / count; if ((offset + step) > count) tmp_step = count - offset; @@ -1268,7 +1270,13 @@ discard_blocks(dev_t dev, uint64_t nsectors) return; offset += tmp_step; + + if (prev_done != done) { + prev_done = done; + fprintf(stderr, _("Discarding: %2lu%% done\n"), done); + } } + fprintf(stderr, _("Discarding is done.\n")); } static __attribute__((noreturn)) void
Signed-off-by: Pavel Reichl <preichl@redhat.com> --- mkfs/xfs_mkfs.c | 8 ++++++++ 1 file changed, 8 insertions(+)