[PATCH] resize2fs: fix overly-pessimistic calculation of minimum size required

[PATCH] resize2fs: fix overly-pessimistic calculation of minimum size required

[Theodore Ts'o]

For extent-mapped file systems, we need to reserve some extra space in
case we need to grow the extent tree.  Calculate the safety margin
more intelligently, so we don't overestimate the amount of space
required.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Reported-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 resize/resize2fs.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/resize/resize2fs.c b/resize/resize2fs.c
index 6b9ca09..38a2fb4 100644
--- a/resize/resize2fs.c
+++ b/resize/resize2fs.c
@@ -2351,9 +2351,10 @@ blk64_t calculate_minimum_resize_size(ext2_filsys fs, int flags)
 	 * inode tables of slack space so the resize operation can be
 	 * guaranteed to finish.
 	 */
+	blks_needed = data_needed;
 	if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_FLEX_BG) {
 		extra_groups = flexbg_size - (groups & (flexbg_size - 1));
-		data_needed += fs->inode_blocks_per_group * extra_groups;
+		blks_needed += fs->inode_blocks_per_group * extra_groups;
 		extra_groups = groups % flexbg_size;
 	}
 
@@ -2387,8 +2388,8 @@ blk64_t calculate_minimum_resize_size(ext2_filsys fs, int flags)
 	 * if we need more group descriptors in order to accomodate our data
 	 * then we need to add them here
 	 */
-	while (data_needed > data_blocks) {
-		blk64_t remainder = data_needed - data_blocks;
+	while (blks_needed > data_blocks) {
+		blk64_t remainder = blks_needed - data_blocks;
 		dgrp_t extra_grps;
 
 		/* figure out how many more groups we need for the data */
@@ -2428,17 +2429,16 @@ blk64_t calculate_minimum_resize_size(ext2_filsys fs, int flags)
 			 */
 			extra_groups = flexbg_size -
 						(groups & (flexbg_size - 1));
-			data_needed += (fs->inode_blocks_per_group *
+			blks_needed += (fs->inode_blocks_per_group *
 					extra_groups);
 			extra_groups = groups % flexbg_size;
 		}
 #ifdef RESIZE2FS_DEBUG
 		if (flags & RESIZE_DEBUG_MIN_CALC)
 			printf("Added %d extra group(s), "
-			       "data_needed %llu, data_blocks %llu, "
-			       "last_start %llu\n",
-			       extra_grps, data_needed, data_blocks,
-			       last_start);
+			       "blks_needed %llu, data_blocks %llu, "
+			       "last_start %llu\n", extra_grps, blks_needed,
+			       data_blocks, last_start);
 #endif
 	}
 
@@ -2453,8 +2453,8 @@ blk64_t calculate_minimum_resize_size(ext2_filsys fs, int flags)
 	 * if this is the case then the last group is going to have data in it
 	 * so we need to adjust the size of the last group accordingly
 	 */
-	if (last_start < data_needed) {
-		blk64_t remainder = data_needed - last_start;
+	if (last_start < blks_needed) {
+		blk64_t remainder = blks_needed - last_start;
 
 #ifdef RESIZE2FS_DEBUG
 		if (flags & RESIZE_DEBUG_MIN_CALC)
@@ -2512,10 +2512,26 @@ blk64_t calculate_minimum_resize_size(ext2_filsys fs, int flags)
 	 * We need to reserve a few extra blocks if extents are
 	 * enabled, in case we need to grow the extent tree.  The more
 	 * we shrink the file system, the more space we need.
+	 *
+	 * The absolute worst case is every single data block is in
+	 * the part of the file system that needs to be evacuated,
+	 * with each data block needs to be in its own extent, and
+	 * with each inode needing at least one extent block.
 	 */
 	if (fs->super->s_feature_incompat & EXT3_FEATURE_INCOMPAT_EXTENTS) {
 		blk64_t safe_margin = (ext2fs_blocks_count(fs->super) -
 				       blks_needed)/500;
+		unsigned int exts_per_blk = (fs->blocksize /
+					     sizeof(struct ext3_extent)) - 1;
+		blk64_t worst_case = ((data_needed + exts_per_blk - 1) /
+				      exts_per_blk);
+
+		if (worst_case < inode_count)
+			data_needed = worst_case;
+
+		if (safe_margin > worst_case)
+			safe_margin = worst_case;
+
 #ifdef RESIZE2FS_DEBUG
 		if (flags & RESIZE_DEBUG_MIN_CALC)
 			printf("Extents safety margin: %llu\n", safe_margin);
-- 
1.9.0

Re: [PATCH] resize2fs: fix overly-pessimistic calculation of minimum size required

[Theodore Ts'o]

On Sat, Apr 26, 2014 at 10:48:14PM -0400, Theodore Ts'o wrote:
> For extent-mapped file systems, we need to reserve some extra space in
> case we need to grow the extent tree.  Calculate the safety margin
> more intelligently, so we don't overestimate the amount of space
> required.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Reported-by: Dmitry Monakhov <dmonakhov@openvz.org>

I'm going to have to self-NACK this.  This patch causes the resize2fs
regression tests to fail.  (In fact, Dmitry's original patch also
causes the resize2fs regression tests to fail.)

The problem is kind of messy; when the file system starts at some
insanely large size, and we shrink it very small, we end up releasing
a lot of inode tables in the first block group (many for other block
groups).  But until we're 100% sure the resize will be successful, we
don't want to start overwriting those inode table blocks.

For this reason, if we try to constrain resize the file system down
from 2TB to 512MB in one shot, we need to do this in multiple steps.
I.e. by calling "resize2fs -M /dev/sdXX" multiple times.

There really isn't a good way around this, and in fact, if people are
going to be doing silly things like take a file system from 16T down
to 750MB, if they need to run resize2fs multiple times, that's fine.
It would be nice if you could shrink the file system down in a single
shot, but it's not high priority.

					- Ted

Re: [PATCH] resize2fs: fix overly-pessimistic calculation of minimum size required

[Dmitry Monakhov]

On Sun, 27 Apr 2014 00:35:37 -0400, "Theodore Ts'o" <tytso@mit.edu> wrote:
> On Sat, Apr 26, 2014 at 10:48:14PM -0400, Theodore Ts'o wrote:
> > For extent-mapped file systems, we need to reserve some extra space in
> > case we need to grow the extent tree.  Calculate the safety margin
> > more intelligently, so we don't overestimate the amount of space
> > required.
> > 
> > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> > Reported-by: Dmitry Monakhov <dmonakhov@openvz.org>
> 
> I'm going to have to self-NACK this.  This patch causes the resize2fs
> regression tests to fail.  (In fact, Dmitry's original patch also
> causes the resize2fs regression tests to fail.)
Agree, regressions are not acceptable. Can you please spacify
which tests are failed. As far as i know xfstetsts has no tests
for resize2fs.
> The problem is kind of messy; when the file system starts at some
> insanely large size, and we shrink it very small, we end up releasing
> a lot of inode tables in the first block group (many for other block
> groups).  But until we're 100% sure the resize will be successful, we
> don't want to start overwriting those inode table blocks.
> 
> For this reason, if we try to constrain resize the file system down
> from 2TB to 512MB in one shot, we need to do this in multiple steps.
> I.e. by calling "resize2fs -M /dev/sdXX" multiple times.
> 
> There really isn't a good way around this, and in fact, if people are
> going to be doing silly things like take a file system from 16T down
> to 750MB, if they need to run resize2fs multiple times, that's fine.
> It would be nice if you could shrink the file system down in a single
> shot, but it's not high priority.
> 
> 					- Ted

Re: [PATCH] resize2fs: fix overly-pessimistic calculation of minimum size required

[Theodore Ts'o]

On Mon, Apr 28, 2014 at 11:34:54AM +0400, Dmitry Monakhov wrote:
> > I'm going to have to self-NACK this.  This patch causes the resize2fs
> > regression tests to fail.  (In fact, Dmitry's original patch also
> > causes the resize2fs regression tests to fail.)
>
> Agree, regressions are not acceptable. Can you please spacify
> which tests are failed. As far as i know xfstetsts has no tests
> for resize2fs.

It was 2 of the tests from e2fsprogs's "make check", where off-one
resize2fs regression tests belong.  The xfstests suite is a good place
to test on-line resize functionality, where the bulk of the code that
has to function correctly is in the kernel,, but for testing
functionality which is specific to e2fsprogs, it's better to test it
within the context of e2fsprogs's regression tests.

Anyway, the patch set I sent out fixes up the problem.

	    	      	     	       	  - Ted

Re: [PATCH] resize2fs: fix overly-pessimistic calculation of minimum size required

[Lukáš Czerner]

On Mon, 28 Apr 2014, Theodore Ts'o wrote:

> Date: Mon, 28 Apr 2014 07:47:55 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Dmitry Monakhov <dmonakhov@openvz.org>
> Cc: Ext4 Developers List <linux-ext4@vger.kernel.org>
> Subject: Re: [PATCH] resize2fs: fix overly-pessimistic calculation of minimum
>     size required
> 
> On Mon, Apr 28, 2014 at 11:34:54AM +0400, Dmitry Monakhov wrote:
> > > I'm going to have to self-NACK this.  This patch causes the resize2fs
> > > regression tests to fail.  (In fact, Dmitry's original patch also
> > > causes the resize2fs regression tests to fail.)
> >
> > Agree, regressions are not acceptable. Can you please spacify
> > which tests are failed. As far as i know xfstetsts has no tests
> > for resize2fs.
> 
> It was 2 of the tests from e2fsprogs's "make check", where off-one
> resize2fs regression tests belong.  The xfstests suite is a good place
> to test on-line resize functionality, where the bulk of the code that
> has to function correctly is in the kernel,, but for testing
> functionality which is specific to e2fsprogs, it's better to test it
> within the context of e2fsprogs's regression tests.

While xfstests is definitely a good place for online resize test, I
do not see any reason not to put some offline resize tests into
xfstests as well. If nothing it is run more often and by much more
people than e2fsprogs tests.

Currently AFAICT we only have one resize2fs test in xfstests.

Thanks!
-Lukas

> 
> Anyway, the patch set I sent out fixes up the problem.
> 
> 	    	      	     	       	  - 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
>