Some users are not happy about the BLKDISCARD taking too long and at the same time not being informed about that - so they think that the command actually hung. This commit changes code so that progress reporting is possible and also typing the ^C will cancel the ongoing BLKDISCARD. Signed-off-by: Pavel Reichl <preichl@redhat.com> --- mkfs/xfs_mkfs.c | 50 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 18338a61..0defadb2 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -1240,17 +1240,40 @@ done: } static void -discard_blocks(dev_t dev, uint64_t nsectors) +discard_blocks(dev_t dev, uint64_t nsectors, int quiet) { - int fd; + int fd; + uint64_t offset = 0; + /* Discard the device 2G at a time */ + const uint64_t step = 2ULL << 30; + const uint64_t count = BBTOB(nsectors); - /* - * We intentionally ignore errors from the discard ioctl. It is - * not necessary for the mkfs functionality but just an optimization. - */ fd = libxfs_device_to_fd(dev); - if (fd > 0) - platform_discard_blocks(fd, 0, nsectors << 9); + if (fd <= 0) + return; + if (!quiet) { + printf("Discarding blocks...\n"); + printf("...\n"); + } + + /* The block discarding happens in smaller batches so it can be + * interrupted prematurely + */ + while (offset < count) { + uint64_t tmp_step = min(step, count - offset); + + /* + * We intentionally ignore errors from the discard ioctl. It is + * not necessary for the mkfs functionality but just an + * optimization. However we should stop on error. + */ + if (platform_discard_blocks(fd, offset, tmp_step)) + return; + + offset += tmp_step; + } + if (!quiet) + printf("Done.\n"); } static __attribute__((noreturn)) void @@ -2507,18 +2530,19 @@ open_devices( static void discard_devices( - struct libxfs_xinit *xi) + struct libxfs_xinit *xi, + int quiet) { /* * This function has to be called after libxfs has been initialized. */ if (!xi->disfile) - discard_blocks(xi->ddev, xi->dsize); + discard_blocks(xi->ddev, xi->dsize, quiet); if (xi->rtdev && !xi->risfile) - discard_blocks(xi->rtdev, xi->rtsize); + discard_blocks(xi->rtdev, xi->rtsize, quiet); if (xi->logdev && xi->logdev != xi->ddev && !xi->lisfile) - discard_blocks(xi->logdev, xi->logBBsize); + discard_blocks(xi->logdev, xi->logBBsize, quiet); } static void @@ -3749,7 +3773,7 @@ main( * All values have been validated, discard the old device layout. */ if (discard && !dry_run) - discard_devices(&xi); + discard_devices(&xi, quiet); /* * we need the libxfs buffer cache from here on in. -- 2.23.0
Not an XFS expert, but patch to handle ^C is been discussed on the block layer mailing list which includes discard operations. [1] This solution seems specific to one file system, which will lead to code repetition for all the file systems which are in question. How about we come up with the generic solution in the block-layer so it can be reused for all the file systems ? (fyi, I'm not aware of any drawbacks of handling ^C it in the block layer and would like to learn if any). [1] https://patchwork.kernel.org/patch/11234607/ -Chaitanya On 11/27/2019 10:21 PM, Pavel Reichl wrote: > Some users are not happy about the BLKDISCARD taking too long and at the same > time not being informed about that - so they think that the command actually > hung. > > This commit changes code so that progress reporting is possible and also typing > the ^C will cancel the ongoing BLKDISCARD. > > Signed-off-by: Pavel Reichl<preichl@redhat.com> > ---
On Sat, Nov 30, 2019 at 10:01:17PM +0000, Chaitanya Kulkarni wrote: > Not an XFS expert, but patch to handle ^C is been discussed on the > block layer mailing list which includes discard operations. [1] Heh, I wasn't aware of that. :) > This solution seems specific to one file system, which will lead to > code repetition for all the file systems which are in question. > > How about we come up with the generic solution in the block-layer so > it can be reused for all the file systems ? > > (fyi, I'm not aware of any drawbacks of handling ^C it in the block > layer and would like to learn if any). The only one that I can think of is how to signal a partial completion, but if you're only aborting on *fatal* signals then that doesn't matter. Fixing the block layer seems like a better answer anyway. > [1] https://patchwork.kernel.org/patch/11234607/ Though looking through that patch raises the question of whether xfs' control loops also need to check for fatal signals, similar to what the online scrub loops do? --D > -Chaitanya > > On 11/27/2019 10:21 PM, Pavel Reichl wrote: > > Some users are not happy about the BLKDISCARD taking too long and at the same > > time not being informed about that - so they think that the command actually > > hung. > > > > This commit changes code so that progress reporting is possible and also typing > > the ^C will cancel the ongoing BLKDISCARD. > > > > Signed-off-by: Pavel Reichl<preichl@redhat.com> > > --- >
On 11/30/19 4:01 PM, Chaitanya Kulkarni wrote:
> Not an XFS expert, but patch to handle ^C is been discussed on the
> block layer mailing list which includes discard operations. [1]
>
> This solution seems specific to one file system, which will lead to
> code repetition for all the file systems which are in question.
>
> How about we come up with the generic solution in the block-layer so
> it can be reused for all the file systems ?
>
> (fyi, I'm not aware of any drawbacks of handling ^C it in the block
> layer and would like to learn if any).
>
> [1] https://patchwork.kernel.org/patch/11234607/
>
> -Chaitanya
It'd be great to fix this universally in the kernel but it seems like
that patch is in discussion for now, and TBH I don't see any real
drawbacks to looping in mkfs - it would also solve the problem on any
old kernel w/o the block layer change.
I'd propose that we go ahead w/ the mkfs change, and if/when the kernel
handles this better, and it's reasonable to expect that we're running
on a kernel where it can be interrupted, we could remove the mkfs loop
at a later date if we wanted to.
-Eric
On Wed, Dec 04, 2019 at 10:24:32AM -0600, Eric Sandeen wrote: > It'd be great to fix this universally in the kernel but it seems like > that patch is in discussion for now, and TBH I don't see any real > drawbacks to looping in mkfs - it would also solve the problem on any > old kernel w/o the block layer change. The problem is that we throw out efficiency for no good reason. > I'd propose that we go ahead w/ the mkfs change, and if/when the kernel > handles this better, and it's reasonable to expect that we're running > on a kernel where it can be interrupted, we could remove the mkfs loop > at a later date if we wanted to. I'd rather not touch mkfs if a trivial kernel patch handles the issue.
On 12/4/19 11:26 AM, Christoph Hellwig wrote: > On Wed, Dec 04, 2019 at 10:24:32AM -0600, Eric Sandeen wrote: >> It'd be great to fix this universally in the kernel but it seems like >> that patch is in discussion for now, and TBH I don't see any real >> drawbacks to looping in mkfs - it would also solve the problem on any >> old kernel w/o the block layer change. > > The problem is that we throw out efficiency for no good reason. The reason, as I stated earlier, is that up to this day, no kernel properly handles this, and people are hitting this problem today. And nobody has shown a significant efficiency loss. (To be fair, nobody has shown that there isn't a loss either, but in my admittedly small test set I didn't see any meaningful overhead from the loop.) >> I'd propose that we go ahead w/ the mkfs change, and if/when the kernel >> handles this better, and it's reasonable to expect that we're running >> on a kernel where it can be interrupted, we could remove the mkfs loop >> at a later date if we wanted to. > > I'd rather not touch mkfs if a trivial kernel patch handles the issue. I don't think a trivial kernel patch to handle the issue even exists yet... -Eric
On Wed, Dec 04, 2019 at 09:26:52AM -0800, Christoph Hellwig wrote: > On Wed, Dec 04, 2019 at 10:24:32AM -0600, Eric Sandeen wrote: > > It'd be great to fix this universally in the kernel but it seems like > > that patch is in discussion for now, and TBH I don't see any real > > drawbacks to looping in mkfs - it would also solve the problem on any > > old kernel w/o the block layer change. > > The problem is that we throw out efficiency for no good reason. True... > > I'd propose that we go ahead w/ the mkfs change, and if/when the kernel > > handles this better, and it's reasonable to expect that we're running How do we detect that the kernel will handle it better? > > on a kernel where it can be interrupted, we could remove the mkfs loop > > at a later date if we wanted to. > > I'd rather not touch mkfs if a trivial kernel patch handles the issue. Did some version of Tetsuo's patch even make it for 5.5? It seemed to call submit_bio_wait from within a blk_plug region, which seems way worse. --D
On 11/28/19 12:21 AM, Pavel Reichl wrote: > Some users are not happy about the BLKDISCARD taking too long and at the same > time not being informed about that - so they think that the command actually > hung. > > This commit changes code so that progress reporting is possible and also typing > the ^C will cancel the ongoing BLKDISCARD. Ok I'm going to nitpick this just a little bit more... > Signed-off-by: Pavel Reichl <preichl@redhat.com> > --- > mkfs/xfs_mkfs.c | 50 ++++++++++++++++++++++++++++++++++++------------- > 1 file changed, 37 insertions(+), 13 deletions(-) > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 18338a61..0defadb2 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -1240,17 +1240,40 @@ done: > } > > static void > -discard_blocks(dev_t dev, uint64_t nsectors) > +discard_blocks(dev_t dev, uint64_t nsectors, int quiet) > { > - int fd; > + int fd; > + uint64_t offset = 0; > + /* Discard the device 2G at a time */ > + const uint64_t step = 2ULL << 30; > + const uint64_t count = BBTOB(nsectors); > > - /* > - * We intentionally ignore errors from the discard ioctl. It is > - * not necessary for the mkfs functionality but just an optimization. > - */ > fd = libxfs_device_to_fd(dev); > - if (fd > 0) > - platform_discard_blocks(fd, 0, nsectors << 9); > + if (fd <= 0) > + return; > + if (!quiet) { > + printf("Discarding blocks...\n"); > + printf("...\n"); > + } Let's change this output so that it prints only a single line, i.e. + if (!quiet) { + printf("Discarding blocks... "); ... + if (!quiet) + printf("Done.\n"); Then in xfstests there's only one line to filter. I think there was a suggestion to test isatty() to see if we're on a terminal, but I have some concern that a hung script will lead to confusion as well if there's no status message. We can add terminal testing in a later patch if that really seems like the right way to go. If you're ok with the above, I can make the change on commit to save another patch round trip, and Reviewed-by: Eric Sandeen <sandeen@redhat.com> > + > + /* The block discarding happens in smaller batches so it can be > + * interrupted prematurely > + */ > + while (offset < count) { > + uint64_t tmp_step = min(step, count - offset); > + > + /* > + * We intentionally ignore errors from the discard ioctl. It is > + * not necessary for the mkfs functionality but just an > + * optimization. However we should stop on error. > + */ > + if (platform_discard_blocks(fd, offset, tmp_step)) > + return; > + > + offset += tmp_step; > + } > + if (!quiet) > + printf("Done.\n"); > } > > static __attribute__((noreturn)) void > @@ -2507,18 +2530,19 @@ open_devices( > > static void > discard_devices( > - struct libxfs_xinit *xi) > + struct libxfs_xinit *xi, > + int quiet) > { > /* > * This function has to be called after libxfs has been initialized. > */ > > if (!xi->disfile) > - discard_blocks(xi->ddev, xi->dsize); > + discard_blocks(xi->ddev, xi->dsize, quiet); > if (xi->rtdev && !xi->risfile) > - discard_blocks(xi->rtdev, xi->rtsize); > + discard_blocks(xi->rtdev, xi->rtsize, quiet); > if (xi->logdev && xi->logdev != xi->ddev && !xi->lisfile) > - discard_blocks(xi->logdev, xi->logBBsize); > + discard_blocks(xi->logdev, xi->logBBsize, quiet); > } > > static void > @@ -3749,7 +3773,7 @@ main( > * All values have been validated, discard the old device layout. > */ > if (discard && !dry_run) > - discard_devices(&xi); > + discard_devices(&xi, quiet); > > /* > * we need the libxfs buffer cache from here on in. >
On Mon, Dec 09, 2019 at 04:00:17PM -0600, Eric Sandeen wrote: > On 11/28/19 12:21 AM, Pavel Reichl wrote: > > Some users are not happy about the BLKDISCARD taking too long and at the same > > time not being informed about that - so they think that the command actually > > hung. > > > > This commit changes code so that progress reporting is possible and also typing > > the ^C will cancel the ongoing BLKDISCARD. > > Ok I'm going to nitpick this just a little bit more... > > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > > --- > > mkfs/xfs_mkfs.c | 50 ++++++++++++++++++++++++++++++++++++------------- > > 1 file changed, 37 insertions(+), 13 deletions(-) > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > index 18338a61..0defadb2 100644 > > --- a/mkfs/xfs_mkfs.c > > +++ b/mkfs/xfs_mkfs.c > > @@ -1240,17 +1240,40 @@ done: > > } > > > > static void > > -discard_blocks(dev_t dev, uint64_t nsectors) > > +discard_blocks(dev_t dev, uint64_t nsectors, int quiet) > > { > > - int fd; > > + int fd; > > + uint64_t offset = 0; > > + /* Discard the device 2G at a time */ > > + const uint64_t step = 2ULL << 30; > > + const uint64_t count = BBTOB(nsectors); > > > > - /* > > - * We intentionally ignore errors from the discard ioctl. It is > > - * not necessary for the mkfs functionality but just an optimization. > > - */ > > fd = libxfs_device_to_fd(dev); > > - if (fd > 0) > > - platform_discard_blocks(fd, 0, nsectors << 9); > > + if (fd <= 0) > > + return; > > + if (!quiet) { > > + printf("Discarding blocks...\n"); > > + printf("...\n"); > > + } > > Let's change this output so that it prints only a single line, i.e. > > + if (!quiet) { > + printf("Discarding blocks... "); This needs fflush(stdout); here to force the message out of stdio output buffering. --D > > ... > > + if (!quiet) > + printf("Done.\n"); > > Then in xfstests there's only one line to filter. > > I think there was a suggestion to test isatty() to see if we're on a terminal, > but I have some concern that a hung script will lead to confusion as well if > there's no status message. We can add terminal testing in a later patch if that > really seems like the right way to go. > > If you're ok with the above, I can make the change on commit to save another > patch round trip, and > > Reviewed-by: Eric Sandeen <sandeen@redhat.com> > > > + > > + /* The block discarding happens in smaller batches so it can be > > + * interrupted prematurely > > + */ > > + while (offset < count) { > > + uint64_t tmp_step = min(step, count - offset); > > + > > + /* > > + * We intentionally ignore errors from the discard ioctl. It is > > + * not necessary for the mkfs functionality but just an > > + * optimization. However we should stop on error. > > + */ > > + if (platform_discard_blocks(fd, offset, tmp_step)) > > + return; > > + > > + offset += tmp_step; > > + } > > + if (!quiet) > > + printf("Done.\n"); > > } > > > > static __attribute__((noreturn)) void > > @@ -2507,18 +2530,19 @@ open_devices( > > > > static void > > discard_devices( > > - struct libxfs_xinit *xi) > > + struct libxfs_xinit *xi, > > + int quiet) > > { > > /* > > * This function has to be called after libxfs has been initialized. > > */ > > > > if (!xi->disfile) > > - discard_blocks(xi->ddev, xi->dsize); > > + discard_blocks(xi->ddev, xi->dsize, quiet); > > if (xi->rtdev && !xi->risfile) > > - discard_blocks(xi->rtdev, xi->rtsize); > > + discard_blocks(xi->rtdev, xi->rtsize, quiet); > > if (xi->logdev && xi->logdev != xi->ddev && !xi->lisfile) > > - discard_blocks(xi->logdev, xi->logBBsize); > > + discard_blocks(xi->logdev, xi->logBBsize, quiet); > > } > > > > static void > > @@ -3749,7 +3773,7 @@ main( > > * All values have been validated, discard the old device layout. > > */ > > if (discard && !dry_run) > > - discard_devices(&xi); > > + discard_devices(&xi, quiet); > > > > /* > > * we need the libxfs buffer cache from here on in. > >
On 12/9/19 5:34 PM, Darrick J. Wong wrote:
> On Mon, Dec 09, 2019 at 04:00:17PM -0600, Eric Sandeen wrote:
>> On 11/28/19 12:21 AM, Pavel Reichl wrote:
>>> Some users are not happy about the BLKDISCARD taking too long and at the same
>>> time not being informed about that - so they think that the command actually
>>> hung.
>>>
>>> This commit changes code so that progress reporting is possible and also typing
>>> the ^C will cancel the ongoing BLKDISCARD.
>> Ok I'm going to nitpick this just a little bit more...
>>
>>> Signed-off-by: Pavel Reichl <preichl@redhat.com>
>>> ---
>>> mkfs/xfs_mkfs.c | 50 ++++++++++++++++++++++++++++++++++++-------------
>>> 1 file changed, 37 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>>> index 18338a61..0defadb2 100644
>>> --- a/mkfs/xfs_mkfs.c
>>> +++ b/mkfs/xfs_mkfs.c
>>> @@ -1240,17 +1240,40 @@ done:
>>> }
>>>
>>> static void
>>> -discard_blocks(dev_t dev, uint64_t nsectors)
>>> +discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
>>> {
>>> - int fd;
>>> + int fd;
>>> + uint64_t offset = 0;
>>> + /* Discard the device 2G at a time */
>>> + const uint64_t step = 2ULL << 30;
>>> + const uint64_t count = BBTOB(nsectors);
>>>
>>> - /*
>>> - * We intentionally ignore errors from the discard ioctl. It is
>>> - * not necessary for the mkfs functionality but just an optimization.
>>> - */
>>> fd = libxfs_device_to_fd(dev);
>>> - if (fd > 0)
>>> - platform_discard_blocks(fd, 0, nsectors << 9);
>>> + if (fd <= 0)
>>> + return;
>>> + if (!quiet) {
>>> + printf("Discarding blocks...\n");
>>> + printf("...\n");
>>> + }
>> Let's change this output so that it prints only a single line, i.e.
>>
>> + if (!quiet) {
>> + printf("Discarding blocks... ");
> This needs fflush(stdout); here to force the message out of stdio output
> buffering.
>
> --D
I thought of that, then forgot. ;) Maybe we do need a formal v4 on the list.
On 12/04/2019 09:42 AM, Darrick J. Wong wrote: > On Wed, Dec 04, 2019 at 09:26:52AM -0800, Christoph Hellwig wrote: >> On Wed, Dec 04, 2019 at 10:24:32AM -0600, Eric Sandeen wrote: >>> It'd be great to fix this universally in the kernel but it seems like >>> that patch is in discussion for now, and TBH I don't see any real >>> drawbacks to looping in mkfs - it would also solve the problem on any >>> old kernel w/o the block layer change. >> >> The problem is that we throw out efficiency for no good reason. > > True... > >>> I'd propose that we go ahead w/ the mkfs change, and if/when the kernel >>> handles this better, and it's reasonable to expect that we're running > > How do we detect that the kernel will handle it better? > >>> on a kernel where it can be interrupted, we could remove the mkfs loop >>> at a later date if we wanted to. >> >> I'd rather not touch mkfs if a trivial kernel patch handles the issue. > > Did some version of Tetsuo's patch even make it for 5.5? It seemed to > call submit_bio_wait from within a blk_plug region, which seems way > worse. > It did not yet, I can ping on the series with reference to this discussion. > --D >
On 12/10/19 1:33 AM, Chaitanya Kulkarni wrote:
> On 12/04/2019 09:42 AM, Darrick J. Wong wrote:
>> On Wed, Dec 04, 2019 at 09:26:52AM -0800, Christoph Hellwig wrote:
>>> On Wed, Dec 04, 2019 at 10:24:32AM -0600, Eric Sandeen wrote:
>>>> It'd be great to fix this universally in the kernel but it seems like
>>>> that patch is in discussion for now, and TBH I don't see any real
>>>> drawbacks to looping in mkfs - it would also solve the problem on any
>>>> old kernel w/o the block layer change.
>>>
>>> The problem is that we throw out efficiency for no good reason.
>>
>> True...
>>
>>>> I'd propose that we go ahead w/ the mkfs change, and if/when the kernel
>>>> handles this better, and it's reasonable to expect that we're running
>>
>> How do we detect that the kernel will handle it better?
>
>>
>>>> on a kernel where it can be interrupted, we could remove the mkfs loop
>>>> at a later date if we wanted to.
>>>
>>> I'd rather not touch mkfs if a trivial kernel patch handles the issue.
>>
>> Did some version of Tetsuo's patch even make it for 5.5? It seemed to
>> call submit_bio_wait from within a blk_plug region, which seems way
>> worse.
>>
>
> It did not yet, I can ping on the series with reference to this discussion.
That's fine, though I'm going to merge this patch in any case; we need a solution
for kernels that are not bleeding-edge new as well.
-eric