| Message ID | 1424697396-17248-1-git-send-email-lczerner@redhat.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Mon 23-02-15 14:16:36, Lukas Czerner wrote: > Previously commit 14ece1028b3ed53ffec1b1213ffc6acaf79ad77c added a > support for for syncing parent directory of newly created inodes to make > sure that the inode is not lost after a power failure in no-journal > mode. > > However this does not work in majority of cases, namely: > - if the directory has inline data > - if the directory is already indexed > - if the directory already has at least one block and: > - the new entry fits into it > - or we've successfully converted it to indexed > > So in those cases we might lose the inode entirely even after fsync in > the no-journal mode. This also includes ext2 default mode obviously. > > I've noticed this while running xfstest generic/321 and even though the > test should fail (we need to run fsck after a crash in no-journal mode) > I could not find a newly created entries even when if it was fsynced > before. > > Fix this by adjusting the ext4_add_entry() successful exit paths to set > the inode EXT4_STATE_NEWENTRY so that fsync has the chance to fsync the > parent directory as well. > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > Cc: Frank Mayhar <fmayhar@google.com> Looks good. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/ext4/namei.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 2291923..25e1e03 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -1865,7 +1865,7 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, > struct inode *inode) > { > struct inode *dir = dentry->d_parent->d_inode; > - struct buffer_head *bh; > + struct buffer_head *bh = NULL; > struct ext4_dir_entry_2 *de; > struct ext4_dir_entry_tail *t; > struct super_block *sb; > @@ -1889,14 +1889,14 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, > return retval; > if (retval == 1) { > retval = 0; > - return retval; > + goto out; > } > } > > if (is_dx(dir)) { > retval = ext4_dx_add_entry(handle, dentry, inode); > if (!retval || (retval != ERR_BAD_DX_DIR)) > - return retval; > + goto out; > ext4_clear_inode_flag(dir, EXT4_INODE_INDEX); > dx_fallback++; > ext4_mark_inode_dirty(handle, dir); > @@ -1908,14 +1908,14 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, > return PTR_ERR(bh); > > retval = add_dirent_to_buf(handle, dentry, inode, NULL, bh); > - if (retval != -ENOSPC) { > - brelse(bh); > - return retval; > - } > + if (retval != -ENOSPC) > + goto out; > > if (blocks == 1 && !dx_fallback && > - EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX)) > - return make_indexed_dir(handle, dentry, inode, bh); > + EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX)) { > + retval = make_indexed_dir(handle, dentry, inode, bh); > + goto out; > + } > brelse(bh); > } > bh = ext4_append(handle, dir, &block); > @@ -1931,6 +1931,7 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, > } > > retval = add_dirent_to_buf(handle, dentry, inode, de, bh); > +out: > brelse(bh); > if (retval == 0) > ext4_set_inode_state(inode, EXT4_STATE_NEWENTRY); > -- > 1.8.3.1 > > -- > 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, Feb 23, 2015 at 02:16:36PM +0100, Lukas Czerner wrote: > Previously commit 14ece1028b3ed53ffec1b1213ffc6acaf79ad77c added a > support for for syncing parent directory of newly created inodes to make > sure that the inode is not lost after a power failure in no-journal > mode. > > However this does not work in majority of cases, namely: > - if the directory has inline data > - if the directory is already indexed > - if the directory already has at least one block and: > - the new entry fits into it > - or we've successfully converted it to indexed > > So in those cases we might lose the inode entirely even after fsync in > the no-journal mode. This also includes ext2 default mode obviously. > > I've noticed this while running xfstest generic/321 and even though the > test should fail (we need to run fsck after a crash in no-journal mode) > I could not find a newly created entries even when if it was fsynced > before. > > Fix this by adjusting the ext4_add_entry() successful exit paths to set > the inode EXT4_STATE_NEWENTRY so that fsync has the chance to fsync the > parent directory as well. > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > Cc: Frank Mayhar <fmayhar@google.com> Thanks, applied. - 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
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 2291923..25e1e03 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -1865,7 +1865,7 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, struct inode *inode) { struct inode *dir = dentry->d_parent->d_inode; - struct buffer_head *bh; + struct buffer_head *bh = NULL; struct ext4_dir_entry_2 *de; struct ext4_dir_entry_tail *t; struct super_block *sb; @@ -1889,14 +1889,14 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, return retval; if (retval == 1) { retval = 0; - return retval; + goto out; } } if (is_dx(dir)) { retval = ext4_dx_add_entry(handle, dentry, inode); if (!retval || (retval != ERR_BAD_DX_DIR)) - return retval; + goto out; ext4_clear_inode_flag(dir, EXT4_INODE_INDEX); dx_fallback++; ext4_mark_inode_dirty(handle, dir); @@ -1908,14 +1908,14 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, return PTR_ERR(bh); retval = add_dirent_to_buf(handle, dentry, inode, NULL, bh); - if (retval != -ENOSPC) { - brelse(bh); - return retval; - } + if (retval != -ENOSPC) + goto out; if (blocks == 1 && !dx_fallback && - EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX)) - return make_indexed_dir(handle, dentry, inode, bh); + EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX)) { + retval = make_indexed_dir(handle, dentry, inode, bh); + goto out; + } brelse(bh); } bh = ext4_append(handle, dir, &block); @@ -1931,6 +1931,7 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, } retval = add_dirent_to_buf(handle, dentry, inode, de, bh); +out: brelse(bh); if (retval == 0) ext4_set_inode_state(inode, EXT4_STATE_NEWENTRY);
Previously commit 14ece1028b3ed53ffec1b1213ffc6acaf79ad77c added a support for for syncing parent directory of newly created inodes to make sure that the inode is not lost after a power failure in no-journal mode. However this does not work in majority of cases, namely: - if the directory has inline data - if the directory is already indexed - if the directory already has at least one block and: - the new entry fits into it - or we've successfully converted it to indexed So in those cases we might lose the inode entirely even after fsync in the no-journal mode. This also includes ext2 default mode obviously. I've noticed this while running xfstest generic/321 and even though the test should fail (we need to run fsck after a crash in no-journal mode) I could not find a newly created entries even when if it was fsynced before. Fix this by adjusting the ext4_add_entry() successful exit paths to set the inode EXT4_STATE_NEWENTRY so that fsync has the chance to fsync the parent directory as well. Signed-off-by: Lukas Czerner <lczerner@redhat.com> Cc: Frank Mayhar <fmayhar@google.com> --- fs/ext4/namei.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)