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

login
register
mail settings
Submitter Lukas Czerner
Date March 5, 2012, 7:49 a.m.
Message ID <1330933776-2696-4-git-send-email-lczerner@redhat.com>
Download mbox | patch
Permalink /patch/144617/
State Accepted
Headers show

Comments

Lukas Czerner - March 5, 2012, 7:49 a.m.
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(-)
Eric Sandeen - March 5, 2012, 7:46 p.m.
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;
>  
>  	/*

--
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
Theodore Ts'o - March 11, 2012, 7:39 p.m.
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
--
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/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;
 
 	/*