Patchwork [v1,19/22] e2fsck: check inline_data in pass3

login
register
mail settings
Submitter Zheng Liu
Date Aug. 2, 2013, 9:49 a.m.
Message ID <1375436989-18948-20-git-send-email-wenqing.lz@taobao.com>
Download mbox | patch
Permalink /patch/264270/
State Superseded
Headers show

Comments

Zheng Liu - Aug. 2, 2013, 9:49 a.m.
From: Zheng Liu <wenqing.lz@taobao.com>

In e2fsck_expand_directory() we don't handle a dir with inline data
because when this function is called the directory inode shouldn't
contains inline data.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
---
 e2fsck/pass3.c  |   12 ++++++++++++
 e2fsck/rehash.c |    3 ++-
 2 files changed, 14 insertions(+), 1 deletion(-)
Darrick J. Wong - Oct. 12, 2013, 12:54 a.m.
On Fri, Aug 02, 2013 at 05:49:46PM +0800, Zheng Liu wrote:
> From: Zheng Liu <wenqing.lz@taobao.com>
> 
> In e2fsck_expand_directory() we don't handle a dir with inline data
> because when this function is called the directory inode shouldn't
> contains inline data.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> ---
>  e2fsck/pass3.c  |   12 ++++++++++++
>  e2fsck/rehash.c |    3 ++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/e2fsck/pass3.c b/e2fsck/pass3.c
> index a379e9b..5052345 100644
> --- a/e2fsck/pass3.c
> +++ b/e2fsck/pass3.c
> @@ -787,6 +787,18 @@ errcode_t e2fsck_expand_directory(e2fsck_t ctx, ext2_ino_t dir,
>  	es.ctx = ctx;
>  	es.dir = dir;
>  
> +	/*
> +	 * 'lost+found' dir shouldn't contains inline data.  So we
> +	 * need to clear this flag.
> +	 */
> +	if (ext2fs_inode_has_inline_data(fs, dir)) {
> +		retval = ext2fs_read_inode(fs, dir, &inode);
> +		if (retval)
> +			return retval;
> +		inode.i_flags &= ~EXT4_INLINE_DATA_FL;
> +		e2fsck_write_inode(ctx, dir, &inode, "clear inline_data flag");
> +	}
> +
>  	retval = ext2fs_block_iterate3(fs, dir, BLOCK_FLAG_APPEND,
>  				       0, expand_dir_proc, &es);

Are you saying that lost+found can have inline_data set yet i_blocks is
actually a block map/extent head?  Or are we supposed to zero i_blocks?

If we clear EXT4_INLINE_DATA_FL and then try to iterate blocks, are we setting
ourselves up to read (formerly inline) dirents as a block map and iterate it?

Shouldn't we care if the inode write fails?

--D

>  
> diff --git a/e2fsck/rehash.c b/e2fsck/rehash.c
> index 5592e3f..82aeddd 100644
> --- a/e2fsck/rehash.c
> +++ b/e2fsck/rehash.c
> @@ -937,7 +937,8 @@ void e2fsck_rehash_directories(e2fsck_t ctx)
>  #if 0
>  		fix_problem(ctx, PR_3A_OPTIMIZE_DIR, &pctx);
>  #endif
> -		pctx.errcode = e2fsck_rehash_dir(ctx, ino);
> +		if (!ext2fs_inode_has_inline_data(ctx->fs, ino))
> +			pctx.errcode = e2fsck_rehash_dir(ctx, ino);
>  		if (pctx.errcode) {
>  			end_problem_latch(ctx, PR_LATCH_OPTIMIZE_DIR);
>  			fix_problem(ctx, PR_3A_OPTIMIZE_DIR_ERR, &pctx);
> -- 
> 1.7.9.7
> 
> --
> 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
--
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
Zheng Liu - Oct. 12, 2013, 9:06 a.m.
On Fri, Oct 11, 2013 at 05:54:10PM -0700, Darrick J. Wong wrote:
> On Fri, Aug 02, 2013 at 05:49:46PM +0800, Zheng Liu wrote:
> > From: Zheng Liu <wenqing.lz@taobao.com>
> > 
> > In e2fsck_expand_directory() we don't handle a dir with inline data
> > because when this function is called the directory inode shouldn't
> > contains inline data.
> > 
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> > ---
> >  e2fsck/pass3.c  |   12 ++++++++++++
> >  e2fsck/rehash.c |    3 ++-
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/e2fsck/pass3.c b/e2fsck/pass3.c
> > index a379e9b..5052345 100644
> > --- a/e2fsck/pass3.c
> > +++ b/e2fsck/pass3.c
> > @@ -787,6 +787,18 @@ errcode_t e2fsck_expand_directory(e2fsck_t ctx, ext2_ino_t dir,
> >  	es.ctx = ctx;
> >  	es.dir = dir;
> >  
> > +	/*
> > +	 * 'lost+found' dir shouldn't contains inline data.  So we
> > +	 * need to clear this flag.
> > +	 */
> > +	if (ext2fs_inode_has_inline_data(fs, dir)) {
> > +		retval = ext2fs_read_inode(fs, dir, &inode);
> > +		if (retval)
> > +			return retval;
> > +		inode.i_flags &= ~EXT4_INLINE_DATA_FL;
> > +		e2fsck_write_inode(ctx, dir, &inode, "clear inline_data flag");
> > +	}
> > +
> >  	retval = ext2fs_block_iterate3(fs, dir, BLOCK_FLAG_APPEND,
> >  				       0, expand_dir_proc, &es);
> 
> Are you saying that lost+found can have inline_data set yet i_blocks is
> actually a block map/extent head?  Or are we supposed to zero i_blocks?
> 
> If we clear EXT4_INLINE_DATA_FL and then try to iterate blocks, are we setting
> ourselves up to read (formerly inline) dirents as a block map and iterate it?
> 
> Shouldn't we care if the inode write fails?

lost+found dir shouldn't have inline_data flag because this is a special
directory that it is preallocated some blocks when it is created because
we need to avoid to allocate some blocks for it when we check a file
system using e2fsck.  So we need to clear inline_data flag if this dir
has this flag.

                                                - Zheng
--
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
Darrick J. Wong - Oct. 12, 2013, 9:09 a.m.
On Sat, Oct 12, 2013 at 05:06:35PM +0800, Zheng Liu wrote:
> On Fri, Oct 11, 2013 at 05:54:10PM -0700, Darrick J. Wong wrote:
> > On Fri, Aug 02, 2013 at 05:49:46PM +0800, Zheng Liu wrote:
> > > From: Zheng Liu <wenqing.lz@taobao.com>
> > > 
> > > In e2fsck_expand_directory() we don't handle a dir with inline data
> > > because when this function is called the directory inode shouldn't
> > > contains inline data.
> > > 
> > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> > > ---
> > >  e2fsck/pass3.c  |   12 ++++++++++++
> > >  e2fsck/rehash.c |    3 ++-
> > >  2 files changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/e2fsck/pass3.c b/e2fsck/pass3.c
> > > index a379e9b..5052345 100644
> > > --- a/e2fsck/pass3.c
> > > +++ b/e2fsck/pass3.c
> > > @@ -787,6 +787,18 @@ errcode_t e2fsck_expand_directory(e2fsck_t ctx, ext2_ino_t dir,
> > >  	es.ctx = ctx;
> > >  	es.dir = dir;
> > >  
> > > +	/*
> > > +	 * 'lost+found' dir shouldn't contains inline data.  So we
> > > +	 * need to clear this flag.
> > > +	 */
> > > +	if (ext2fs_inode_has_inline_data(fs, dir)) {
> > > +		retval = ext2fs_read_inode(fs, dir, &inode);
> > > +		if (retval)
> > > +			return retval;
> > > +		inode.i_flags &= ~EXT4_INLINE_DATA_FL;
> > > +		e2fsck_write_inode(ctx, dir, &inode, "clear inline_data flag");
> > > +	}
> > > +
> > >  	retval = ext2fs_block_iterate3(fs, dir, BLOCK_FLAG_APPEND,
> > >  				       0, expand_dir_proc, &es);
> > 
> > Are you saying that lost+found can have inline_data set yet i_blocks is
> > actually a block map/extent head?  Or are we supposed to zero i_blocks?
> > 
> > If we clear EXT4_INLINE_DATA_FL and then try to iterate blocks, are we setting
> > ourselves up to read (formerly inline) dirents as a block map and iterate it?
> > 
> > Shouldn't we care if the inode write fails?
> 
> lost+found dir shouldn't have inline_data flag because this is a special
> directory that it is preallocated some blocks when it is created because
> we need to avoid to allocate some blocks for it when we check a file
> system using e2fsck.  So we need to clear inline_data flag if this dir
> has this flag.

How does get that flag in the first place?

--D
> 
>                                                 - Zheng
--
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
Zheng Liu - Oct. 12, 2013, 9:17 a.m.
On Sat, Oct 12, 2013 at 02:09:35AM -0700, Darrick J. Wong wrote:
> On Sat, Oct 12, 2013 at 05:06:35PM +0800, Zheng Liu wrote:
> > On Fri, Oct 11, 2013 at 05:54:10PM -0700, Darrick J. Wong wrote:
> > > On Fri, Aug 02, 2013 at 05:49:46PM +0800, Zheng Liu wrote:
> > > > From: Zheng Liu <wenqing.lz@taobao.com>
> > > > 
> > > > In e2fsck_expand_directory() we don't handle a dir with inline data
> > > > because when this function is called the directory inode shouldn't
> > > > contains inline data.
> > > > 
> > > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> > > > ---
> > > >  e2fsck/pass3.c  |   12 ++++++++++++
> > > >  e2fsck/rehash.c |    3 ++-
> > > >  2 files changed, 14 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/e2fsck/pass3.c b/e2fsck/pass3.c
> > > > index a379e9b..5052345 100644
> > > > --- a/e2fsck/pass3.c
> > > > +++ b/e2fsck/pass3.c
> > > > @@ -787,6 +787,18 @@ errcode_t e2fsck_expand_directory(e2fsck_t ctx, ext2_ino_t dir,
> > > >  	es.ctx = ctx;
> > > >  	es.dir = dir;
> > > >  
> > > > +	/*
> > > > +	 * 'lost+found' dir shouldn't contains inline data.  So we
> > > > +	 * need to clear this flag.
> > > > +	 */
> > > > +	if (ext2fs_inode_has_inline_data(fs, dir)) {
> > > > +		retval = ext2fs_read_inode(fs, dir, &inode);
> > > > +		if (retval)
> > > > +			return retval;
> > > > +		inode.i_flags &= ~EXT4_INLINE_DATA_FL;
> > > > +		e2fsck_write_inode(ctx, dir, &inode, "clear inline_data flag");
> > > > +	}
> > > > +
> > > >  	retval = ext2fs_block_iterate3(fs, dir, BLOCK_FLAG_APPEND,
> > > >  				       0, expand_dir_proc, &es);
> > > 
> > > Are you saying that lost+found can have inline_data set yet i_blocks is
> > > actually a block map/extent head?  Or are we supposed to zero i_blocks?
> > > 
> > > If we clear EXT4_INLINE_DATA_FL and then try to iterate blocks, are we setting
> > > ourselves up to read (formerly inline) dirents as a block map and iterate it?
> > > 
> > > Shouldn't we care if the inode write fails?
> > 
> > lost+found dir shouldn't have inline_data flag because this is a special
> > directory that it is preallocated some blocks when it is created because
> > we need to avoid to allocate some blocks for it when we check a file
> > system using e2fsck.  So we need to clear inline_data flag if this dir
> > has this flag.
> 
> How does get that flag in the first place?

Technically, it shouldn't get this flag.  Think about it again, it seems
that we don't need to handle this because it couldn't happen.

                                                - Zheng
--
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
Darrick J. Wong - Oct. 12, 2013, 9:22 a.m.
On Sat, Oct 12, 2013 at 05:17:55PM +0800, Zheng Liu wrote:
> On Sat, Oct 12, 2013 at 02:09:35AM -0700, Darrick J. Wong wrote:
> > On Sat, Oct 12, 2013 at 05:06:35PM +0800, Zheng Liu wrote:
> > > On Fri, Oct 11, 2013 at 05:54:10PM -0700, Darrick J. Wong wrote:
> > > > On Fri, Aug 02, 2013 at 05:49:46PM +0800, Zheng Liu wrote:
> > > > > From: Zheng Liu <wenqing.lz@taobao.com>
> > > > > 
> > > > > In e2fsck_expand_directory() we don't handle a dir with inline data
> > > > > because when this function is called the directory inode shouldn't
> > > > > contains inline data.
> > > > > 
> > > > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > > > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> > > > > ---
> > > > >  e2fsck/pass3.c  |   12 ++++++++++++
> > > > >  e2fsck/rehash.c |    3 ++-
> > > > >  2 files changed, 14 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/e2fsck/pass3.c b/e2fsck/pass3.c
> > > > > index a379e9b..5052345 100644
> > > > > --- a/e2fsck/pass3.c
> > > > > +++ b/e2fsck/pass3.c
> > > > > @@ -787,6 +787,18 @@ errcode_t e2fsck_expand_directory(e2fsck_t ctx, ext2_ino_t dir,
> > > > >  	es.ctx = ctx;
> > > > >  	es.dir = dir;
> > > > >  
> > > > > +	/*
> > > > > +	 * 'lost+found' dir shouldn't contains inline data.  So we
> > > > > +	 * need to clear this flag.
> > > > > +	 */
> > > > > +	if (ext2fs_inode_has_inline_data(fs, dir)) {
> > > > > +		retval = ext2fs_read_inode(fs, dir, &inode);
> > > > > +		if (retval)
> > > > > +			return retval;
> > > > > +		inode.i_flags &= ~EXT4_INLINE_DATA_FL;
> > > > > +		e2fsck_write_inode(ctx, dir, &inode, "clear inline_data flag");
> > > > > +	}
> > > > > +
> > > > >  	retval = ext2fs_block_iterate3(fs, dir, BLOCK_FLAG_APPEND,
> > > > >  				       0, expand_dir_proc, &es);
> > > > 
> > > > Are you saying that lost+found can have inline_data set yet i_blocks is
> > > > actually a block map/extent head?  Or are we supposed to zero i_blocks?
> > > > 
> > > > If we clear EXT4_INLINE_DATA_FL and then try to iterate blocks, are we setting
> > > > ourselves up to read (formerly inline) dirents as a block map and iterate it?
> > > > 
> > > > Shouldn't we care if the inode write fails?
> > > 
> > > lost+found dir shouldn't have inline_data flag because this is a special
> > > directory that it is preallocated some blocks when it is created because
> > > we need to avoid to allocate some blocks for it when we check a file
> > > system using e2fsck.  So we need to clear inline_data flag if this dir
> > > has this flag.
> > 
> > How does get that flag in the first place?
> 
> Technically, it shouldn't get this flag.  Think about it again, it seems
> that we don't need to handle this because it couldn't happen.

Hmm.  Maybe there should be an explicit entry and fix_problem() for this
condition?  I think there's some function in e2fsck that specifically messes
with lost+found, but I'm going to bed before the parts of my brain that form
English sentences really crashes. 8)

Though I suppose since we're rehashing directories anyway, there might be
no point in pestering the user more.

--D
> 
>                                                 - Zheng
--
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
Zheng Liu - Oct. 12, 2013, 9:32 a.m.
On Sat, Oct 12, 2013 at 02:22:43AM -0700, Darrick J. Wong wrote:
> On Sat, Oct 12, 2013 at 05:17:55PM +0800, Zheng Liu wrote:
> > On Sat, Oct 12, 2013 at 02:09:35AM -0700, Darrick J. Wong wrote:
> > > On Sat, Oct 12, 2013 at 05:06:35PM +0800, Zheng Liu wrote:
> > > > On Fri, Oct 11, 2013 at 05:54:10PM -0700, Darrick J. Wong wrote:
> > > > > On Fri, Aug 02, 2013 at 05:49:46PM +0800, Zheng Liu wrote:
> > > > > > From: Zheng Liu <wenqing.lz@taobao.com>
> > > > > > 
> > > > > > In e2fsck_expand_directory() we don't handle a dir with inline data
> > > > > > because when this function is called the directory inode shouldn't
> > > > > > contains inline data.
> > > > > > 
> > > > > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > > > > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> > > > > > ---
> > > > > >  e2fsck/pass3.c  |   12 ++++++++++++
> > > > > >  e2fsck/rehash.c |    3 ++-
> > > > > >  2 files changed, 14 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/e2fsck/pass3.c b/e2fsck/pass3.c
> > > > > > index a379e9b..5052345 100644
> > > > > > --- a/e2fsck/pass3.c
> > > > > > +++ b/e2fsck/pass3.c
> > > > > > @@ -787,6 +787,18 @@ errcode_t e2fsck_expand_directory(e2fsck_t ctx, ext2_ino_t dir,
> > > > > >  	es.ctx = ctx;
> > > > > >  	es.dir = dir;
> > > > > >  
> > > > > > +	/*
> > > > > > +	 * 'lost+found' dir shouldn't contains inline data.  So we
> > > > > > +	 * need to clear this flag.
> > > > > > +	 */
> > > > > > +	if (ext2fs_inode_has_inline_data(fs, dir)) {
> > > > > > +		retval = ext2fs_read_inode(fs, dir, &inode);
> > > > > > +		if (retval)
> > > > > > +			return retval;
> > > > > > +		inode.i_flags &= ~EXT4_INLINE_DATA_FL;
> > > > > > +		e2fsck_write_inode(ctx, dir, &inode, "clear inline_data flag");
> > > > > > +	}
> > > > > > +
> > > > > >  	retval = ext2fs_block_iterate3(fs, dir, BLOCK_FLAG_APPEND,
> > > > > >  				       0, expand_dir_proc, &es);
> > > > > 
> > > > > Are you saying that lost+found can have inline_data set yet i_blocks is
> > > > > actually a block map/extent head?  Or are we supposed to zero i_blocks?
> > > > > 
> > > > > If we clear EXT4_INLINE_DATA_FL and then try to iterate blocks, are we setting
> > > > > ourselves up to read (formerly inline) dirents as a block map and iterate it?
> > > > > 
> > > > > Shouldn't we care if the inode write fails?
> > > > 
> > > > lost+found dir shouldn't have inline_data flag because this is a special
> > > > directory that it is preallocated some blocks when it is created because
> > > > we need to avoid to allocate some blocks for it when we check a file
> > > > system using e2fsck.  So we need to clear inline_data flag if this dir
> > > > has this flag.
> > > 
> > > How does get that flag in the first place?
> > 
> > Technically, it shouldn't get this flag.  Think about it again, it seems
> > that we don't need to handle this because it couldn't happen.
> 
> Hmm.  Maybe there should be an explicit entry and fix_problem() for this
> condition?  I think there's some function in e2fsck that specifically messes
> with lost+found, but I'm going to bed before the parts of my brain that form
> English sentences really crashes. 8)

Good idea.  Let me try it.

                                                - Zheng
--
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/pass3.c b/e2fsck/pass3.c
index a379e9b..5052345 100644
--- a/e2fsck/pass3.c
+++ b/e2fsck/pass3.c
@@ -787,6 +787,18 @@  errcode_t e2fsck_expand_directory(e2fsck_t ctx, ext2_ino_t dir,
 	es.ctx = ctx;
 	es.dir = dir;
 
+	/*
+	 * 'lost+found' dir shouldn't contains inline data.  So we
+	 * need to clear this flag.
+	 */
+	if (ext2fs_inode_has_inline_data(fs, dir)) {
+		retval = ext2fs_read_inode(fs, dir, &inode);
+		if (retval)
+			return retval;
+		inode.i_flags &= ~EXT4_INLINE_DATA_FL;
+		e2fsck_write_inode(ctx, dir, &inode, "clear inline_data flag");
+	}
+
 	retval = ext2fs_block_iterate3(fs, dir, BLOCK_FLAG_APPEND,
 				       0, expand_dir_proc, &es);
 
diff --git a/e2fsck/rehash.c b/e2fsck/rehash.c
index 5592e3f..82aeddd 100644
--- a/e2fsck/rehash.c
+++ b/e2fsck/rehash.c
@@ -937,7 +937,8 @@  void e2fsck_rehash_directories(e2fsck_t ctx)
 #if 0
 		fix_problem(ctx, PR_3A_OPTIMIZE_DIR, &pctx);
 #endif
-		pctx.errcode = e2fsck_rehash_dir(ctx, ino);
+		if (!ext2fs_inode_has_inline_data(ctx->fs, ino))
+			pctx.errcode = e2fsck_rehash_dir(ctx, ino);
 		if (pctx.errcode) {
 			end_problem_latch(ctx, PR_LATCH_OPTIMIZE_DIR);
 			fix_problem(ctx, PR_3A_OPTIMIZE_DIR_ERR, &pctx);