| Message ID | 1375436989-18948-20-git-send-email-wenqing.lz@taobao.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
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
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
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
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
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
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);