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

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

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;

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

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