Patchwork ext4: fix free clusters calculation in bigalloc filesystem

login
register
mail settings
Submitter Lukas Czerner
Date Feb. 21, 2013, 8:01 a.m.
Message ID <1361433665-16880-1-git-send-email-lczerner@redhat.com>
Download mbox | patch
Permalink /patch/222194/
State Superseded
Headers show

Comments

Lukas Czerner - Feb. 21, 2013, 8:01 a.m.
ext4_has_free_clusters() should tell us whether there is enough free
clusters to allocate, however number of free clusters in the file system
is converted to blocks using EXT4_C2B() which is not only wrong use of
the macro (we should have used EXT4_NUM_B2C) but it's also completely
wrong concept since everything else is in cluster units.

Moreover when calculating number of root clusters we should be using
macro EXT4_NUM_B2C() instead of EXT4_C2B() otherwise the result will
usually be off by one.

As a result number of free clusters is much bigger than it should have
been and ext4_has_free_clusters() would return 1 even if there is really
not enough free clusters available.

Fix this by removing the EXT4_C2B() conversion of free clusters and
using EXT4_NUM_B2C() when calculating number of root clusters. This bug
affects number of xfstests tests covering file system ENOSPC situation
handling. With this patch most of the ENOSPC problems with bigalloc file
system disappear, especially the errors caused by delayed allocation not
having enough space when the actual allocation is finally requested.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/balloc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Theodore Ts'o - Feb. 22, 2013, 5:10 a.m.
On Thu, Feb 21, 2013 at 09:01:05AM +0100, Lukas Czerner wrote:
> 
> Moreover when calculating number of root clusters we should be using
> macro EXT4_NUM_B2C() instead of EXT4_C2B() otherwise the result will
> usually be off by one.

That should be "instead of EXT4_B2C()", and I don't think this is
true, since the number of blocks should always be a multiple of the
cluster ratio.  So the use of EXT4_B2C() is a bit of an optimization
(it avoids an add and mask operation), but it's probably not a
measurable optimizatoin in practice.

							- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Czerner - Feb. 22, 2013, 7:57 a.m.
On Fri, 22 Feb 2013, Theodore Ts'o wrote:

> Date: Fri, 22 Feb 2013 00:10:48 -0500
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH] ext4: fix free clusters calculation in bigalloc
>     filesystem
> 
> On Thu, Feb 21, 2013 at 09:01:05AM +0100, Lukas Czerner wrote:
> > 
> > Moreover when calculating number of root clusters we should be using
> > macro EXT4_NUM_B2C() instead of EXT4_C2B() otherwise the result will
> > usually be off by one.
> 
> That should be "instead of EXT4_B2C()", and I don't think this is
> true, since the number of blocks should always be a multiple of the
> cluster ratio.  So the use of EXT4_B2C() is a bit of an optimization
> (it avoids an add and mask operation), but it's probably not a
> measurable optimizatoin in practice.
> 
> 							- Ted

Oh right it was EXT4_B2C(), I'll update the description. Even though
it should always be a multiple of cluster ratio it's really bad
precedence for the use of EXT4_B2C() since it is confusing enough
as it is with blocks/clusters.

I'll resend.

-Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index cf18217..9f4302a 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -482,11 +482,11 @@  static int ext4_has_free_clusters(struct ext4_sb_info *sbi,
 
 	free_clusters  = percpu_counter_read_positive(fcc);
 	dirty_clusters = percpu_counter_read_positive(dcc);
-	root_clusters = EXT4_B2C(sbi, ext4_r_blocks_count(sbi->s_es));
+	root_clusters = EXT4_NUM_B2C(sbi, ext4_r_blocks_count(sbi->s_es));
 
 	if (free_clusters - (nclusters + root_clusters + dirty_clusters) <
 					EXT4_FREECLUSTERS_WATERMARK) {
-		free_clusters  = EXT4_C2B(sbi, percpu_counter_sum_positive(fcc));
+		free_clusters  = percpu_counter_sum_positive(fcc);
 		dirty_clusters = percpu_counter_sum_positive(dcc);
 	}
 	/* Check whether we have space after accounting for current