Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] btrfs-progs: balance: Sync the fs before balancing metadata chunks
@ 2019-01-29  6:57 Qu Wenruo
  2019-02-25 17:21 ` David Sterba
  0 siblings, 1 reply; 4+ messages in thread
From: Qu Wenruo @ 2019-01-29  6:57 UTC (permalink / raw)
  To: linux-btrfs

[BUG]
Btrfs will report false ENOSPC balancing metadata chunk.
The following script can easily reproduce it:

  #!/bin/bash
  dev=/dev/test/test
  mnt=/mnt/btrfs

  umount $dev &> /dev/null
  umount $mnt &> /dev/null
  mkfs.btrfs -f $dev

  mount $dev $mnt
  btrfs subv create $mnt/subv
  for ((i = 0; i < 1024; i++)) do
  	xfs_io -f -c "pwrite 0 4k" $mnt/subv/file_$i > /dev/null
  done
  btrfs balance start -m $mnt

[CAUSE]
It's metadata space_info::bytes_may_use causing the problem.
For above case, we need to reserve enough metadata space for all the
created small files.

[FIX]
The most straightforward is to sync the fs before balancing metadata
chunks.

We could enhance the kernel bytes_may_use calculation, but I doubt about
the complexity.
So I take the easy fix to reduce the false ENOSPC reports.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds-balance.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/cmds-balance.c b/cmds-balance.c
index 15dc385e..a617a1d2 100644
--- a/cmds-balance.c
+++ b/cmds-balance.c
@@ -24,6 +24,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <errno.h>
+#include <btrfsutil.h>
 
 #include "kerncompat.h"
 #include "ctree.h"
@@ -32,6 +33,7 @@
 
 #include "commands.h"
 #include "utils.h"
+#include "utils.h"
 #include "help.h"
 
 static const char * const balance_cmd_group_usage[] = {
@@ -455,6 +457,22 @@ static int do_balance(const char *path, struct btrfs_ioctl_balance_args *args,
 		printf("\nStarting balance without any filters.\n");
 	}
 
+	/*
+	 * There may be many over-reserved space for metadata block groups,
+	 * especially for inlined file extents.
+	 *
+	 * Do a sync here will free those over-reserved space and hugely
+	 * reduce the possibility of some false ENOSPC
+	 */
+	if (args->flags & BTRFS_BALANCE_METADATA) {
+		ret = btrfs_util_sync(path);
+		if (ret) {
+			error("failed to sync the fs before balance: %m");
+			ret = -errno;
+			goto out;
+		}
+	}
+
 	ret = ioctl(fd, BTRFS_IOC_BALANCE_V2, args);
 	if (ret < 0) {
 		/*
-- 
2.18.0


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

* Re: [PATCH] btrfs-progs: balance: Sync the fs before balancing metadata chunks
  2019-01-29  6:57 [PATCH] btrfs-progs: balance: Sync the fs before balancing metadata chunks Qu Wenruo
@ 2019-02-25 17:21 ` David Sterba
  2019-02-26  5:55   ` Qu Wenruo
  0 siblings, 1 reply; 4+ messages in thread
From: David Sterba @ 2019-02-25 17:21 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Tue, Jan 29, 2019 at 02:57:39PM +0800, Qu Wenruo wrote:
> The most straightforward is to sync the fs before balancing metadata
> chunks.
> 
> We could enhance the kernel bytes_may_use calculation, but I doubt about
> the complexity.
> So I take the easy fix to reduce the false ENOSPC reports.

Agreed.

> +	/*
> +	 * There may be many over-reserved space for metadata block groups,
> +	 * especially for inlined file extents.
> +	 *
> +	 * Do a sync here will free those over-reserved space and hugely
> +	 * reduce the possibility of some false ENOSPC
> +	 */
> +	if (args->flags & BTRFS_BALANCE_METADATA) {
> +		ret = btrfs_util_sync(path);

As the fd is already open, we should use the _fd version,

> +		if (ret) {
> +			error("failed to sync the fs before balance: %m");
> +			ret = -errno;
> +			goto out;

and possibly only warn if there's an error returned as the sync failure
is not a critical condition.

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

* Re: [PATCH] btrfs-progs: balance: Sync the fs before balancing metadata chunks
  2019-02-25 17:21 ` David Sterba
@ 2019-02-26  5:55   ` Qu Wenruo
  2019-02-27 16:25     ` David Sterba
  0 siblings, 1 reply; 4+ messages in thread
From: Qu Wenruo @ 2019-02-26  5:55 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, linux-btrfs

[-- Attachment #1.1: Type: text/plain, Size: 1321 bytes --]



On 2019/2/26 上午1:21, David Sterba wrote:
> On Tue, Jan 29, 2019 at 02:57:39PM +0800, Qu Wenruo wrote:
>> The most straightforward is to sync the fs before balancing metadata
>> chunks.
>>
>> We could enhance the kernel bytes_may_use calculation, but I doubt about
>> the complexity.
>> So I take the easy fix to reduce the false ENOSPC reports.
> 
> Agreed.
> 
>> +	/*
>> +	 * There may be many over-reserved space for metadata block groups,
>> +	 * especially for inlined file extents.
>> +	 *
>> +	 * Do a sync here will free those over-reserved space and hugely
>> +	 * reduce the possibility of some false ENOSPC
>> +	 */
>> +	if (args->flags & BTRFS_BALANCE_METADATA) {
>> +		ret = btrfs_util_sync(path);
> 
> As the fd is already open, we should use the _fd version,
> 
>> +		if (ret) {
>> +			error("failed to sync the fs before balance: %m");
>> +			ret = -errno;
>> +			goto out;
> 
> and possibly only warn if there's an error returned as the sync failure
> is not a critical condition.

AFAIK if we can't even sync the fs, the balance is definitely going to
fail, as the most common failure mode for syncfs is RO fs, caused by
aborted transaction.

Thus I still think we should error out. Or is there some other
non-critical failure mode I missed?

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH] btrfs-progs: balance: Sync the fs before balancing metadata chunks
  2019-02-26  5:55   ` Qu Wenruo
@ 2019-02-27 16:25     ` David Sterba
  0 siblings, 0 replies; 4+ messages in thread
From: David Sterba @ 2019-02-27 16:25 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: dsterba, Qu Wenruo, linux-btrfs

On Tue, Feb 26, 2019 at 01:55:06PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/2/26 上午1:21, David Sterba wrote:
> > On Tue, Jan 29, 2019 at 02:57:39PM +0800, Qu Wenruo wrote:
> >> The most straightforward is to sync the fs before balancing metadata
> >> chunks.
> >>
> >> We could enhance the kernel bytes_may_use calculation, but I doubt about
> >> the complexity.
> >> So I take the easy fix to reduce the false ENOSPC reports.
> > 
> > Agreed.
> > 
> >> +	/*
> >> +	 * There may be many over-reserved space for metadata block groups,
> >> +	 * especially for inlined file extents.
> >> +	 *
> >> +	 * Do a sync here will free those over-reserved space and hugely
> >> +	 * reduce the possibility of some false ENOSPC
> >> +	 */
> >> +	if (args->flags & BTRFS_BALANCE_METADATA) {
> >> +		ret = btrfs_util_sync(path);
> > 
> > As the fd is already open, we should use the _fd version,
> > 
> >> +		if (ret) {
> >> +			error("failed to sync the fs before balance: %m");
> >> +			ret = -errno;
> >> +			goto out;
> > 
> > and possibly only warn if there's an error returned as the sync failure
> > is not a critical condition.
> 
> AFAIK if we can't even sync the fs, the balance is definitely going to
> fail, as the most common failure mode for syncfs is RO fs, caused by
> aborted transaction.
> 
> Thus I still think we should error out. Or is there some other
> non-critical failure mode I missed?

The read-only filesystem will be checked when balance starts, that's
where it gets reported.

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

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-29  6:57 [PATCH] btrfs-progs: balance: Sync the fs before balancing metadata chunks Qu Wenruo
2019-02-25 17:21 ` David Sterba
2019-02-26  5:55   ` Qu Wenruo
2019-02-27 16:25     ` David Sterba

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/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-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


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


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