[PATCH 1/4] e2fsck: Discard only unused parts of inode table

[PATCH 1/4] e2fsck: Discard only unused parts of inode table

[Lukas Czerner]

When calling e2fsck with '-E discard' option it might happen that
valid inodes are discarded accidentally. This is because we just
discard the part of inode table which lies past the free inode count.
This is terribly wrong (sorry!).

This patch fixes it so only the free parts of an inode table
is discarded, leaving used inodes intact. This was tested with highly
fragmented inode tables with block size 4k and 1k.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Reported-by: Phillip Susi <psusi@ubuntu.com>
---
 e2fsck/pass5.c |   90 +++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index 1e836e3..ee73dd5 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -94,6 +94,52 @@ static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
 		ctx->options &= ~E2F_OPT_DISCARD;
 }
 
+/*
+ * This will try to discard number 'count' inodes starting at
+ * inode number 'start' within the 'group'. Note that 'start'
+ * is 1-based, it means that we need to adjust it by -1 in this
+ * function to compute right offset in the particular inode table.
+ */
+static void e2fsck_discard_inodes(e2fsck_t ctx, int group,
+				  int start, int count)
+{
+	ext2_filsys fs = ctx->fs;
+	blk64_t blk, num;
+	int orig = count;
+
+	/*
+	 * Sanity check for 'start'
+	 */
+	if ((start < 1) || (start > EXT2_INODES_PER_GROUP(fs->super))) {
+		printf("PROGRAMMING ERROR: Got start %d outside of group %d!"
+		       " Disabling discard\n",
+			start, group);
+		ctx->options &= ~E2F_OPT_DISCARD;
+	}
+
+	if ((ctx->options & E2F_OPT_NO) || !(ctx->options & E2F_OPT_DISCARD))
+		return;
+
+	/*
+	 * Start is inode number within the group which starts
+	 * counting from 1, so we need to adjust it.
+	 */
+	start -= 1;
+
+	/*
+	 * We can discard only blocks containing only unused
+	 * inodes in the table.
+	 */
+	blk = DIV_ROUND_UP(start,
+		EXT2_INODES_PER_BLOCK(fs->super));
+	count -= (blk * EXT2_INODES_PER_BLOCK(fs->super) - start);
+	blk += ext2fs_inode_table_loc(fs, group);
+	num = count / EXT2_INODES_PER_BLOCK(fs->super);
+
+	if (num > 0)
+		e2fsck_discard_blocks(ctx, fs->io->manager, blk, num);
+}
+
 #define NO_BLK ((blk64_t) -1)
 
 static void print_bitmap_problem(e2fsck_t ctx, int problem,
@@ -435,6 +481,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
 	int		skip_group = 0;
 	int		redo_flag = 0;
 	io_manager	manager = ctx->fs->io->manager;
+	unsigned long long	first_free = fs->super->s_inodes_per_group + 1;
 
 	clear_problem_context(&pctx);
 	free_array = (int *) e2fsck_allocate_memory(ctx,
@@ -497,6 +544,7 @@ redo_counts:
 				 * are 0, count the free inode,
 				 * skip the current block group.
 				 */
+				first_free = 1;
 				inodes = fs->super->s_inodes_per_group - 1;
 				group_free = inodes;
 				free_inodes += inodes;
@@ -561,50 +609,46 @@ redo_counts:
 		ctx->options &= ~E2F_OPT_DISCARD;
 
 do_counts:
+		inodes++;
 		if (bitmap) {
 			if (ext2fs_test_inode_bitmap2(ctx->inode_dir_map, i))
 				dirs_count++;
+			if (inodes > first_free) {
+				e2fsck_discard_inodes(ctx, group, first_free,
+						      inodes - first_free);
+				first_free = fs->super->s_inodes_per_group + 1;
+			}
 		} else if (!skip_group || csum_flag) {
 			group_free++;
 			free_inodes++;
+			if (first_free > inodes)
+				first_free = inodes;
 		}
 
-		inodes++;
 		if ((inodes == fs->super->s_inodes_per_group) ||
 		    (i == fs->super->s_inodes_count)) {
-
-			free_array[group] = group_free;
-			dir_array[group] = dirs_count;
-
-			/* Discard inode table */
-			if (ctx->options & E2F_OPT_DISCARD) {
-				blk64_t used_blks, blk, num;
-
-				used_blks = DIV_ROUND_UP(
-					(EXT2_INODES_PER_GROUP(fs->super) -
-					group_free),
-					EXT2_INODES_PER_BLOCK(fs->super));
-
-				blk = ext2fs_inode_table_loc(fs, group) +
-				      used_blks;
-				num = fs->inode_blocks_per_group -
-				      used_blks;
-				e2fsck_discard_blocks(ctx, manager, blk, num);
-			}
-
+			/*
+			 * If the last inode is free, we can discard it as well.
+			 */
+			if (inodes >= first_free)
+				e2fsck_discard_inodes(ctx, group, first_free,
+						      inodes - first_free + 1);
 			/*
 			 * If discard zeroes data and the group inode table
 			 * was not zeroed yet, set itable as zeroed
 			 */
 			if ((ctx->options & E2F_OPT_DISCARD) &&
-			    (io_channel_discard_zeroes_data(fs->io)) &&
+			    io_channel_discard_zeroes_data(fs->io) &&
 			    !(ext2fs_bg_flags_test(fs, group,
-						  EXT2_BG_INODE_ZEROED))) {
+						   EXT2_BG_INODE_ZEROED))) {
 				ext2fs_bg_flags_set(fs, group,
 						    EXT2_BG_INODE_ZEROED);
 				ext2fs_group_desc_csum_set(fs, group);
 			}
 
+			first_free = fs->super->s_inodes_per_group + 1;
+			free_array[group] = group_free;
+			dir_array[group] = dirs_count;
 			group ++;
 			inodes = 0;
 			skip_group = 0;
-- 
1.7.4.4

[PATCH 2/4] e2fsck: Do not forget to discard last block group

[Lukas Czerner]

Previously when running e2fsck with '-E discard' argument the end of
the last group has not been discarded. This patch fixes it so we
always discard the end of the last group if needed.

This commit also removes unneeded argument from the
e2fsck_discard_blocks(). Simultaneously the commit causes the block
groups with BLOCK_UNINIT flag not to be discarded, which makes
sense because we do not need to reclaim the space since so far
there has not been written anything.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 e2fsck/pass5.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index ee73dd5..37bbac2 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -74,8 +74,8 @@ void e2fsck_pass5(e2fsck_t ctx)
 	print_resource_track(ctx, _("Pass 5"), &rtrack, ctx->fs->io);
 }
 
-static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
-				  blk64_t start, blk64_t count)
+static void e2fsck_discard_blocks(e2fsck_t ctx, blk64_t start,
+				  blk64_t count)
 {
 	ext2_filsys fs = ctx->fs;
 
@@ -85,7 +85,7 @@ static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
 	 * not enough to fix the problem, hence it is not safe to run discard
 	 * in this case.
 	 */
-	if (ext2fs_test_changed(ctx->fs))
+	if (ext2fs_test_changed(fs))
 		ctx->options &= ~E2F_OPT_DISCARD;
 
 	if (!(ctx->options & E2F_OPT_NO) &&
@@ -137,7 +137,7 @@ static void e2fsck_discard_inodes(e2fsck_t ctx, int group,
 	num = count / EXT2_INODES_PER_BLOCK(fs->super);
 
 	if (num > 0)
-		e2fsck_discard_blocks(ctx, fs->io->manager, blk, num);
+		e2fsck_discard_blocks(ctx, blk, num);
 }
 
 #define NO_BLK ((blk64_t) -1)
@@ -377,16 +377,24 @@ redo_counts:
 			free_blocks++;
 			if (first_free > i)
 				first_free = i;
-		} else {
-			if (i > first_free)
-				e2fsck_discard_blocks(ctx, manager, first_free,
-						      (i - first_free));
+		} else if (i > first_free) {
+			e2fsck_discard_blocks(ctx, first_free,
+					      (i - first_free));
 			first_free = ext2fs_blocks_count(fs->super);
 		}
 		blocks ++;
 		if ((blocks == fs->super->s_clusters_per_group) ||
 		    (EXT2FS_B2C(fs, i) ==
 		     EXT2FS_B2C(fs, ext2fs_blocks_count(fs->super)-1))) {
+			/*
+			 * If the last block of this group is free, then we can
+			 * discard it as well.
+			 */
+			if (i >= first_free)
+				e2fsck_discard_blocks(ctx, first_free,
+						      (i - first_free) + 1);
+			first_free = ext2fs_blocks_count(fs->super);
+
 			free_array[group] = group_free;
 			group ++;
 			blocks = 0;
-- 
1.7.4.4

[PATCH 3/4] e2fsck: Do not discard when in read only mode

[Lukas Czerner]

When argument '-n' was specified and should run in read-only mode, we
should not attempt to discard anything. In order to do that we have to
check for E2F_OPT_NO flag and clear E2F_OPT_DISCARD flag if E2F_OPT_NO
is set.

This commit fixes the problem when we would mark inode tables as zeroed
(EXT2_BG_INODE_ZEROED) even when e2fsck is running in read-only mode. We
also move the check for E2F_OPT_NO so we can clear E2F_OPT_DISCARD as
early as possible.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 e2fsck/pass5.c |    5 ++---
 e2fsck/unix.c  |    5 +++++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index 37bbac2..741e6dd 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -88,8 +88,7 @@ static void e2fsck_discard_blocks(e2fsck_t ctx, blk64_t start,
 	if (ext2fs_test_changed(fs))
 		ctx->options &= ~E2F_OPT_DISCARD;
 
-	if (!(ctx->options & E2F_OPT_NO) &&
-	    (ctx->options & E2F_OPT_DISCARD) &&
+	if ((ctx->options & E2F_OPT_DISCARD) &&
 	    (io_channel_discard(fs->io, start, count)))
 		ctx->options &= ~E2F_OPT_DISCARD;
 }
@@ -117,7 +116,7 @@ static void e2fsck_discard_inodes(e2fsck_t ctx, int group,
 		ctx->options &= ~E2F_OPT_DISCARD;
 	}
 
-	if ((ctx->options & E2F_OPT_NO) || !(ctx->options & E2F_OPT_DISCARD))
+	if (!(ctx->options & E2F_OPT_DISCARD))
 		return;
 
 	/*
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index 6f97b0f..b31a1e3 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -903,6 +903,11 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
 	profile_set_syntax_err_cb(syntax_err_report);
 	profile_init(config_fn, &ctx->profile);
 
+	/* Turn off discard in read-only mode */
+	if ((ctx->options & E2F_OPT_NO) &&
+	    (ctx->options & E2F_OPT_DISCARD))
+		ctx->options &= ~E2F_OPT_DISCARD;
+
 	if (flush) {
 		fd = open(ctx->filesystem_name, O_RDONLY, 0);
 		if (fd < 0) {
-- 
1.7.4.4

[PATCH 4/4] e2fsck: Do not discard itable if discard doen't zero data

[Lukas Czerner]

We do not want to discard inode table if the underlying device does not
return zeros when reading non-provisioned blocks. The reason is that if
the inode table is not zeroed yet, then discard would not help us since
we would have to zero it anyway. In the case that inode table was
already zeroed, then the discard would cause subsequent reads to contain
non-deterministic data so we would not be able to assume that the inode
table was zeroed and we would need to zero it again, which does not
really make sense.

This commit adds check to prevent inode table from being discarded if
the discard does not zero data.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 e2fsck/pass5.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index 741e6dd..9e63037 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -116,7 +116,18 @@ static void e2fsck_discard_inodes(e2fsck_t ctx, int group,
 		ctx->options &= ~E2F_OPT_DISCARD;
 	}
 
-	if (!(ctx->options & E2F_OPT_DISCARD))
+	/*
+	 * Do not attempt to discard if E2F_OPT_DISCARD is not set. And also
+	 * skip the discard on this group if discard does not zero data.
+	 * The reason is that if the inode table is not zeroed discard would
+	 * no help us since we need to zero it anyway, or if the inode table
+	 * is zeroed then the read after discard would not be deterministic
+	 * anyway and we would not be able to assume that this inode table
+	 * was zeroed anymore so we would have to zero it again, which does
+	 * not really make sense.
+	 */
+	if (!(ctx->options & E2F_OPT_DISCARD) ||
+	    !io_channel_discard_zeroes_data(fs->io))
 		return;
 
 	/*
-- 
1.7.4.4

Re: [PATCH 1/4] e2fsck: Discard only unused parts of inode table

[Eric Sandeen]

On 03/05/2012 01:49 AM, Lukas Czerner wrote:
> When calling e2fsck with '-E discard' option it might happen that
> valid inodes are discarded accidentally. This is because we just
> discard the part of inode table which lies past the free inode count.
> This is terribly wrong (sorry!).
> 
> This patch fixes it so only the free parts of an inode table
> is discarded, leaving used inodes intact. This was tested with highly
> fragmented inode tables with block size 4k and 1k.

Every time I look at this I have new comments.  The surrounding code
is confusing to read, IMHO, so don't take it too hard. ;)

> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> Reported-by: Phillip Susi <psusi@ubuntu.com>

I'll go ahead and:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

so hopefully we can get this in & fix the bug.

but I have one more suggestion, which could be done as a cleanup later
on (I think the check_*_bitmaps could use a fair bit of cleanup for
clarity):

/*
 * If the last inode is free, we can discard it as well.
 */
if (inodes >= first_free)
	e2fsck_discard_inodes(ctx, group, first_free,
					inodes - first_free + 1);

could/should probably be something like:

if (inodes == first_free)
	e2fsck_discard_inodes(ctx, group, inodes, 1);

I _think_ this case should only, ever, handle the last inode in the
group, right?

-Eric

> ---
>  e2fsck/pass5.c |   90 +++++++++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 67 insertions(+), 23 deletions(-)
> 
> diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
> index 1e836e3..ee73dd5 100644
> --- a/e2fsck/pass5.c
> +++ b/e2fsck/pass5.c
> @@ -94,6 +94,52 @@ static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
>  		ctx->options &= ~E2F_OPT_DISCARD;
>  }
>  
> +/*
> + * This will try to discard number 'count' inodes starting at
> + * inode number 'start' within the 'group'. Note that 'start'
> + * is 1-based, it means that we need to adjust it by -1 in this
> + * function to compute right offset in the particular inode table.
> + */
> +static void e2fsck_discard_inodes(e2fsck_t ctx, int group,
> +				  int start, int count)
> +{
> +	ext2_filsys fs = ctx->fs;
> +	blk64_t blk, num;
> +	int orig = count;
> +
> +	/*
> +	 * Sanity check for 'start'
> +	 */
> +	if ((start < 1) || (start > EXT2_INODES_PER_GROUP(fs->super))) {
> +		printf("PROGRAMMING ERROR: Got start %d outside of group %d!"
> +		       " Disabling discard\n",
> +			start, group);
> +		ctx->options &= ~E2F_OPT_DISCARD;
> +	}
> +
> +	if ((ctx->options & E2F_OPT_NO) || !(ctx->options & E2F_OPT_DISCARD))
> +		return;
> +
> +	/*
> +	 * Start is inode number within the group which starts
> +	 * counting from 1, so we need to adjust it.
> +	 */
> +	start -= 1;
> +
> +	/*
> +	 * We can discard only blocks containing only unused
> +	 * inodes in the table.
> +	 */
> +	blk = DIV_ROUND_UP(start,
> +		EXT2_INODES_PER_BLOCK(fs->super));
> +	count -= (blk * EXT2_INODES_PER_BLOCK(fs->super) - start);
> +	blk += ext2fs_inode_table_loc(fs, group);
> +	num = count / EXT2_INODES_PER_BLOCK(fs->super);
> +
> +	if (num > 0)
> +		e2fsck_discard_blocks(ctx, fs->io->manager, blk, num);
> +}
> +
>  #define NO_BLK ((blk64_t) -1)
>  
>  static void print_bitmap_problem(e2fsck_t ctx, int problem,
> @@ -435,6 +481,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
>  	int		skip_group = 0;
>  	int		redo_flag = 0;
>  	io_manager	manager = ctx->fs->io->manager;
> +	unsigned long long	first_free = fs->super->s_inodes_per_group + 1;
>  
>  	clear_problem_context(&pctx);
>  	free_array = (int *) e2fsck_allocate_memory(ctx,
> @@ -497,6 +544,7 @@ redo_counts:
>  				 * are 0, count the free inode,
>  				 * skip the current block group.
>  				 */
> +				first_free = 1;
>  				inodes = fs->super->s_inodes_per_group - 1;
>  				group_free = inodes;
>  				free_inodes += inodes;
> @@ -561,50 +609,46 @@ redo_counts:
>  		ctx->options &= ~E2F_OPT_DISCARD;
>  
>  do_counts:
> +		inodes++;
>  		if (bitmap) {
>  			if (ext2fs_test_inode_bitmap2(ctx->inode_dir_map, i))
>  				dirs_count++;
> +			if (inodes > first_free) {
> +				e2fsck_discard_inodes(ctx, group, first_free,
> +						      inodes - first_free);
> +				first_free = fs->super->s_inodes_per_group + 1;
> +			}
>  		} else if (!skip_group || csum_flag) {
>  			group_free++;
>  			free_inodes++;
> +			if (first_free > inodes)
> +				first_free = inodes;
>  		}
>  
> -		inodes++;
>  		if ((inodes == fs->super->s_inodes_per_group) ||
>  		    (i == fs->super->s_inodes_count)) {
> -
> -			free_array[group] = group_free;
> -			dir_array[group] = dirs_count;
> -
> -			/* Discard inode table */
> -			if (ctx->options & E2F_OPT_DISCARD) {
> -				blk64_t used_blks, blk, num;
> -
> -				used_blks = DIV_ROUND_UP(
> -					(EXT2_INODES_PER_GROUP(fs->super) -
> -					group_free),
> -					EXT2_INODES_PER_BLOCK(fs->super));
> -
> -				blk = ext2fs_inode_table_loc(fs, group) +
> -				      used_blks;
> -				num = fs->inode_blocks_per_group -
> -				      used_blks;
> -				e2fsck_discard_blocks(ctx, manager, blk, num);
> -			}
> -
> +			/*
> +			 * If the last inode is free, we can discard it as well.
> +			 */
> +			if (inodes >= first_free)
> +				e2fsck_discard_inodes(ctx, group, first_free,
> +						      inodes - first_free + 1);
>  			/*
>  			 * If discard zeroes data and the group inode table
>  			 * was not zeroed yet, set itable as zeroed
>  			 */
>  			if ((ctx->options & E2F_OPT_DISCARD) &&
> -			    (io_channel_discard_zeroes_data(fs->io)) &&
> +			    io_channel_discard_zeroes_data(fs->io) &&
>  			    !(ext2fs_bg_flags_test(fs, group,
> -						  EXT2_BG_INODE_ZEROED))) {
> +						   EXT2_BG_INODE_ZEROED))) {
>  				ext2fs_bg_flags_set(fs, group,
>  						    EXT2_BG_INODE_ZEROED);
>  				ext2fs_group_desc_csum_set(fs, group);
>  			}
>  
> +			first_free = fs->super->s_inodes_per_group + 1;
> +			free_array[group] = group_free;
> +			dir_array[group] = dirs_count;
>  			group ++;
>  			inodes = 0;
>  			skip_group = 0;

Re: [PATCH 1/4] e2fsck: Discard only unused parts of inode table

[Lukas Czerner]

On Mon, 5 Mar 2012, Eric Sandeen wrote:

> On 03/05/2012 01:49 AM, Lukas Czerner wrote:
> > When calling e2fsck with '-E discard' option it might happen that
> > valid inodes are discarded accidentally. This is because we just
> > discard the part of inode table which lies past the free inode count.
> > This is terribly wrong (sorry!).
> > 
> > This patch fixes it so only the free parts of an inode table
> > is discarded, leaving used inodes intact. This was tested with highly
> > fragmented inode tables with block size 4k and 1k.
> 
> Every time I look at this I have new comments.  The surrounding code
> is confusing to read, IMHO, so don't take it too hard. ;)
> 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > Reported-by: Phillip Susi <psusi@ubuntu.com>
> 
> I'll go ahead and:
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> so hopefully we can get this in & fix the bug.
> 
> but I have one more suggestion, which could be done as a cleanup later
> on (I think the check_*_bitmaps could use a fair bit of cleanup for
> clarity):
> 
> /*
>  * If the last inode is free, we can discard it as well.
>  */
> if (inodes >= first_free)
> 	e2fsck_discard_inodes(ctx, group, first_free,
> 					inodes - first_free + 1);
> 
> could/should probably be something like:
> 
> if (inodes == first_free)
> 	e2fsck_discard_inodes(ctx, group, inodes, 1);
> 
> I _think_ this case should only, ever, handle the last inode in the
> group, right?

Hi Eric,

thanks for the review. It almost seems that I am not able to write
understandable comments :). The reason the comment is there is that we
usually do:

	if (inodes > first_free)

but in this particular case we do

	if (inodes >= first_free)

And we have '=' there because at that point we might have last inode
which is free. But because we will not have another iteration on that
group, there will not be another 'used' inode which will result the code
to go in the 'if (bitmap)' branch discarding that last inode. So we have
to discard the last inode as well.

However, doing (inodes == first_free) would be wrong, because we might
have more free inodes at the end of the inode table so first_free will
be smaller than inodes (which is actually the usual case as we do in the
'if (bitmap)' branch).

So the comment:

	If the last inode is free, we can discard it as well.

is simply there 'because' we will only hit this if the last inode is
free and we have to discard it as well (thus the '>=').

Hope that makes sense, I am not very good at explaining things :)

Thanks!
-Lukas

> 
> -Eric
> 
> > ---
> >  e2fsck/pass5.c |   90 +++++++++++++++++++++++++++++++++++++++++--------------
> >  1 files changed, 67 insertions(+), 23 deletions(-)
> > 
> > diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
> > index 1e836e3..ee73dd5 100644
> > --- a/e2fsck/pass5.c
> > +++ b/e2fsck/pass5.c
> > @@ -94,6 +94,52 @@ static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
> >  		ctx->options &= ~E2F_OPT_DISCARD;
> >  }
> >  
> > +/*
> > + * This will try to discard number 'count' inodes starting at
> > + * inode number 'start' within the 'group'. Note that 'start'
> > + * is 1-based, it means that we need to adjust it by -1 in this
> > + * function to compute right offset in the particular inode table.
> > + */
> > +static void e2fsck_discard_inodes(e2fsck_t ctx, int group,
> > +				  int start, int count)
> > +{
> > +	ext2_filsys fs = ctx->fs;
> > +	blk64_t blk, num;
> > +	int orig = count;
> > +
> > +	/*
> > +	 * Sanity check for 'start'
> > +	 */
> > +	if ((start < 1) || (start > EXT2_INODES_PER_GROUP(fs->super))) {
> > +		printf("PROGRAMMING ERROR: Got start %d outside of group %d!"
> > +		       " Disabling discard\n",
> > +			start, group);
> > +		ctx->options &= ~E2F_OPT_DISCARD;
> > +	}
> > +
> > +	if ((ctx->options & E2F_OPT_NO) || !(ctx->options & E2F_OPT_DISCARD))
> > +		return;
> > +
> > +	/*
> > +	 * Start is inode number within the group which starts
> > +	 * counting from 1, so we need to adjust it.
> > +	 */
> > +	start -= 1;
> > +
> > +	/*
> > +	 * We can discard only blocks containing only unused
> > +	 * inodes in the table.
> > +	 */
> > +	blk = DIV_ROUND_UP(start,
> > +		EXT2_INODES_PER_BLOCK(fs->super));
> > +	count -= (blk * EXT2_INODES_PER_BLOCK(fs->super) - start);
> > +	blk += ext2fs_inode_table_loc(fs, group);
> > +	num = count / EXT2_INODES_PER_BLOCK(fs->super);
> > +
> > +	if (num > 0)
> > +		e2fsck_discard_blocks(ctx, fs->io->manager, blk, num);
> > +}
> > +
> >  #define NO_BLK ((blk64_t) -1)
> >  
> >  static void print_bitmap_problem(e2fsck_t ctx, int problem,
> > @@ -435,6 +481,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
> >  	int		skip_group = 0;
> >  	int		redo_flag = 0;
> >  	io_manager	manager = ctx->fs->io->manager;
> > +	unsigned long long	first_free = fs->super->s_inodes_per_group + 1;
> >  
> >  	clear_problem_context(&pctx);
> >  	free_array = (int *) e2fsck_allocate_memory(ctx,
> > @@ -497,6 +544,7 @@ redo_counts:
> >  				 * are 0, count the free inode,
> >  				 * skip the current block group.
> >  				 */
> > +				first_free = 1;
> >  				inodes = fs->super->s_inodes_per_group - 1;
> >  				group_free = inodes;
> >  				free_inodes += inodes;
> > @@ -561,50 +609,46 @@ redo_counts:
> >  		ctx->options &= ~E2F_OPT_DISCARD;
> >  
> >  do_counts:
> > +		inodes++;
> >  		if (bitmap) {
> >  			if (ext2fs_test_inode_bitmap2(ctx->inode_dir_map, i))
> >  				dirs_count++;
> > +			if (inodes > first_free) {
> > +				e2fsck_discard_inodes(ctx, group, first_free,
> > +						      inodes - first_free);
> > +				first_free = fs->super->s_inodes_per_group + 1;
> > +			}
> >  		} else if (!skip_group || csum_flag) {
> >  			group_free++;
> >  			free_inodes++;
> > +			if (first_free > inodes)
> > +				first_free = inodes;
> >  		}
> >  
> > -		inodes++;
> >  		if ((inodes == fs->super->s_inodes_per_group) ||
> >  		    (i == fs->super->s_inodes_count)) {
> > -
> > -			free_array[group] = group_free;
> > -			dir_array[group] = dirs_count;
> > -
> > -			/* Discard inode table */
> > -			if (ctx->options & E2F_OPT_DISCARD) {
> > -				blk64_t used_blks, blk, num;
> > -
> > -				used_blks = DIV_ROUND_UP(
> > -					(EXT2_INODES_PER_GROUP(fs->super) -
> > -					group_free),
> > -					EXT2_INODES_PER_BLOCK(fs->super));
> > -
> > -				blk = ext2fs_inode_table_loc(fs, group) +
> > -				      used_blks;
> > -				num = fs->inode_blocks_per_group -
> > -				      used_blks;
> > -				e2fsck_discard_blocks(ctx, manager, blk, num);
> > -			}
> > -
> > +			/*
> > +			 * If the last inode is free, we can discard it as well.
> > +			 */
> > +			if (inodes >= first_free)
> > +				e2fsck_discard_inodes(ctx, group, first_free,
> > +						      inodes - first_free + 1);
> >  			/*
> >  			 * If discard zeroes data and the group inode table
> >  			 * was not zeroed yet, set itable as zeroed
> >  			 */
> >  			if ((ctx->options & E2F_OPT_DISCARD) &&
> > -			    (io_channel_discard_zeroes_data(fs->io)) &&
> > +			    io_channel_discard_zeroes_data(fs->io) &&
> >  			    !(ext2fs_bg_flags_test(fs, group,
> > -						  EXT2_BG_INODE_ZEROED))) {
> > +						   EXT2_BG_INODE_ZEROED))) {
> >  				ext2fs_bg_flags_set(fs, group,
> >  						    EXT2_BG_INODE_ZEROED);
> >  				ext2fs_group_desc_csum_set(fs, group);
> >  			}
> >  
> > +			first_free = fs->super->s_inodes_per_group + 1;
> > +			free_array[group] = group_free;
> > +			dir_array[group] = dirs_count;
> >  			group ++;
> >  			inodes = 0;
> >  			skip_group = 0;
> 
> 

-- 

Re: [PATCH 1/4] e2fsck: Discard only unused parts of inode table

[Eric Sandeen]

On 03/05/2012 12:43 PM, Lukas Czerner wrote:
> On Mon, 5 Mar 2012, Eric Sandeen wrote:
> 
>> On 03/05/2012 01:49 AM, Lukas Czerner wrote:
>>> When calling e2fsck with '-E discard' option it might happen that
>>> valid inodes are discarded accidentally. This is because we just
>>> discard the part of inode table which lies past the free inode count.
>>> This is terribly wrong (sorry!).
>>>
>>> This patch fixes it so only the free parts of an inode table
>>> is discarded, leaving used inodes intact. This was tested with highly
>>> fragmented inode tables with block size 4k and 1k.
>>
>> Every time I look at this I have new comments.  The surrounding code
>> is confusing to read, IMHO, so don't take it too hard. ;)
>>
>>> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
>>> Reported-by: Phillip Susi <psusi@ubuntu.com>
>>
>> I'll go ahead and:
>>
>> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>>
>> so hopefully we can get this in & fix the bug.
>>
>> but I have one more suggestion, which could be done as a cleanup later
>> on (I think the check_*_bitmaps could use a fair bit of cleanup for
>> clarity):
>>
>> /*
>>  * If the last inode is free, we can discard it as well.
>>  */
>> if (inodes >= first_free)
>> 	e2fsck_discard_inodes(ctx, group, first_free,
>> 					inodes - first_free + 1);
>>
>> could/should probably be something like:
>>
>> if (inodes == first_free)
>> 	e2fsck_discard_inodes(ctx, group, inodes, 1);
>>
>> I _think_ this case should only, ever, handle the last inode in the
>> group, right?
> 
> Hi Eric,
> 
> thanks for the review. It almost seems that I am not able to write
> understandable comments :). 

It's as least as likely that I cannot understand understandable code ;)

I think you are right, and my suggestion was wrong.  Sorry for the noise!

-Eric

> The reason the comment is there is that we
> usually do:
> 
> 	if (inodes > first_free)
> 
> but in this particular case we do
> 
> 	if (inodes >= first_free)
> 
> And we have '=' there because at that point we might have last inode
> which is free. But because we will not have another iteration on that
> group, there will not be another 'used' inode which will result the code
> to go in the 'if (bitmap)' branch discarding that last inode. So we have
> to discard the last inode as well.
> 
> However, doing (inodes == first_free) would be wrong, because we might
> have more free inodes at the end of the inode table so first_free will
> be smaller than inodes (which is actually the usual case as we do in the
> 'if (bitmap)' branch).
> 
> So the comment:
> 
> 	If the last inode is free, we can discard it as well.
> 
> is simply there 'because' we will only hit this if the last inode is
> free and we have to discard it as well (thus the '>=').
> 
> Hope that makes sense, I am not very good at explaining things :)
> 
> Thanks!
> -Lukas
> 
>>
>> -Eric
>>
>>> ---
>>>  e2fsck/pass5.c |   90 +++++++++++++++++++++++++++++++++++++++++--------------
>>>  1 files changed, 67 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
>>> index 1e836e3..ee73dd5 100644
>>> --- a/e2fsck/pass5.c
>>> +++ b/e2fsck/pass5.c
>>> @@ -94,6 +94,52 @@ static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
>>>  		ctx->options &= ~E2F_OPT_DISCARD;
>>>  }
>>>  
>>> +/*
>>> + * This will try to discard number 'count' inodes starting at
>>> + * inode number 'start' within the 'group'. Note that 'start'
>>> + * is 1-based, it means that we need to adjust it by -1 in this
>>> + * function to compute right offset in the particular inode table.
>>> + */
>>> +static void e2fsck_discard_inodes(e2fsck_t ctx, int group,
>>> +				  int start, int count)
>>> +{
>>> +	ext2_filsys fs = ctx->fs;
>>> +	blk64_t blk, num;
>>> +	int orig = count;
>>> +
>>> +	/*
>>> +	 * Sanity check for 'start'
>>> +	 */
>>> +	if ((start < 1) || (start > EXT2_INODES_PER_GROUP(fs->super))) {
>>> +		printf("PROGRAMMING ERROR: Got start %d outside of group %d!"
>>> +		       " Disabling discard\n",
>>> +			start, group);
>>> +		ctx->options &= ~E2F_OPT_DISCARD;
>>> +	}
>>> +
>>> +	if ((ctx->options & E2F_OPT_NO) || !(ctx->options & E2F_OPT_DISCARD))
>>> +		return;
>>> +
>>> +	/*
>>> +	 * Start is inode number within the group which starts
>>> +	 * counting from 1, so we need to adjust it.
>>> +	 */
>>> +	start -= 1;
>>> +
>>> +	/*
>>> +	 * We can discard only blocks containing only unused
>>> +	 * inodes in the table.
>>> +	 */
>>> +	blk = DIV_ROUND_UP(start,
>>> +		EXT2_INODES_PER_BLOCK(fs->super));
>>> +	count -= (blk * EXT2_INODES_PER_BLOCK(fs->super) - start);
>>> +	blk += ext2fs_inode_table_loc(fs, group);
>>> +	num = count / EXT2_INODES_PER_BLOCK(fs->super);
>>> +
>>> +	if (num > 0)
>>> +		e2fsck_discard_blocks(ctx, fs->io->manager, blk, num);
>>> +}
>>> +
>>>  #define NO_BLK ((blk64_t) -1)
>>>  
>>>  static void print_bitmap_problem(e2fsck_t ctx, int problem,
>>> @@ -435,6 +481,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
>>>  	int		skip_group = 0;
>>>  	int		redo_flag = 0;
>>>  	io_manager	manager = ctx->fs->io->manager;
>>> +	unsigned long long	first_free = fs->super->s_inodes_per_group + 1;
>>>  
>>>  	clear_problem_context(&pctx);
>>>  	free_array = (int *) e2fsck_allocate_memory(ctx,
>>> @@ -497,6 +544,7 @@ redo_counts:
>>>  				 * are 0, count the free inode,
>>>  				 * skip the current block group.
>>>  				 */
>>> +				first_free = 1;
>>>  				inodes = fs->super->s_inodes_per_group - 1;
>>>  				group_free = inodes;
>>>  				free_inodes += inodes;
>>> @@ -561,50 +609,46 @@ redo_counts:
>>>  		ctx->options &= ~E2F_OPT_DISCARD;
>>>  
>>>  do_counts:
>>> +		inodes++;
>>>  		if (bitmap) {
>>>  			if (ext2fs_test_inode_bitmap2(ctx->inode_dir_map, i))
>>>  				dirs_count++;
>>> +			if (inodes > first_free) {
>>> +				e2fsck_discard_inodes(ctx, group, first_free,
>>> +						      inodes - first_free);
>>> +				first_free = fs->super->s_inodes_per_group + 1;
>>> +			}
>>>  		} else if (!skip_group || csum_flag) {
>>>  			group_free++;
>>>  			free_inodes++;
>>> +			if (first_free > inodes)
>>> +				first_free = inodes;
>>>  		}
>>>  
>>> -		inodes++;
>>>  		if ((inodes == fs->super->s_inodes_per_group) ||
>>>  		    (i == fs->super->s_inodes_count)) {
>>> -
>>> -			free_array[group] = group_free;
>>> -			dir_array[group] = dirs_count;
>>> -
>>> -			/* Discard inode table */
>>> -			if (ctx->options & E2F_OPT_DISCARD) {
>>> -				blk64_t used_blks, blk, num;
>>> -
>>> -				used_blks = DIV_ROUND_UP(
>>> -					(EXT2_INODES_PER_GROUP(fs->super) -
>>> -					group_free),
>>> -					EXT2_INODES_PER_BLOCK(fs->super));
>>> -
>>> -				blk = ext2fs_inode_table_loc(fs, group) +
>>> -				      used_blks;
>>> -				num = fs->inode_blocks_per_group -
>>> -				      used_blks;
>>> -				e2fsck_discard_blocks(ctx, manager, blk, num);
>>> -			}
>>> -
>>> +			/*
>>> +			 * If the last inode is free, we can discard it as well.
>>> +			 */
>>> +			if (inodes >= first_free)
>>> +				e2fsck_discard_inodes(ctx, group, first_free,
>>> +						      inodes - first_free + 1);
>>>  			/*
>>>  			 * If discard zeroes data and the group inode table
>>>  			 * was not zeroed yet, set itable as zeroed
>>>  			 */
>>>  			if ((ctx->options & E2F_OPT_DISCARD) &&
>>> -			    (io_channel_discard_zeroes_data(fs->io)) &&
>>> +			    io_channel_discard_zeroes_data(fs->io) &&
>>>  			    !(ext2fs_bg_flags_test(fs, group,
>>> -						  EXT2_BG_INODE_ZEROED))) {
>>> +						   EXT2_BG_INODE_ZEROED))) {
>>>  				ext2fs_bg_flags_set(fs, group,
>>>  						    EXT2_BG_INODE_ZEROED);
>>>  				ext2fs_group_desc_csum_set(fs, group);
>>>  			}
>>>  
>>> +			first_free = fs->super->s_inodes_per_group + 1;
>>> +			free_array[group] = group_free;
>>> +			dir_array[group] = dirs_count;
>>>  			group ++;
>>>  			inodes = 0;
>>>  			skip_group = 0;
>>
>>
> 

Re: [PATCH 2/4] e2fsck: Do not forget to discard last block group

[Eric Sandeen]

On 03/05/2012 01:49 AM, Lukas Czerner wrote:
> Previously when running e2fsck with '-E discard' argument the end of
> the last group has not been discarded. This patch fixes it so we
> always discard the end of the last group if needed.
> 
> This commit also removes unneeded argument from the
> e2fsck_discard_blocks(). Simultaneously the commit causes the block
> groups with BLOCK_UNINIT flag not to be discarded, which makes
> sense because we do not need to reclaim the space since so far
> there has not been written anything.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

As Phillip found, though, the part about any changes to BLOCK_UNINIT
behavior are pretty non-obvious, and could probably have used a more
verbose explanation in the changelog.

> ---
>  e2fsck/pass5.c |   24 ++++++++++++++++--------
>  1 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
> index ee73dd5..37bbac2 100644
> --- a/e2fsck/pass5.c
> +++ b/e2fsck/pass5.c
> @@ -74,8 +74,8 @@ void e2fsck_pass5(e2fsck_t ctx)
>  	print_resource_track(ctx, _("Pass 5"), &rtrack, ctx->fs->io);
>  }
>  
> -static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
> -				  blk64_t start, blk64_t count)
> +static void e2fsck_discard_blocks(e2fsck_t ctx, blk64_t start,
> +				  blk64_t count)
>  {
>  	ext2_filsys fs = ctx->fs;
>  
> @@ -85,7 +85,7 @@ static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
>  	 * not enough to fix the problem, hence it is not safe to run discard
>  	 * in this case.
>  	 */
> -	if (ext2fs_test_changed(ctx->fs))
> +	if (ext2fs_test_changed(fs))
>  		ctx->options &= ~E2F_OPT_DISCARD;
>  
>  	if (!(ctx->options & E2F_OPT_NO) &&
> @@ -137,7 +137,7 @@ static void e2fsck_discard_inodes(e2fsck_t ctx, int group,
>  	num = count / EXT2_INODES_PER_BLOCK(fs->super);
>  
>  	if (num > 0)
> -		e2fsck_discard_blocks(ctx, fs->io->manager, blk, num);
> +		e2fsck_discard_blocks(ctx, blk, num);
>  }
>  
>  #define NO_BLK ((blk64_t) -1)
> @@ -377,16 +377,24 @@ redo_counts:
>  			free_blocks++;
>  			if (first_free > i)
>  				first_free = i;
> -		} else {
> -			if (i > first_free)
> -				e2fsck_discard_blocks(ctx, manager, first_free,
> -						      (i - first_free));
> +		} else if (i > first_free) {
> +			e2fsck_discard_blocks(ctx, first_free,
> +					      (i - first_free));
>  			first_free = ext2fs_blocks_count(fs->super);
>  		}
>  		blocks ++;
>  		if ((blocks == fs->super->s_clusters_per_group) ||
>  		    (EXT2FS_B2C(fs, i) ==
>  		     EXT2FS_B2C(fs, ext2fs_blocks_count(fs->super)-1))) {
> +			/*
> +			 * If the last block of this group is free, then we can
> +			 * discard it as well.
> +			 */
> +			if (i >= first_free)
> +				e2fsck_discard_blocks(ctx, first_free,
> +						      (i - first_free) + 1);
> +			first_free = ext2fs_blocks_count(fs->super);
> +
>  			free_array[group] = group_free;
>  			group ++;
>  			blocks = 0;

Re: [PATCH 3/4] e2fsck: Do not discard when in read only mode

[Eric Sandeen]

On 03/05/2012 01:49 AM, Lukas Czerner wrote:
> When argument '-n' was specified and should run in read-only mode, we
> should not attempt to discard anything. In order to do that we have to
> check for E2F_OPT_NO flag and clear E2F_OPT_DISCARD flag if E2F_OPT_NO
> is set.
> 
> This commit fixes the problem when we would mark inode tables as zeroed
> (EXT2_BG_INODE_ZEROED) even when e2fsck is running in read-only mode. We
> also move the check for E2F_OPT_NO so we can clear E2F_OPT_DISCARD as
> early as possible.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Probably no need to test (ctx->options & E2F_OPT_DISCARD) before
clearing it, but no big deal.

> ---
>  e2fsck/pass5.c |    5 ++---
>  e2fsck/unix.c  |    5 +++++
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
> index 37bbac2..741e6dd 100644
> --- a/e2fsck/pass5.c
> +++ b/e2fsck/pass5.c
> @@ -88,8 +88,7 @@ static void e2fsck_discard_blocks(e2fsck_t ctx, blk64_t start,
>  	if (ext2fs_test_changed(fs))
>  		ctx->options &= ~E2F_OPT_DISCARD;
>  
> -	if (!(ctx->options & E2F_OPT_NO) &&
> -	    (ctx->options & E2F_OPT_DISCARD) &&
> +	if ((ctx->options & E2F_OPT_DISCARD) &&
>  	    (io_channel_discard(fs->io, start, count)))
>  		ctx->options &= ~E2F_OPT_DISCARD;
>  }
> @@ -117,7 +116,7 @@ static void e2fsck_discard_inodes(e2fsck_t ctx, int group,
>  		ctx->options &= ~E2F_OPT_DISCARD;
>  	}
>  
> -	if ((ctx->options & E2F_OPT_NO) || !(ctx->options & E2F_OPT_DISCARD))
> +	if (!(ctx->options & E2F_OPT_DISCARD))
>  		return;
>  
>  	/*
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index 6f97b0f..b31a1e3 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -903,6 +903,11 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
>  	profile_set_syntax_err_cb(syntax_err_report);
>  	profile_init(config_fn, &ctx->profile);
>  
> +	/* Turn off discard in read-only mode */
> +	if ((ctx->options & E2F_OPT_NO) &&
> +	    (ctx->options & E2F_OPT_DISCARD))
> +		ctx->options &= ~E2F_OPT_DISCARD;
> +
>  	if (flush) {
>  		fd = open(ctx->filesystem_name, O_RDONLY, 0);
>  		if (fd < 0) {

Re: [PATCH 4/4] e2fsck: Do not discard itable if discard doen't zero data

[Eric Sandeen]

On 03/05/2012 01:49 AM, Lukas Czerner wrote:
> We do not want to discard inode table if the underlying device does not
> return zeros when reading non-provisioned blocks. The reason is that if
> the inode table is not zeroed yet, then discard would not help us since
> we would have to zero it anyway. In the case that inode table was
> already zeroed, then the discard would cause subsequent reads to contain
> non-deterministic data so we would not be able to assume that the inode
> table was zeroed and we would need to zero it again, which does not
> really make sense.
> 
> This commit adds check to prevent inode table from being discarded if
> the discard does not zero data.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

seems fine

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  e2fsck/pass5.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
> index 741e6dd..9e63037 100644
> --- a/e2fsck/pass5.c
> +++ b/e2fsck/pass5.c
> @@ -116,7 +116,18 @@ static void e2fsck_discard_inodes(e2fsck_t ctx, int group,
>  		ctx->options &= ~E2F_OPT_DISCARD;
>  	}
>  
> -	if (!(ctx->options & E2F_OPT_DISCARD))
> +	/*
> +	 * Do not attempt to discard if E2F_OPT_DISCARD is not set. And also
> +	 * skip the discard on this group if discard does not zero data.
> +	 * The reason is that if the inode table is not zeroed discard would
> +	 * no help us since we need to zero it anyway, or if the inode table
> +	 * is zeroed then the read after discard would not be deterministic
> +	 * anyway and we would not be able to assume that this inode table
> +	 * was zeroed anymore so we would have to zero it again, which does
> +	 * not really make sense.
> +	 */
> +	if (!(ctx->options & E2F_OPT_DISCARD) ||
> +	    !io_channel_discard_zeroes_data(fs->io))
>  		return;
>  
>  	/*

Re: [PATCH 2/4] e2fsck: Do not forget to discard last block group

[Ted Ts'o]

On Mon, Mar 05, 2012 at 08:49:34AM +0100, Lukas Czerner wrote:
> Previously when running e2fsck with '-E discard' argument the end of
> the last group has not been discarded. This patch fixes it so we
> always discard the end of the last group if needed.
> 
> This commit also removes unneeded argument from the
> e2fsck_discard_blocks(). Simultaneously the commit causes the block
> groups with BLOCK_UNINIT flag not to be discarded, which makes
> sense because we do not need to reclaim the space since so far
> there has not been written anything.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

I split this into 3 patches, just to make it easier to reivew, and to
demonstrate the patch granularity I prefer.  The code paths in
check_block_bitmaps() and check_inode_bitmaps() *are* somewhat hard to
understand, partially because they are performance-critical.  So it's
especially important to separate out clean up patches from things that
actually change things.

>  		if ((blocks == fs->super->s_clusters_per_group) ||
>  		    (EXT2FS_B2C(fs, i) ==
>  		     EXT2FS_B2C(fs, ext2fs_blocks_count(fs->super)-1))) {
> +			/*
> +			 * If the last block of this group is free, then we can
> +			 * discard it as well.
> +			 */
> +			if (i >= first_free)
> +				e2fsck_discard_blocks(ctx, first_free,
> +						      (i - first_free) + 1);

This is buggy, because you're assuming the last block in the group is
free.  It might not be so, and if so, we would end up discarding valid
data.  The conditional needs to read:

			if (!bitmap && i >= first_free)
> +				e2fsck_discard_blocks(ctx, first_free,
> +						      (i - first_free) + 1);

In the case where bitmap is set, then any discards (if necessary) will
have been done already.

> +			first_free = ext2fs_blocks_count(fs->super);

I don't believe this is necessary, since we're not going to refer to
first_free again, are we?

			   	       	 - Ted

Re: [PATCH 1/4] e2fsck: Discard only unused parts of inode table

[Ted Ts'o]

On Mon, Mar 05, 2012 at 08:49:33AM +0100, Lukas Czerner wrote:
> @@ -435,6 +481,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
>  	int		skip_group = 0;
>  	int		redo_flag = 0;
>  	io_manager	manager = ctx->fs->io->manager;
> +	unsigned long long	first_free = fs->super->s_inodes_per_group + 1;

This doesn't need to be an unsigned long long; an "int" will do just
as well.

> +			/*
> +			 * If the last inode is free, we can discard it as well.
> +			 */
> +			if (inodes >= first_free)
> +				e2fsck_discard_inodes(ctx, group, first_free,
> +						      inodes - first_free + 1);

There's no guarantee the last inode is free here.  This needs to read:

			if (!bitmap && inodes >= first_free)

I'll fix this up in my version of the patches.

					- Ted

[PATCH 1/3] e2fsck: remove last argument from e2fsck_discard_blocks()

[Theodore Ts'o]

From: Lukas Czerner <lczerner@redhat.com>

Simple code cleanup.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 e2fsck/pass5.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index 9a6a4b0..ca438cb 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -74,8 +74,8 @@ void e2fsck_pass5(e2fsck_t ctx)
 	print_resource_track(ctx, _("Pass 5"), &rtrack, ctx->fs->io);
 }
 
-static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
-				  blk64_t start, blk64_t count)
+static void e2fsck_discard_blocks(e2fsck_t ctx, blk64_t start,
+				  blk64_t count)
 {
 	ext2_filsys fs = ctx->fs;
 
@@ -85,7 +85,7 @@ static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
 	 * not enough to fix the problem, hence it is not safe to run discard
 	 * in this case.
 	 */
-	if (ext2fs_test_changed(ctx->fs))
+	if (ext2fs_test_changed(fs))
 		ctx->options &= ~E2F_OPT_DISCARD;
 
 	if (!(ctx->options & E2F_OPT_NO) &&
@@ -137,7 +137,7 @@ static void e2fsck_discard_inodes(e2fsck_t ctx, int group,
 	num = count / EXT2_INODES_PER_BLOCK(fs->super);
 
 	if (num > 0)
-		e2fsck_discard_blocks(ctx, fs->io->manager, blk, num);
+		e2fsck_discard_blocks(ctx, blk, num);
 }
 
 #define NO_BLK ((blk64_t) -1)
@@ -379,7 +379,7 @@ redo_counts:
 				first_free = i;
 		} else {
 			if (i > first_free)
-				e2fsck_discard_blocks(ctx, manager, first_free,
+				e2fsck_discard_blocks(ctx, first_free,
 						      (i - first_free));
 			first_free = ext2fs_blocks_count(fs->super);
 		}
-- 
1.7.9.107.g97f9a

[PATCH 2/3] e2fsck: do not forget to discard last block group

[Theodore Ts'o]

From: Lukas Czerner <lczerner@redhat.com>

Previously when running e2fsck with '-E discard' argument the end of
the last group has not been discarded. This patch fixes it so we
always discard the end of the last group if needed.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 e2fsck/pass5.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index ca438cb..e25f080 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -387,6 +387,15 @@ redo_counts:
 		if ((blocks == fs->super->s_clusters_per_group) ||
 		    (EXT2FS_B2C(fs, i) ==
 		     EXT2FS_B2C(fs, ext2fs_blocks_count(fs->super)-1))) {
+			/*
+			 * If the last block of this group is free, then we can
+			 * discard it as well.
+			 */
+			if (!bitmap && i >= first_free)
+				e2fsck_discard_blocks(ctx, first_free,
+						      (i - first_free) + 1);
+			first_free = ext2fs_blocks_count(fs->super);
+
 			free_array[group] = group_free;
 			group ++;
 			blocks = 0;
-- 
1.7.9.107.g97f9a

[PATCH 3/3] e2fsck: optimize CPU usage in check_{block,inode}_bitmaps()

[Theodore Ts'o]

The boolean expression (!skip_group || csum_flag) is always true,
since if csum_flag is FALSE, skip_group must also be FALSE.  Hence, we
can just remove the expression from the conditional altogether, thus
simplifying the code and making it easier to read/understand.

Also, in the case where the bit is set in the bitmap, there's no point
repeatedly setting first_free to be ext2fs_block_count(fs->super).

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 e2fsck/pass5.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index e25f080..c121d92 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -372,15 +372,14 @@ redo_counts:
 		ctx->options &= ~E2F_OPT_DISCARD;
 
 	do_counts:
-		if (!bitmap && (!skip_group || csum_flag)) {
+		if (!bitmap) {
 			group_free++;
 			free_blocks++;
 			if (first_free > i)
 				first_free = i;
-		} else {
-			if (i > first_free)
-				e2fsck_discard_blocks(ctx, first_free,
-						      (i - first_free));
+		} else if (i > first_free) {
+			e2fsck_discard_blocks(ctx, first_free,
+					      (i - first_free));
 			first_free = ext2fs_blocks_count(fs->super);
 		}
 		blocks ++;
@@ -627,7 +626,7 @@ do_counts:
 						      inodes - first_free);
 				first_free = fs->super->s_inodes_per_group + 1;
 			}
-		} else if (!skip_group || csum_flag) {
+		} else {
 			group_free++;
 			free_inodes++;
 			if (first_free > inodes)
-- 
1.7.9.107.g97f9a

Re: [PATCH 3/4] e2fsck: Do not discard when in read only mode

[Ted Ts'o]

On Mon, Mar 05, 2012 at 08:49:35AM +0100, Lukas Czerner wrote:
> When argument '-n' was specified and should run in read-only mode, we
> should not attempt to discard anything. In order to do that we have to
> check for E2F_OPT_NO flag and clear E2F_OPT_DISCARD flag if E2F_OPT_NO
> is set.
> 
> This commit fixes the problem when we would mark inode tables as zeroed
> (EXT2_BG_INODE_ZEROED) even when e2fsck is running in read-only mode. We
> also move the check for E2F_OPT_NO so we can clear E2F_OPT_DISCARD as
> early as possible.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Applied, thanks.

						- Ted

Re: [PATCH 4/4] e2fsck: Do not discard itable if discard doen't zero data

[Ted Ts'o]

On Mon, Mar 05, 2012 at 08:49:36AM +0100, Lukas Czerner wrote:
> We do not want to discard inode table if the underlying device does not
> return zeros when reading non-provisioned blocks. The reason is that if
> the inode table is not zeroed yet, then discard would not help us since
> we would have to zero it anyway. In the case that inode table was
> already zeroed, then the discard would cause subsequent reads to contain
> non-deterministic data so we would not be able to assume that the inode
> table was zeroed and we would need to zero it again, which does not
> really make sense.
> 
> This commit adds check to prevent inode table from being discarded if
> the discard does not zero data.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Applied, thanks.

				- Ted

Re: [PATCH 1/4] e2fsck: Discard only unused parts of inode table

[Lukas Czerner]

On Sun, 11 Mar 2012, Ted Ts'o wrote:

> On Mon, Mar 05, 2012 at 08:49:33AM +0100, Lukas Czerner wrote:
> > @@ -435,6 +481,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
> >  	int		skip_group = 0;
> >  	int		redo_flag = 0;
> >  	io_manager	manager = ctx->fs->io->manager;
> > +	unsigned long long	first_free = fs->super->s_inodes_per_group + 1;
> 
> This doesn't need to be an unsigned long long; an "int" will do just
> as well.
> 
> > +			/*
> > +			 * If the last inode is free, we can discard it as well.
> > +			 */
> > +			if (inodes >= first_free)
> > +				e2fsck_discard_inodes(ctx, group, first_free,
> > +						      inodes - first_free + 1);
> 
> There's no guarantee the last inode is free here.  This needs to read:
> 
> 			if (!bitmap && inodes >= first_free)

Hi Ted,

if the last inode is not free then:

	first_free = fs->super->s_inodes_per_group + 1;

so the condition

	if (inodes >= first_free)

would be false. Otherwise it is guaranteed that the last inode is free. But
I agree that your change makes it more obvious.

Thanks!
-Lukas

> 
> I'll fix this up in my version of the patches.
> 
> 					- Ted
> 

Re: [PATCH 1/3] e2fsck: remove last argument from e2fsck_discard_blocks()

[Lukas Czerner]

On Sun, 11 Mar 2012, Theodore Ts'o wrote:

> From: Lukas Czerner <lczerner@redhat.com>
> 
> Simple code cleanup.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

Looks good. Thanks!
-Lukas

> ---
>  e2fsck/pass5.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
> index 9a6a4b0..ca438cb 100644
> --- a/e2fsck/pass5.c
> +++ b/e2fsck/pass5.c
> @@ -74,8 +74,8 @@ void e2fsck_pass5(e2fsck_t ctx)
>  	print_resource_track(ctx, _("Pass 5"), &rtrack, ctx->fs->io);
>  }
>  
> -static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
> -				  blk64_t start, blk64_t count)
> +static void e2fsck_discard_blocks(e2fsck_t ctx, blk64_t start,
> +				  blk64_t count)
>  {
>  	ext2_filsys fs = ctx->fs;
>  
> @@ -85,7 +85,7 @@ static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
>  	 * not enough to fix the problem, hence it is not safe to run discard
>  	 * in this case.
>  	 */
> -	if (ext2fs_test_changed(ctx->fs))
> +	if (ext2fs_test_changed(fs))
>  		ctx->options &= ~E2F_OPT_DISCARD;
>  
>  	if (!(ctx->options & E2F_OPT_NO) &&
> @@ -137,7 +137,7 @@ static void e2fsck_discard_inodes(e2fsck_t ctx, int group,
>  	num = count / EXT2_INODES_PER_BLOCK(fs->super);
>  
>  	if (num > 0)
> -		e2fsck_discard_blocks(ctx, fs->io->manager, blk, num);
> +		e2fsck_discard_blocks(ctx, blk, num);
>  }
>  
>  #define NO_BLK ((blk64_t) -1)
> @@ -379,7 +379,7 @@ redo_counts:
>  				first_free = i;
>  		} else {
>  			if (i > first_free)
> -				e2fsck_discard_blocks(ctx, manager, first_free,
> +				e2fsck_discard_blocks(ctx, first_free,
>  						      (i - first_free));
>  			first_free = ext2fs_blocks_count(fs->super);
>  		}
> 

-- 

Re: [PATCH 2/3] e2fsck: do not forget to discard last block group

[Lukas Czerner]

On Sun, 11 Mar 2012, Theodore Ts'o wrote:

> From: Lukas Czerner <lczerner@redhat.com>
> 
> Previously when running e2fsck with '-E discard' argument the end of
> the last group has not been discarded. This patch fixes it so we
> always discard the end of the last group if needed.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Looks good. Thanks for splitting the patch, it makes sense.

-Lukas

> ---
>  e2fsck/pass5.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
> index ca438cb..e25f080 100644
> --- a/e2fsck/pass5.c
> +++ b/e2fsck/pass5.c
> @@ -387,6 +387,15 @@ redo_counts:
>  		if ((blocks == fs->super->s_clusters_per_group) ||
>  		    (EXT2FS_B2C(fs, i) ==
>  		     EXT2FS_B2C(fs, ext2fs_blocks_count(fs->super)-1))) {
> +			/*
> +			 * If the last block of this group is free, then we can
> +			 * discard it as well.
> +			 */
> +			if (!bitmap && i >= first_free)
> +				e2fsck_discard_blocks(ctx, first_free,
> +						      (i - first_free) + 1);
> +			first_free = ext2fs_blocks_count(fs->super);
> +
>  			free_array[group] = group_free;
>  			group ++;
>  			blocks = 0;
> 

-- 

Re: [PATCH 3/3] e2fsck: optimize CPU usage in check_{block,inode}_bitmaps()

[Lukas Czerner]

On Sun, 11 Mar 2012, Theodore Ts'o wrote:

> The boolean expression (!skip_group || csum_flag) is always true,
> since if csum_flag is FALSE, skip_group must also be FALSE.  Hence, we
> can just remove the expression from the conditional altogether, thus
> simplifying the code and making it easier to read/understand.
> 
> Also, in the case where the bit is set in the bitmap, there's no point
> repeatedly setting first_free to be ext2fs_block_count(fs->super).
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

Looks good, thanks!
-Lukas

> ---
>  e2fsck/pass5.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
> index e25f080..c121d92 100644
> --- a/e2fsck/pass5.c
> +++ b/e2fsck/pass5.c
> @@ -372,15 +372,14 @@ redo_counts:
>  		ctx->options &= ~E2F_OPT_DISCARD;
>  
>  	do_counts:
> -		if (!bitmap && (!skip_group || csum_flag)) {
> +		if (!bitmap) {
>  			group_free++;
>  			free_blocks++;
>  			if (first_free > i)
>  				first_free = i;
> -		} else {
> -			if (i > first_free)
> -				e2fsck_discard_blocks(ctx, first_free,
> -						      (i - first_free));
> +		} else if (i > first_free) {
> +			e2fsck_discard_blocks(ctx, first_free,
> +					      (i - first_free));
>  			first_free = ext2fs_blocks_count(fs->super);
>  		}
>  		blocks ++;
> @@ -627,7 +626,7 @@ do_counts:
>  						      inodes - first_free);
>  				first_free = fs->super->s_inodes_per_group + 1;
>  			}
> -		} else if (!skip_group || csum_flag) {
> +		} else {
>  			group_free++;
>  			free_inodes++;
>  			if (first_free > inodes)
> 

--