linux-ext4 - Re: [PATCH 3/5] ext4: avoid running out of journal credits when appending to an inline file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <A87E4282-9935-40AA-8D88-10C15820D52C@dilger.ca>
Date:   Mon, 18 Jun 2018 12:21:04 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        wen.xu@...ech.edu
Subject: Re: [PATCH 3/5] ext4: avoid running out of journal credits when
 appending to an inline file

On Jun 17, 2018, at 9:22 PM, Theodore Ts'o <tytso@....edu> wrote:
> 
> Use a separate journal transaction if it turns out that we need to
> convert an inline file to use an data block.  Otherwise we could end
> up failing due to not having journal credits.

The callpath is a bit tricky here, but it seems that this is evicting the
inline data from the xattr, but only putting all of the data into the dirty
page for delalloc write?  That would imply that the inline data xattr removal
could commit in one transaction, but the delalloc write could be lost if the
following transaction fails (e.g. crash), losing the original data along with
any new data written to the file.

I think it is acceptable to lose the newly-written delalloc data if it wasn't
sync'd before the crash, but we shouldn't lose data that might have been
written at some earlier time.

Am I missing something here?

Cheers, Andreas

> https://bugzilla.kernel.org/show_bug.cgi?id=200071
> 
> Signed-off-by: Theodore Ts'o <tytso@....edu>
> ---
> fs/ext4/ext4.h   |  3 ---
> fs/ext4/inline.c | 38 +-------------------------------------
> fs/ext4/xattr.c  | 19 ++-----------------
> 3 files changed, 3 insertions(+), 57 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 856b6a54d82b..859d6433dcc1 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3013,9 +3013,6 @@ extern int ext4_inline_data_fiemap(struct inode *inode,
> struct iomap;
> extern int ext4_inline_data_iomap(struct inode *inode, struct iomap *iomap);
> 
> -extern int ext4_try_to_evict_inline_data(handle_t *handle,
> -					 struct inode *inode,
> -					 int needed);
> extern int ext4_inline_data_truncate(struct inode *inode, int *has_inline);
> 
> extern int ext4_convert_inline_data(struct inode *inode);
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index d79115d8d716..851bc552d849 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -887,11 +887,11 @@ int ext4_da_write_inline_data_begin(struct address_space *mapping,
> 	flags |= AOP_FLAG_NOFS;
> 
> 	if (ret == -ENOSPC) {
> +		ext4_journal_stop(handle);
> 		ret = ext4_da_convert_inline_data_to_extent(mapping,
> 							    inode,
> 							    flags,
> 							    fsdata);
> -		ext4_journal_stop(handle);
> 		if (ret == -ENOSPC &&
> 		    ext4_should_retry_alloc(inode->i_sb, &retries))
> 			goto retry_journal;
> @@ -1891,42 +1891,6 @@ int ext4_inline_data_fiemap(struct inode *inode,
> 	return (error < 0 ? error : 0);
> }
> 
> -/*
> - * Called during xattr set, and if we can sparse space 'needed',
> - * just create the extent tree evict the data to the outer block.
> - *
> - * We use jbd2 instead of page cache to move data to the 1st block
> - * so that the whole transaction can be committed as a whole and
> - * the data isn't lost because of the delayed page cache write.
> - */
> -int ext4_try_to_evict_inline_data(handle_t *handle,
> -				  struct inode *inode,
> -				  int needed)
> -{
> -	int error;
> -	struct ext4_xattr_entry *entry;
> -	struct ext4_inode *raw_inode;
> -	struct ext4_iloc iloc;
> -
> -	error = ext4_get_inode_loc(inode, &iloc);
> -	if (error)
> -		return error;
> -
> -	raw_inode = ext4_raw_inode(&iloc);
> -	entry = (struct ext4_xattr_entry *)((void *)raw_inode +
> -					    EXT4_I(inode)->i_inline_off);
> -	if (EXT4_XATTR_LEN(entry->e_name_len) +
> -	    EXT4_XATTR_SIZE(le32_to_cpu(entry->e_value_size)) < needed) {
> -		error = -ENOSPC;
> -		goto out;
> -	}
> -
> -	error = ext4_convert_inline_data_nolock(handle, inode, &iloc);
> -out:
> -	brelse(iloc.bh);
> -	return error;
> -}
> -
> int ext4_inline_data_truncate(struct inode *inode, int *has_inline)
> {
> 	handle_t *handle;
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 72377b77fbd7..723df14f4084 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2212,23 +2212,8 @@ int ext4_xattr_ibody_inline_set(handle_t *handle, struct inode *inode,
> 	if (EXT4_I(inode)->i_extra_isize == 0)
> 		return -ENOSPC;
> 	error = ext4_xattr_set_entry(i, s, handle, inode, false /* is_block */);
> -	if (error) {
> -		if (error == -ENOSPC &&
> -		    ext4_has_inline_data(inode)) {
> -			error = ext4_try_to_evict_inline_data(handle, inode,
> -					EXT4_XATTR_LEN(strlen(i->name) +
> -					EXT4_XATTR_SIZE(i->value_len)));
> -			if (error)
> -				return error;
> -			error = ext4_xattr_ibody_find(inode, i, is);
> -			if (error)
> -				return error;
> -			error = ext4_xattr_set_entry(i, s, handle, inode,
> -						     false /* is_block */);
> -		}
> -		if (error)
> -			return error;
> -	}
> +	if (error)
> +		return error;
> 	header = IHDR(inode, ext4_raw_inode(&is->iloc));
> 	if (!IS_LAST_ENTRY(s->first)) {
> 		header->h_magic = cpu_to_le32(EXT4_XATTR_MAGIC);
> --
> 2.18.0.rc0
> 


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)