[PATCH] ext4: have ext4_xattr_set_handle() allocate journal credits

[PATCH] ext4: have ext4_xattr_set_handle() allocate journal credits

[Tahsin Erdogan]

Currently, callers of ext4_xattr_set_handle() are expected to allocate
journal credits for setting extended attributes. ext4_xattr_set_credits()
helps them figure out necessary credits. ext4_xattr_set_handle()
performs a sufficient credits check before starting data updates.

This model works fine for callers that start the transaction handle
themselves, such as ext4_xattr_set(). Other callers like ext4_set_context()
and __ext4_set_acl() use transaction handles that were started by outer
scopes so for this to work properly we would have to involve outer scope
callers to incorporate xattr credits.

Also, existing sufficient credits check in ext4_xattr_set_handle()
ignores the fact that allocated credits may have been intended for
operations other than setting extended attributes. So in theory
ext4_xattr_set_handle() may finish successfully but have also depleted
the journal credits that were supposed to be used by other operations
afterwards.

In this patch, responsibility of allocating journal credits is moved to
ext4_xattr_set_handle(). It tries to extend the journal credits as
needed or will restart the transaction handle if extend doesn't work.

Fixes: 021264a98d700e8 ("ext4: improve journal credit handling in set xattr paths")

Signed-off-by: Tahsin Erdogan <tahsin@google.com>
---
 fs/ext4/acl.c   | 10 +++-------
 fs/ext4/super.c |  9 +++------
 fs/ext4/xattr.c | 39 +++++++--------------------------------
 fs/ext4/xattr.h |  2 --
 4 files changed, 13 insertions(+), 47 deletions(-)

diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index 8db03e5c78bc..537a469d8039 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -231,18 +231,14 @@ int
 ext4_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	handle_t *handle;
-	int error, credits, retries = 0;
-	size_t acl_size = acl ? ext4_acl_size(acl->a_count) : 0;
+	int error, retries = 0;
 
 	error = dquot_initialize(inode);
 	if (error)
 		return error;
 retry:
-	error = ext4_xattr_set_credits(inode, acl_size, &credits);
-	if (error)
-		return error;
-
-	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
+	/* ext4_xattr_set_handle() allocates journal credits if necessary. */
+	handle = ext4_journal_start(inode, EXT4_HT_XATTR, 0 /* credits */);
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 56c971807df5..96d4b5296fa0 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1150,7 +1150,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 							void *fs_data)
 {
 	handle_t *handle = fs_data;
-	int res, res2, credits, retries = 0;
+	int res, res2, retries = 0;
 
 	/*
 	 * Encrypting the root directory is not allowed because e2fsck expects
@@ -1194,11 +1194,8 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	if (res)
 		return res;
 retry:
-	res = ext4_xattr_set_credits(inode, len, &credits);
-	if (res)
-		return res;
-
-	handle = ext4_journal_start(inode, EXT4_HT_MISC, credits);
+	/* ext4_xattr_set_handle() allocates journal credits if necessary. */
+	handle = ext4_journal_start(inode, EXT4_HT_MISC, 0 /* credits */);
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 062756b4e6d8..6b909804aa5e 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2253,7 +2253,11 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 		credits = __ext4_xattr_set_credits(inode->i_sb, bh, value_len);
 		brelse(bh);
 
-		if (!ext4_handle_has_enough_credits(handle, credits)) {
+		if (ext4_xattr_ensure_credits(
+					handle, inode,
+					handle->h_buffer_credits + credits,
+					NULL /* bh */, false /* dirty */,
+					false /* block_csum */)) {
 			error = -ENOSPC;
 			goto cleanup;
 		}
@@ -2357,31 +2361,6 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 	return error;
 }
 
-int ext4_xattr_set_credits(struct inode *inode, size_t value_len, int *credits)
-{
-	struct buffer_head *bh;
-	int err;
-
-	*credits = 0;
-
-	if (!EXT4_SB(inode->i_sb)->s_journal)
-		return 0;
-
-	down_read(&EXT4_I(inode)->xattr_sem);
-
-	bh = ext4_xattr_get_block(inode);
-	if (IS_ERR(bh)) {
-		err = PTR_ERR(bh);
-	} else {
-		*credits = __ext4_xattr_set_credits(inode->i_sb, bh, value_len);
-		brelse(bh);
-		err = 0;
-	}
-
-	up_read(&EXT4_I(inode)->xattr_sem);
-	return err;
-}
-
 /*
  * ext4_xattr_set()
  *
@@ -2397,18 +2376,14 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
 	handle_t *handle;
 	struct super_block *sb = inode->i_sb;
 	int error, retries = 0;
-	int credits;
 
 	error = dquot_initialize(inode);
 	if (error)
 		return error;
 
 retry:
-	error = ext4_xattr_set_credits(inode, value_len, &credits);
-	if (error)
-		return error;
-
-	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
+	/* ext4_xattr_set_handle() allocates journal credits if necessary. */
+	handle = ext4_journal_start(inode, EXT4_HT_XATTR, 0 /* credits */);
 	if (IS_ERR(handle)) {
 		error = PTR_ERR(handle);
 	} else {
diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
index 26119a67c8c3..f81cee5c21cc 100644
--- a/fs/ext4/xattr.h
+++ b/fs/ext4/xattr.h
@@ -152,8 +152,6 @@ extern ssize_t ext4_listxattr(struct dentry *, char *, size_t);
 extern int ext4_xattr_get(struct inode *, int, const char *, void *, size_t);
 extern int ext4_xattr_set(struct inode *, int, const char *, const void *, size_t, int);
 extern int ext4_xattr_set_handle(handle_t *, struct inode *, int, const char *, const void *, size_t, int);
-extern int ext4_xattr_set_credits(struct inode *inode, size_t value_len,
-				  int *credits);
 
 extern int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode,
 				   struct ext4_xattr_inode_array **array,
-- 
2.13.2.725.g09c95d1e9-goog

[PATCH v2] ext4: have ext4_xattr_set_handle() allocate journal credits

[Tahsin Erdogan]

Currently, callers of ext4_xattr_set_handle() are expected to allocate
journal credits for setting extended attributes. ext4_xattr_set_credits()
helps them figure out necessary credits. ext4_xattr_set_handle()
performs a sufficient credits check before starting data updates.

This model works fine for callers that start the transaction handle
themselves, such as ext4_xattr_set(). Other callers like ext4_set_context()
and __ext4_set_acl() use transaction handles that were started by outer
scopes so for this to work properly we would have to involve outer scope
callers to incorporate xattr credits.

Also, existing sufficient credits check in ext4_xattr_set_handle()
ignores the fact that allocated credits may have been intended for
operations other than setting extended attributes. So in theory
ext4_xattr_set_handle() may finish successfully but have also depleted
the journal credits that were supposed to be used by other operations
afterwards.

In this patch, responsibility of allocating journal credits is moved to
ext4_xattr_set_handle(). It tries to extend the journal credits as
needed or will restart the transaction handle if extend doesn't work.

Fixes: 021264a98d700e8 ("ext4: improve journal credit handling in set xattr paths")

Signed-off-by: Tahsin Erdogan <tahsin@google.com>
---
v2: Allocate 1 journal credit in ext4_set_acl() and ext4_set_context() for modifying
    inode outside ext4_xattr_set_handle() call.

 fs/ext4/acl.c   | 14 +++++++-------
 fs/ext4/super.c | 13 +++++++------
 fs/ext4/xattr.c | 39 +++++++--------------------------------
 fs/ext4/xattr.h |  2 --
 4 files changed, 21 insertions(+), 47 deletions(-)

diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index 8db03e5c78bc..6a4c2b831699 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -231,18 +231,18 @@ int
 ext4_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	handle_t *handle;
-	int error, credits, retries = 0;
-	size_t acl_size = acl ? ext4_acl_size(acl->a_count) : 0;
+	int error, retries = 0;
 
 	error = dquot_initialize(inode);
 	if (error)
 		return error;
 retry:
-	error = ext4_xattr_set_credits(inode, acl_size, &credits);
-	if (error)
-		return error;
-
-	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
+	/*
+	 * Allocate 1 journal credit to allow __ext4_set_acl() update inode
+	 * before calling ext4_xattr_set_handle(). Extended attribute related
+	 * credits are allocated by ext4_xattr_set_handle() itself.
+	 */
+	handle = ext4_journal_start(inode, EXT4_HT_XATTR, 1 /* credits */);
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 56c971807df5..74036c382b63 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1150,7 +1150,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 							void *fs_data)
 {
 	handle_t *handle = fs_data;
-	int res, res2, credits, retries = 0;
+	int res, res2, retries = 0;
 
 	/*
 	 * Encrypting the root directory is not allowed because e2fsck expects
@@ -1194,11 +1194,12 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	if (res)
 		return res;
 retry:
-	res = ext4_xattr_set_credits(inode, len, &credits);
-	if (res)
-		return res;
-
-	handle = ext4_journal_start(inode, EXT4_HT_MISC, credits);
+	/*
+	 * Allocate 1 journal credit for ext4_mark_inode_dirty() call in this
+	 * function. Extended attribute related credits are allocated by
+	 * ext4_xattr_set_handle() itself.
+	 */
+	handle = ext4_journal_start(inode, EXT4_HT_MISC, 1 /* credits */);
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 062756b4e6d8..6b909804aa5e 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2253,7 +2253,11 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 		credits = __ext4_xattr_set_credits(inode->i_sb, bh, value_len);
 		brelse(bh);
 
-		if (!ext4_handle_has_enough_credits(handle, credits)) {
+		if (ext4_xattr_ensure_credits(
+					handle, inode,
+					handle->h_buffer_credits + credits,
+					NULL /* bh */, false /* dirty */,
+					false /* block_csum */)) {
 			error = -ENOSPC;
 			goto cleanup;
 		}
@@ -2357,31 +2361,6 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 	return error;
 }
 
-int ext4_xattr_set_credits(struct inode *inode, size_t value_len, int *credits)
-{
-	struct buffer_head *bh;
-	int err;
-
-	*credits = 0;
-
-	if (!EXT4_SB(inode->i_sb)->s_journal)
-		return 0;
-
-	down_read(&EXT4_I(inode)->xattr_sem);
-
-	bh = ext4_xattr_get_block(inode);
-	if (IS_ERR(bh)) {
-		err = PTR_ERR(bh);
-	} else {
-		*credits = __ext4_xattr_set_credits(inode->i_sb, bh, value_len);
-		brelse(bh);
-		err = 0;
-	}
-
-	up_read(&EXT4_I(inode)->xattr_sem);
-	return err;
-}
-
 /*
  * ext4_xattr_set()
  *
@@ -2397,18 +2376,14 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
 	handle_t *handle;
 	struct super_block *sb = inode->i_sb;
 	int error, retries = 0;
-	int credits;
 
 	error = dquot_initialize(inode);
 	if (error)
 		return error;
 
 retry:
-	error = ext4_xattr_set_credits(inode, value_len, &credits);
-	if (error)
-		return error;
-
-	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
+	/* ext4_xattr_set_handle() allocates journal credits if necessary. */
+	handle = ext4_journal_start(inode, EXT4_HT_XATTR, 0 /* credits */);
 	if (IS_ERR(handle)) {
 		error = PTR_ERR(handle);
 	} else {
diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
index 26119a67c8c3..f81cee5c21cc 100644
--- a/fs/ext4/xattr.h
+++ b/fs/ext4/xattr.h
@@ -152,8 +152,6 @@ extern ssize_t ext4_listxattr(struct dentry *, char *, size_t);
 extern int ext4_xattr_get(struct inode *, int, const char *, void *, size_t);
 extern int ext4_xattr_set(struct inode *, int, const char *, const void *, size_t, int);
 extern int ext4_xattr_set_handle(handle_t *, struct inode *, int, const char *, const void *, size_t, int);
-extern int ext4_xattr_set_credits(struct inode *inode, size_t value_len,
-				  int *credits);
 
 extern int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode,
 				   struct ext4_xattr_inode_array **array,
-- 
2.13.2.725.g09c95d1e9-goog

Re: [PATCH v2] ext4: have ext4_xattr_set_handle() allocate journal credits

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 8340 bytes --]

On Jun 28, 2017, at 9:06 PM, Tahsin Erdogan <tahsin@google.com> wrote:
> 
> Currently, callers of ext4_xattr_set_handle() are expected to allocate
> journal credits for setting extended attributes. ext4_xattr_set_credits()
> helps them figure out necessary credits. ext4_xattr_set_handle()
> performs a sufficient credits check before starting data updates.
> 
> This model works fine for callers that start the transaction handle
> themselves, such as ext4_xattr_set(). Other callers like ext4_set_context()
> and __ext4_set_acl() use transaction handles that were started by outer
> scopes so for this to work properly we would have to involve outer scope
> callers to incorporate xattr credits.
> 
> Also, existing sufficient credits check in ext4_xattr_set_handle()
> ignores the fact that allocated credits may have been intended for
> operations other than setting extended attributes. So in theory
> ext4_xattr_set_handle() may finish successfully but have also depleted
> the journal credits that were supposed to be used by other operations
> afterwards.
> 
> In this patch, responsibility of allocating journal credits is moved to
> ext4_xattr_set_handle(). It tries to extend the journal credits as
> needed or will restart the transaction handle if extend doesn't work.

One problem with this approach is that restarting the transaction handle will
make the xattr update non-atomic, which could be a real problem for some
workloads.  For example, ACLs or SELinux or fscrypt xattrs being added in
a separate transaction from file creation, or being modified (delete in a
separate transaction from add) and then lost completely if the system crashes
before the second transaction is committed.

It isn't clear to me why using the current helper function to precompute the
required transaction credits doesn't get this right in the first place?  It
would just need to add the xattr credits to the original journal handle?



This brings up a potential area for improvement in ext4, namely that instead
of having all of the callers precompute the number of transaction credits, it
would be possible to declare operations (e.g. ext4_declare_create(),
ext4_declare_setxattr(), ext4_declare_add_entry(), ext4_declare_write())
and then have helpers that will compute these values (possibly dependent on
the specific inode being modified) and the caller just adds up the various
credits returned for each operation.  The ext4_declare_create() function would
gain subsidiary calls to ext4_declare_setxattr() that depend on on filesystem
features enabled and/or the parent directory, so that the caller doesn't need
to know all of the details.

That is probably larger than what is needed for this particular patch, but
something to consider as this code gets increasingly complex.

Cheers, Andreas

> Fixes: 021264a98d700e8 ("ext4: improve journal credit handling in set xattr paths")
> 
> Signed-off-by: Tahsin Erdogan <tahsin@google.com>
> ---
> v2: Allocate 1 journal credit in ext4_set_acl() and ext4_set_context() for modifying
>    inode outside ext4_xattr_set_handle() call.
> 
> fs/ext4/acl.c   | 14 +++++++-------
> fs/ext4/super.c | 13 +++++++------
> fs/ext4/xattr.c | 39 +++++++--------------------------------
> fs/ext4/xattr.h |  2 --
> 4 files changed, 21 insertions(+), 47 deletions(-)
> 
> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
> index 8db03e5c78bc..6a4c2b831699 100644
> --- a/fs/ext4/acl.c
> +++ b/fs/ext4/acl.c
> @@ -231,18 +231,18 @@ int
> ext4_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> {
> 	handle_t *handle;
> -	int error, credits, retries = 0;
> -	size_t acl_size = acl ? ext4_acl_size(acl->a_count) : 0;
> +	int error, retries = 0;
> 
> 	error = dquot_initialize(inode);
> 	if (error)
> 		return error;
> retry:
> -	error = ext4_xattr_set_credits(inode, acl_size, &credits);
> -	if (error)
> -		return error;
> -
> -	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
> +	/*
> +	 * Allocate 1 journal credit to allow __ext4_set_acl() update inode
> +	 * before calling ext4_xattr_set_handle(). Extended attribute related
> +	 * credits are allocated by ext4_xattr_set_handle() itself.
> +	 */
> +	handle = ext4_journal_start(inode, EXT4_HT_XATTR, 1 /* credits */);
> 	if (IS_ERR(handle))
> 		return PTR_ERR(handle);
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 56c971807df5..74036c382b63 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1150,7 +1150,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
> 							void *fs_data)
> {
> 	handle_t *handle = fs_data;
> -	int res, res2, credits, retries = 0;
> +	int res, res2, retries = 0;
> 
> 	/*
> 	 * Encrypting the root directory is not allowed because e2fsck expects
> @@ -1194,11 +1194,12 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
> 	if (res)
> 		return res;
> retry:
> -	res = ext4_xattr_set_credits(inode, len, &credits);
> -	if (res)
> -		return res;
> -
> -	handle = ext4_journal_start(inode, EXT4_HT_MISC, credits);
> +	/*
> +	 * Allocate 1 journal credit for ext4_mark_inode_dirty() call in this
> +	 * function. Extended attribute related credits are allocated by
> +	 * ext4_xattr_set_handle() itself.
> +	 */
> +	handle = ext4_journal_start(inode, EXT4_HT_MISC, 1 /* credits */);
> 	if (IS_ERR(handle))
> 		return PTR_ERR(handle);
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 062756b4e6d8..6b909804aa5e 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2253,7 +2253,11 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
> 		credits = __ext4_xattr_set_credits(inode->i_sb, bh, value_len);
> 		brelse(bh);
> 
> -		if (!ext4_handle_has_enough_credits(handle, credits)) {
> +		if (ext4_xattr_ensure_credits(
> +					handle, inode,
> +					handle->h_buffer_credits + credits,
> +					NULL /* bh */, false /* dirty */,
> +					false /* block_csum */)) {
> 			error = -ENOSPC;
> 			goto cleanup;
> 		}
> @@ -2357,31 +2361,6 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
> 	return error;
> }
> 
> -int ext4_xattr_set_credits(struct inode *inode, size_t value_len, int *credits)
> -{
> -	struct buffer_head *bh;
> -	int err;
> -
> -	*credits = 0;
> -
> -	if (!EXT4_SB(inode->i_sb)->s_journal)
> -		return 0;
> -
> -	down_read(&EXT4_I(inode)->xattr_sem);
> -
> -	bh = ext4_xattr_get_block(inode);
> -	if (IS_ERR(bh)) {
> -		err = PTR_ERR(bh);
> -	} else {
> -		*credits = __ext4_xattr_set_credits(inode->i_sb, bh, value_len);
> -		brelse(bh);
> -		err = 0;
> -	}
> -
> -	up_read(&EXT4_I(inode)->xattr_sem);
> -	return err;
> -}
> -
> /*
>  * ext4_xattr_set()
>  *
> @@ -2397,18 +2376,14 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
> 	handle_t *handle;
> 	struct super_block *sb = inode->i_sb;
> 	int error, retries = 0;
> -	int credits;
> 
> 	error = dquot_initialize(inode);
> 	if (error)
> 		return error;
> 
> retry:
> -	error = ext4_xattr_set_credits(inode, value_len, &credits);
> -	if (error)
> -		return error;
> -
> -	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
> +	/* ext4_xattr_set_handle() allocates journal credits if necessary. */
> +	handle = ext4_journal_start(inode, EXT4_HT_XATTR, 0 /* credits */);
> 	if (IS_ERR(handle)) {
> 		error = PTR_ERR(handle);
> 	} else {
> diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
> index 26119a67c8c3..f81cee5c21cc 100644
> --- a/fs/ext4/xattr.h
> +++ b/fs/ext4/xattr.h
> @@ -152,8 +152,6 @@ extern ssize_t ext4_listxattr(struct dentry *, char *, size_t);
> extern int ext4_xattr_get(struct inode *, int, const char *, void *, size_t);
> extern int ext4_xattr_set(struct inode *, int, const char *, const void *, size_t, int);
> extern int ext4_xattr_set_handle(handle_t *, struct inode *, int, const char *, const void *, size_t, int);
> -extern int ext4_xattr_set_credits(struct inode *inode, size_t value_len,
> -				  int *credits);
> 
> extern int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode,
> 				   struct ext4_xattr_inode_array **array,
> --
> 2.13.2.725.g09c95d1e9-goog
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v2] ext4: have ext4_xattr_set_handle() allocate journal credits

[Tahsin Erdogan]

> One problem with this approach is that restarting the transaction handle will
> make the xattr update non-atomic, which could be a real problem for some
> workloads.  For example, ACLs or SELinux or fscrypt xattrs being added in
> a separate transaction from file creation, or being modified (delete in a
> separate transaction from add) and then lost completely if the system crashes
> before the second transaction is committed.

Agreed.

> It isn't clear to me why using the current helper function to precompute the
> required transaction credits doesn't get this right in the first place?  It
> would just need to add the xattr credits to the original journal handle?

An example code path is this:

ext4_mkdir()
  ext4_new_inode_start_handle()
    __ext4_new_inode()   <<== transaction handle is started here
      ext4_init_acl()
        __ext4_set_acl()
          ext4_xattr_set_handle()

In this case, __ext4_new_inode() needs to figure out all journal
credits needed including the ones for ext4_xattr_set_handle(). This is
a few levels deep so reaching out to ext4_xattr_set_credits() with the
right parameters is where the complexity lies.

Re: [PATCH v2] ext4: have ext4_xattr_set_handle() allocate journal credits

[Theodore Ts'o]

On Fri, Jun 30, 2017 at 12:36:51PM -0700, Tahsin Erdogan wrote:
> > One problem with this approach is that restarting the transaction handle will
> > make the xattr update non-atomic, which could be a real problem for some
> > workloads.  For example, ACLs or SELinux or fscrypt xattrs being added in
> > a separate transaction from file creation, or being modified (delete in a
> > separate transaction from add) and then lost completely if the system crashes
> > before the second transaction is committed.
> 
> Agreed.

I really don't like this patch for this reason.

In fact, it doesn't work because in your example code path:

> An example code path is this:
> 
> ext4_mkdir()
>   ext4_new_inode_start_handle()
>     __ext4_new_inode()   <<== transaction handle is started here
>       ext4_init_acl()
>         __ext4_set_acl()
>           ext4_xattr_set_handle()
> 
> In this case, __ext4_new_inode() needs to figure out all journal
> credits needed including the ones for ext4_xattr_set_handle(). This is
> a few levels deep so reaching out to ext4_xattr_set_credits() with the
> right parameters is where the complexity lies.

If we need to restart a transaction in ext4_init_acl(), we will end up
breaking up a transaction into two pieces.  Which means if we crash,
we could very easily end up with a corrupt file system because the
inode might be allocated, but not yet linked into the directory
hierarchy.

Worse, it doesn't really solve the problem because
ext4_xattr_ensure_credits() merely makes sure there are enough credits
for the xattr operation.  If setting the xattr ACL chews up credits
needed to insert the name of the newly created file into the
directory, you're still going to end up running into problems.

The way we've historically handled this is to simplify things by
making worse-case estimates when the transaction handled started.  So
for example, we assume the worst case that we'll split an directory
hash tree, even though we might not know whether or not this will be
necessary.  That's because if we over-estimate the number of credits
needed for a handle, it's really not a disaster.  Most handles are
active for a very short time, and when we close the handle, we can
give back any unused credits.

I understand that for ext4_new_inode it can be quite tricky, since in
theory we might need to add an SE Linux label, plus an ACL, plus an
encryption context.

The one good news is that is that with the xattr inode deduplication
feature, ext4_init_acl as called from ext4_new_inode should always
require just bumping a refcount, since the ACL will be inherited from
the directory's default ACL.

The bad news is that in general, we don't know what
security_inode_init_security() will do.  In theory, it could try to
set an arbitrarily large lael, although in practice we know the SE
Linux label tends not to be too terribly large.

Are you aware of other cases where we're likely to run into problems
besides ext4_new_inode()?

						- Ted
						

Re: [PATCH v2] ext4: have ext4_xattr_set_handle() allocate journal credits

[Tahsin Erdogan]

> Are you aware of other cases where we're likely to run into problems
> besides ext4_new_inode()?

Nope. If we can get ext4_new_inode() case covered we should be fine.

I will abandon this patch and will work on a patch that adds extra
credits in __ext4_new_inode().

thanks