Re: [PATCH] ext4: Make fsync to sync parent dir in no-journal for real this time

[Jan Kara <jack@suse.cz>]

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
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR