ext4: allow inode expansion for nojournal file systems

Runs of xfstest ext4/022 on nojournal file systems result in failures
because the inodes of some of its test files do not expand as expected.
The cause is a conditional in ext4_mark_inode_dirty() that prevents inode
expansion unless the test file system has a journal.  Remove this
unnecessary restriction.

Signed-off-by: Eric Whitney <enwlinux@gmail.com>
---
 fs/ext4/inode.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

On Oct 25, 2016, at 3:14 PM, Eric Whitney <enwlinux@gmail.com> wrote:
> 
> Runs of xfstest ext4/022 on nojournal file systems result in failures
> because the inodes of some of its test files do not expand as expected.
> The cause is a conditional in ext4_mark_inode_dirty() that prevents inode
> expansion unless the test file system has a journal.  Remove this
> unnecessary restriction.

I think the reason we required that the filesystem be journaled to expand
the inode reserved space is to ensure that the update was an all-or-nothing
approach.  If there are in-inode xattrs that need to be moved to an external
xattr block, and that block needs allocation and such, it is possible that
a nojournal filesystem could result in the xattrs being moved and the inode
is written (w/o the xattrs), but the xattr block is not written to disk
before a crash.

That said, this check could potentially be moved later in the xattr handling,
since most xattrs will stay within the inode (when growing only a few bytes),
which is always safe.  If moving xattrs to an external block, we could sync
write the xattr block before marking the inode dirty to ensure that the xattr
is not lost in a crash (though it may be duplicated, e2fsck should handle
that?).  However, that has a potentially large performance cost.

Cheers, Andreas

> 
> Signed-off-by: Eric Whitney <enwlinux@gmail.com>
> ---
> fs/ext4/inode.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 9c06472..260da4d 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5455,18 +5455,20 @@ int ext4_mark_inode_dirty(handle_t *handle, struct inode *inode)
> 	err = ext4_reserve_inode_write(handle, inode, &iloc);
> 	if (err)
> 		return err;
> -	if (ext4_handle_valid(handle) &&
> -	    EXT4_I(inode)->i_extra_isize < sbi->s_want_extra_isize &&
> +	if (EXT4_I(inode)->i_extra_isize < sbi->s_want_extra_isize &&
> 	    !ext4_test_inode_state(inode, EXT4_STATE_NO_EXPAND)) {
> 		/*
> -		 * We need extra buffer credits since we may write into EA block
> +		 * In nojournal mode, we can immediately attempt to expand
> +		 * the inode.  When journaled, we first need to obtain extra
> +		 * buffer credits since we may write into the EA block
> 		 * with this same handle. If journal_extend fails, then it will
> 		 * only result in a minor loss of functionality for that inode.
> 		 * If this is felt to be critical, then e2fsck should be run to
> 		 * force a large enough s_min_extra_isize.
> 		 */
> -		if ((jbd2_journal_extend(handle,
> -			     EXT4_DATA_TRANS_BLOCKS(inode->i_sb))) == 0) {
> +		if (!ext4_handle_valid(handle) ||
> +		    jbd2_journal_extend(handle,
> +			     EXT4_DATA_TRANS_BLOCKS(inode->i_sb)) == 0) {
> 			ret = ext4_expand_extra_isize(inode,
> 						      sbi->s_want_extra_isize,
> 						      iloc, handle);
> --
> 2.1.4
> 
> --
> 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


Cheers, Andreas

On Tue, Oct 25, 2016 at 03:54:16PM -0600, Andreas Dilger wrote:
> I think the reason we required that the filesystem be journaled to expand
> the inode reserved space is to ensure that the update was an all-or-nothing
> approach.  If there are in-inode xattrs that need to be moved to an external
> xattr block, and that block needs allocation and such, it is possible that
> a nojournal filesystem could result in the xattrs being moved and the inode
> is written (w/o the xattrs), but the xattr block is not written to disk
> before a crash.

In nojournal mode, any number of failures can cause data loss.  For
example, we could be splitting a node in an extent tree, or in a htree
directory block, and we can end up losing data if we crash at an
inconvenient time.  The chances of losing half of an interior node of
an extent tree block after a crash is going to be the same, and the
data loss form such same is going to be far worse than the loss if we
are unluck expanding an inode and moving xattrs to an external block....

    	   	     	      	  - 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

On Tue, Oct 25, 2016 at 05:14:05PM -0400, Eric Whitney wrote:
> Runs of xfstest ext4/022 on nojournal file systems result in failures
> because the inodes of some of its test files do not expand as expected.
> The cause is a conditional in ext4_mark_inode_dirty() that prevents inode
> expansion unless the test file system has a journal.  Remove this
> unnecessary restriction.
> 
> Signed-off-by: Eric Whitney <enwlinux@gmail.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