Linux-XFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] mkfs: Break block discard into chunks of 2 GB
@ 2019-11-28  6:21 Pavel Reichl
  2019-11-30 22:01 ` Chaitanya Kulkarni
  0 siblings, 1 reply; 7+ messages in thread
From: Pavel Reichl @ 2019-11-28  6:21 UTC (permalink / raw)
  To: linux-xfs; +Cc: Pavel Reichl

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mkfs: Break block discard into chunks of 2 GB
  2019-11-28  6:21 [PATCH v3] mkfs: Break block discard into chunks of 2 GB Pavel Reichl
@ 2019-11-30 22:01 ` Chaitanya Kulkarni
  2019-12-02 16:40   ` Darrick J. Wong
  2019-12-04 16:24   ` Eric Sandeen
  0 siblings, 2 replies; 7+ messages in thread
From: Chaitanya Kulkarni @ 2019-11-30 22:01 UTC (permalink / raw)
  To: Pavel Reichl, linux-xfs

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>
> ---


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mkfs: Break block discard into chunks of 2 GB
  2019-11-30 22:01 ` Chaitanya Kulkarni
@ 2019-12-02 16:40   ` Darrick J. Wong
  2019-12-04 16:24   ` Eric Sandeen
  1 sibling, 0 replies; 7+ messages in thread
From: Darrick J. Wong @ 2019-12-02 16:40 UTC (permalink / raw)
  To: Chaitanya Kulkarni; +Cc: Pavel Reichl, linux-xfs

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>
> > ---
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mkfs: Break block discard into chunks of 2 GB
  2019-11-30 22:01 ` Chaitanya Kulkarni
  2019-12-02 16:40   ` Darrick J. Wong
@ 2019-12-04 16:24   ` Eric Sandeen
  2019-12-04 17:26     ` Christoph Hellwig
  1 sibling, 1 reply; 7+ messages in thread
From: Eric Sandeen @ 2019-12-04 16:24 UTC (permalink / raw)
  To: Chaitanya Kulkarni, Pavel Reichl, linux-xfs

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mkfs: Break block discard into chunks of 2 GB
  2019-12-04 16:24   ` Eric Sandeen
@ 2019-12-04 17:26     ` Christoph Hellwig
  2019-12-04 17:32       ` Eric Sandeen
  2019-12-04 17:42       ` Darrick J. Wong
  0 siblings, 2 replies; 7+ messages in thread
From: Christoph Hellwig @ 2019-12-04 17:26 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Chaitanya Kulkarni, Pavel Reichl, linux-xfs

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mkfs: Break block discard into chunks of 2 GB
  2019-12-04 17:26     ` Christoph Hellwig
@ 2019-12-04 17:32       ` Eric Sandeen
  2019-12-04 17:42       ` Darrick J. Wong
  1 sibling, 0 replies; 7+ messages in thread
From: Eric Sandeen @ 2019-12-04 17:32 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Chaitanya Kulkarni, Pavel Reichl, linux-xfs

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mkfs: Break block discard into chunks of 2 GB
  2019-12-04 17:26     ` Christoph Hellwig
  2019-12-04 17:32       ` Eric Sandeen
@ 2019-12-04 17:42       ` Darrick J. Wong
  1 sibling, 0 replies; 7+ messages in thread
From: Darrick J. Wong @ 2019-12-04 17:42 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Eric Sandeen, Chaitanya Kulkarni, Pavel Reichl, linux-xfs

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-28  6:21 [PATCH v3] mkfs: Break block discard into chunks of 2 GB Pavel Reichl
2019-11-30 22:01 ` Chaitanya Kulkarni
2019-12-02 16:40   ` Darrick J. Wong
2019-12-04 16:24   ` Eric Sandeen
2019-12-04 17:26     ` Christoph Hellwig
2019-12-04 17:32       ` Eric Sandeen
2019-12-04 17:42       ` Darrick J. Wong

Linux-XFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-xfs/0 linux-xfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-xfs linux-xfs/ https://lore.kernel.org/linux-xfs \
		linux-xfs@vger.kernel.org
	public-inbox-index linux-xfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-xfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git