[PATCH 1/2] ext4: skip ext4_init_security() and encryption on ea_inodes

[PATCH 1/2] ext4: skip ext4_init_security() and encryption on ea_inodes

[Tahsin Erdogan]

Extended attribute inodes are internal to ext4. Adding encryption/security
related attributes on them would mean dealing with nested calls into ea code.
Since they have no direct exposure to user mode, just avoid creating ea
entries for them.

Signed-off-by: Tahsin Erdogan <tahsin@google.com>
---
 fs/ext4/ialloc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index fb1b3df17f6e..0c79e3efcaf7 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -771,7 +771,8 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 
 	if ((ext4_encrypted_inode(dir) ||
 	     DUMMY_ENCRYPTION_ENABLED(EXT4_SB(dir->i_sb))) &&
-	    (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode))) {
+	    (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)) &&
+	    !(i_flags & EXT4_EA_INODE_FL)) {
 		err = fscrypt_get_encryption_info(dir);
 		if (err)
 			return ERR_PTR(err);
@@ -1114,11 +1115,11 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 		err = ext4_init_acl(handle, inode, dir);
 		if (err)
 			goto fail_free_drop;
-	}
 
-	err = ext4_init_security(handle, inode, dir, qstr);
-	if (err)
-		goto fail_free_drop;
+		err = ext4_init_security(handle, inode, dir, qstr);
+		if (err)
+			goto fail_free_drop;
+	}
 
 	if (ext4_has_feature_extents(sb)) {
 		/* set extent flag only for directory, file and normal symlink*/
-- 
2.13.2.725.g09c95d1e9-goog

[PATCH 2/2] ext4: fix __ext4_new_inode() journal credits calculation

[Tahsin Erdogan]

ea_inode feature allows creating extended attributes that are up to
64k in size. Update __ext4_new_inode() to pick increased credit limits.

To avoid overallocating too many journal credits, update
__ext4_xattr_set_credits() to make a distinction between xattr create
vs update. This helps __ext4_new_inode() because all attributes are
known to be new, so we can save credits that are normally needed to
delete old values.

Also, have fscrypt specify its maximum context size so that we don't
end up allocating credits for 64k size.

Signed-off-by: Tahsin Erdogan <tahsin@google.com>
---
 fs/crypto/policy.c             |  1 +
 fs/ext4/acl.c                  | 13 ++++++------
 fs/ext4/ialloc.c               | 26 +++++++++++++++++-------
 fs/ext4/super.c                |  3 ++-
 fs/ext4/xattr.c                | 46 +++++++++++++++++++++++++-----------------
 fs/ext4/xattr.h                |  5 ++++-
 include/linux/fscrypt_common.h |  3 +++
 7 files changed, 63 insertions(+), 34 deletions(-)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 210976e7a269..94becf5a1519 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -260,6 +260,7 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child,
 	memcpy(ctx.master_key_descriptor, ci->ci_master_key,
 	       FS_KEY_DESCRIPTOR_SIZE);
 	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
+	BUILD_BUG_ON(sizeof(ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
 	res = parent->i_sb->s_cop->set_context(child, &ctx,
 						sizeof(ctx), fs_data);
 	if (res)
diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index 8db03e5c78bc..09441ae07a5b 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -183,7 +183,7 @@ ext4_get_acl(struct inode *inode, int type)
  */
 static int
 __ext4_set_acl(handle_t *handle, struct inode *inode, int type,
-	     struct posix_acl *acl)
+	     struct posix_acl *acl, int xattr_flags)
 {
 	int name_index;
 	void *value = NULL;
@@ -218,7 +218,7 @@ __ext4_set_acl(handle_t *handle, struct inode *inode, int type,
 	}
 
 	error = ext4_xattr_set_handle(handle, inode, name_index, "",
-				      value, size, 0);
+				      value, size, xattr_flags);
 
 	kfree(value);
 	if (!error)
@@ -238,7 +238,8 @@ ext4_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	if (error)
 		return error;
 retry:
-	error = ext4_xattr_set_credits(inode, acl_size, &credits);
+	error = ext4_xattr_set_credits(inode, acl_size, false /* is_create */,
+				       &credits);
 	if (error)
 		return error;
 
@@ -246,7 +247,7 @@ ext4_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 
-	error = __ext4_set_acl(handle, inode, type, acl);
+	error = __ext4_set_acl(handle, inode, type, acl, 0 /* xattr_flags */);
 	ext4_journal_stop(handle);
 	if (error == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
 		goto retry;
@@ -271,13 +272,13 @@ ext4_init_acl(handle_t *handle, struct inode *inode, struct inode *dir)
 
 	if (default_acl) {
 		error = __ext4_set_acl(handle, inode, ACL_TYPE_DEFAULT,
-				       default_acl);
+				       default_acl, XATTR_CREATE);
 		posix_acl_release(default_acl);
 	}
 	if (acl) {
 		if (!error)
 			error = __ext4_set_acl(handle, inode, ACL_TYPE_ACCESS,
-					       acl);
+					       acl, XATTR_CREATE);
 		posix_acl_release(acl);
 	}
 	return error;
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 0c79e3efcaf7..21a2538afcc2 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -766,11 +766,13 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 	if (!dir || !dir->i_nlink)
 		return ERR_PTR(-EPERM);
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(dir->i_sb))))
+	sb = dir->i_sb;
+	sbi = EXT4_SB(sb);
+
+	if (unlikely(ext4_forced_shutdown(sbi)))
 		return ERR_PTR(-EIO);
 
-	if ((ext4_encrypted_inode(dir) ||
-	     DUMMY_ENCRYPTION_ENABLED(EXT4_SB(dir->i_sb))) &&
+	if ((ext4_encrypted_inode(dir) || DUMMY_ENCRYPTION_ENABLED(sbi)) &&
 	    (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)) &&
 	    !(i_flags & EXT4_EA_INODE_FL)) {
 		err = fscrypt_get_encryption_info(dir);
@@ -778,19 +780,29 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 			return ERR_PTR(err);
 		if (!fscrypt_has_encryption_key(dir))
 			return ERR_PTR(-ENOKEY);
-		if (!handle)
-			nblocks += EXT4_DATA_TRANS_BLOCKS(dir->i_sb);
 		encrypt = 1;
 	}
 
-	sb = dir->i_sb;
+	if (!handle && sbi->s_journal && !(i_flags & EXT4_EA_INODE_FL)) {
+		/*
+		 * 2 ea entries for ext4_init_acl(), 2 for ext4_init_security().
+		 */
+		nblocks += 4 * __ext4_xattr_set_credits(sb, NULL /* inode */,
+					NULL /* block_bh */, XATTR_SIZE_MAX,
+					true /* is_create */);
+		if (encrypt)
+			nblocks += __ext4_xattr_set_credits(sb,
+					NULL /* inode */, NULL /* block_bh */,
+					FSCRYPT_SET_CONTEXT_MAX_SIZE,
+					true /* is_create */);
+	}
+
 	ngroups = ext4_get_groups_count(sb);
 	trace_ext4_request_inode(dir, mode);
 	inode = new_inode(sb);
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
 	ei = EXT4_I(inode);
-	sbi = EXT4_SB(sb);
 
 	/*
 	 * Initialize owners and quota early so that we don't have to account
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 56c971807df5..f666042a3d58 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1194,7 +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);
+	res = ext4_xattr_set_credits(inode, len, false /* is_create */,
+				     &credits);
 	if (res)
 		return res;
 
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 624aad69991d..1f617ffb775a 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -829,11 +829,10 @@ static void ext4_xattr_inode_free_quota(struct inode *inode, size_t len)
 	dquot_free_inode(inode);
 }
 
-static int __ext4_xattr_set_credits(struct inode *inode,
-				    struct buffer_head *block_bh,
-				    size_t value_len)
+int __ext4_xattr_set_credits(struct super_block *sb, struct inode *inode,
+			     struct buffer_head *block_bh, size_t value_len,
+			     bool is_create)
 {
-	struct super_block *sb = inode->i_sb;
 	int credits;
 	int blocks;
 
@@ -859,7 +858,7 @@ static int __ext4_xattr_set_credits(struct inode *inode,
 	 * In case of inline data, we may push out the data to a block,
 	 * so we need to reserve credits for this eventuality
 	 */
-	if (ext4_has_inline_data(inode))
+	if (inode && ext4_has_inline_data(inode))
 		credits += ext4_writepage_trans_blocks(inode) + 1;
 
 	/* We are done if ea_inode feature is not enabled. */
@@ -881,19 +880,23 @@ static int __ext4_xattr_set_credits(struct inode *inode,
 	/* Blocks themselves. */
 	credits += blocks;
 
-	/* Dereference ea_inode holding old xattr value.
-	 * Old ea_inode, inode map, block bitmap, group descriptor.
-	 */
-	credits += 4;
+	if (!is_create) {
+		/* Dereference ea_inode holding old xattr value.
+		 * Old ea_inode, inode map, block bitmap, group descriptor.
+		 */
+		credits += 4;
 
-	/* Data blocks for old ea_inode. */
-	blocks = XATTR_SIZE_MAX >> sb->s_blocksize_bits;
+		/* Data blocks for old ea_inode. */
+		blocks = XATTR_SIZE_MAX >> sb->s_blocksize_bits;
 
-	/* Indirection block or one level of extent tree for old ea_inode. */
-	blocks += 1;
+		/* Indirection block or one level of extent tree for old
+		 * ea_inode.
+		 */
+		blocks += 1;
 
-	/* Block bitmap and group descriptor updates for each block. */
-	credits += blocks * 2;
+		/* Block bitmap and group descriptor updates for each block. */
+		credits += blocks * 2;
+	}
 
 	/* We may need to clone the existing xattr block in which case we need
 	 * to increment ref counts for existing ea_inodes referenced by it.
@@ -2262,7 +2265,9 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 			goto cleanup;
 		}
 
-		credits = __ext4_xattr_set_credits(inode, bh, value_len);
+		credits = __ext4_xattr_set_credits(inode->i_sb, inode, bh,
+						   value_len,
+						   flags & XATTR_CREATE);
 		brelse(bh);
 
 		if (!ext4_handle_has_enough_credits(handle, credits)) {
@@ -2369,7 +2374,8 @@ 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)
+int ext4_xattr_set_credits(struct inode *inode, size_t value_len,
+			   bool is_create, int *credits)
 {
 	struct buffer_head *bh;
 	int err;
@@ -2385,7 +2391,8 @@ int ext4_xattr_set_credits(struct inode *inode, size_t value_len, int *credits)
 	if (IS_ERR(bh)) {
 		err = PTR_ERR(bh);
 	} else {
-		*credits = __ext4_xattr_set_credits(inode, bh, value_len);
+		*credits = __ext4_xattr_set_credits(inode->i_sb, inode, bh,
+						    value_len, is_create);
 		brelse(bh);
 		err = 0;
 	}
@@ -2416,7 +2423,8 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
 		return error;
 
 retry:
-	error = ext4_xattr_set_credits(inode, value_len, &credits);
+	error = ext4_xattr_set_credits(inode, value_len, flags & XATTR_CREATE,
+				       &credits);
 	if (error)
 		return error;
 
diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
index 26119a67c8c3..0d2dde1fa87a 100644
--- a/fs/ext4/xattr.h
+++ b/fs/ext4/xattr.h
@@ -153,7 +153,10 @@ 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);
+				  bool is_create, int *credits);
+extern int __ext4_xattr_set_credits(struct super_block *sb, struct inode *inode,
+				struct buffer_head *block_bh, size_t value_len,
+				bool is_create);
 
 extern int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode,
 				   struct ext4_xattr_inode_array **array,
diff --git a/include/linux/fscrypt_common.h b/include/linux/fscrypt_common.h
index 0a30c106c1e5..82beaf70e7e2 100644
--- a/include/linux/fscrypt_common.h
+++ b/include/linux/fscrypt_common.h
@@ -83,6 +83,9 @@ struct fscrypt_operations {
 	unsigned (*max_namelen)(struct inode *);
 };
 
+/* Maximum value for the third parameter of fscrypt_operations.set_context(). */
+#define FSCRYPT_SET_CONTEXT_MAX_SIZE	28
+
 static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
 {
 	if (inode->i_sb->s_cop->dummy_context &&
-- 
2.13.2.725.g09c95d1e9-goog

Re: [PATCH 2/2] ext4: fix __ext4_new_inode() journal credits calculation

[Theodore Ts'o]

On Wed, Jul 05, 2017 at 07:38:19PM -0700, Tahsin Erdogan wrote:
> ea_inode feature allows creating extended attributes that are up to
> 64k in size. Update __ext4_new_inode() to pick increased credit limits.
> 
> To avoid overallocating too many journal credits, update
> __ext4_xattr_set_credits() to make a distinction between xattr create
> vs update. This helps __ext4_new_inode() because all attributes are
> known to be new, so we can save credits that are normally needed to
> delete old values.
> 
> Also, have fscrypt specify its maximum context size so that we don't
> end up allocating credits for 64k size.
> 
> Signed-off-by: Tahsin Erdogan <tahsin@google.com>

I've applied the following change to this patch in order to better
calculate the credits needed by ext4_new_inode.  The problem is that
it was estimating a worse case of 287 blocks for ext4_new_inode() on a
10MB file system using the default 1024 block size.  And on such a
file system, the typical size of the journal is 1024 blocks, and the
maximum number of credits that are allowed by a handle is 1024 / 4 =
256 blocks.  This cases a number of regression tests to blow up.

In reality the SElinux label and EVM/integrity xattrs are never going
to be 64k, so calculating the credits assuming that as the worst case
is not productive.  And normally the Posix ACL is never going to be a
worst case of 64k long, either...

					- Ted

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 21a2538afcc2..507bfb3344d4 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -784,12 +784,38 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 	}
 
 	if (!handle && sbi->s_journal && !(i_flags & EXT4_EA_INODE_FL)) {
-		/*
-		 * 2 ea entries for ext4_init_acl(), 2 for ext4_init_security().
-		 */
-		nblocks += 4 * __ext4_xattr_set_credits(sb, NULL /* inode */,
-					NULL /* block_bh */, XATTR_SIZE_MAX,
+#ifdef CONFIG_EXT4_FS_POSIX_ACL
+		struct posix_acl *p = get_acl(dir, ACL_TYPE_DEFAULT);
+
+		if (p) {
+			int acl_size = p->a_count * sizeof(ext4_acl_entry);
+
+			nblocks += (S_ISDIR(mode) ? 2 : 1) *
+				__ext4_xattr_set_credits(sb, NULL /* inode */,
+					NULL /* block_bh */, acl_size,
+					true /* is_create */);
+			posix_acl_release(p);
+		}
+#endif
+
+#ifdef CONFIG_SECURITY
+		{
+			int num_security_xattrs = 1;
+
+#ifdef CONFIG_INTEGRITY
+			num_security_xattrs++;
+#endif
+			/*
+			 * We assume that security xattrs are never
+			 * more than 1k.  In practice they are under
+			 * 128 bytes.
+			 */
+			nblocks += num_security_xattrs *
+				__ext4_xattr_set_credits(sb, NULL /* inode */,
+					NULL /* block_bh */, 1024,
 					true /* is_create */);
+		}
+#endif
 		if (encrypt)
 			nblocks += __ext4_xattr_set_credits(sb,
 					NULL /* inode */, NULL /* block_bh */,

More thoughts about xattrs, journal credits, and their location

[Theodore Ts'o]

On Sat, Jul 08, 2017 at 01:09:00AM -0400, Theodore Ts'o wrote:
> I've applied the following change to this patch in order to better
> calculate the credits needed by ext4_new_inode....

So I've been thinking about this some more, and why we can't use
extend_transaction because it might break up what needs to be single
transaction for ext4_new_inode().  And I think I may have come up with
an interesting solution.  It adds extra complexity, so it may not be
worth it, but I'll throw it out for general consideration.

What we could do is have ext4_new_inode check to see if there are
enough credits to do add the xattr's (if necessary) in a single
commit.  If not, what we could do is to add the inode to the orphan
list, and then set an inode state flag indicating we have done this.
At this point, we *can* break the ext4_new_inode() operation into
multiple commits, because if we crash in the middle the inode will be
cleaned up when we do the orphan list processing.

The downsides of this approach is that it causes the orphan list to be
a bottleneck.  So we would definitely not want to do this all time.
And the extra complexity of tracking whether the inode is on the
orphan list might not make it worth it.


The other thing that we might want to do is to establish a priority
weight system so we can indicate which xattrs should have preference
to be stored in the inode (no seeks to access), in the xattr block
(requires an extra seek to access), or an ea_inode (requires 2-3 seeks
to access).  In rough order

* Security xattrs -- they are smallest, are needed the most often (you
  might need them even to see if you are allowed to stat the inode).
* Encryption xattr, Acl and Rich Acl's --- needed whenever you
  access the inode.
* In-line data
* Everything else

Within each priority tier, the smaller xattrs should get preferential
treatment since the are more likely to leave space for other xattrs in
the storage area.

One of the other things we could do is to implement a further
optimization where once we decide that an xattr in an xattr block
should have its value moved to an ea_inode, , if there is space to
move the xattr from the xattr block to the extra space in the inode.
This could be done in a separate transaction, or even via a workqueue,
since it's an optimization of the on-disk encoding of the inode'x
xattr.  It's not clear the complexity is worth it (maybe it's
something we do in e2fsck first, the way we can optimize directories,
and from there we can decide if it's worth implementing in the
kernel).

					- Ted

Re: More thoughts about xattrs, journal credits, and their location

[Tahsin Erdogan]

> What we could do is have ext4_new_inode check to see if there are
> enough credits to do add the xattr's (if necessary) in a single
> commit.  If not, what we could do is to add the inode to the orphan
> list, and then set an inode state flag indicating we have done this.
> At this point, we *can* break the ext4_new_inode() operation into
> multiple commits, because if we crash in the middle the inode will be
> cleaned up when we do the orphan list processing.

This makes sense. Also, we currently add the worst case credit
estimates of individual set xattr ops and start a journal handle with
the sum of it. A slight optimization is to do this lazily.
We can start with enough credits that can get us to a point where it
is safe to start a new transaction (safe because of orphan addition).
Then opportunistically extend the credits to get us to the next safe
point, if that doesn't work, do the orphan add operation and start a
new transaction. This should handle the worst case scenario and also
optimize for common case. Also this should in general reduce the
amount of allocated-but-unused credits which helps parallelism.

> The downsides of this approach is that it causes the orphan list to be
> a bottleneck.  So we would definitely not want to do this all time.

Yes and I think lazy extend/restart should mitigate this.

Re: More thoughts about xattrs, journal credits, and their location

[Theodore Ts'o]

On Sun, Jul 09, 2017 at 01:01:00PM -0700, Tahsin Erdogan wrote:
> > What we could do is have ext4_new_inode check to see if there are
> > enough credits to do add the xattr's (if necessary) in a single
> > commit.  If not, what we could do is to add the inode to the orphan
> > list, and then set an inode state flag indicating we have done this.
> > At this point, we *can* break the ext4_new_inode() operation into
> > multiple commits, because if we crash in the middle the inode will be
> > cleaned up when we do the orphan list processing.
> 
> This makes sense. Also, we currently add the worst case credit
> estimates of individual set xattr ops and start a journal handle with
> the sum of it. A slight optimization is to do this lazily.
> We can start with enough credits that can get us to a point where it
> is safe to start a new transaction (safe because of orphan addition).

I still am very concerned about the code complexity that this approach
requires.  I am also very concerned about the CPU scalability
bottleneck that adding and removing the inode from the orphan list
would entail.

And if we have to wait for the new commit to start so that we can
start a new handle, that's also a CPU scalability bottleneck and is
guaranteed to add significant latency.

One of the nice things about the xattr priority proposal is that it
would guarantee that the security xattrs would never be in an
ea_inode.  (Since in the inode creation case, the only thing they
would be competing with is the acl's, which are lower priority).  So
this reduces the chances of needing to do a lazy extend/restart in the
first place.

> > The downsides of this approach is that it causes the orphan list to be
> > a bottleneck.  So we would definitely not want to do this all time.
> 
> Yes and I think lazy extend/restart should mitigate this.

It mitigates it so long as we the lazy extent/restart is never/rarely
*used*, since that's when we would incur the orphan list overhead.

One other bit about the lazy extend/restart idea is that we need to
make sure that there are enough credits left for the callers of
ext4_new_inode() before it returns.  Otherwise the complexity of this
approach would infect all of the users of this interface (since they
would have to potentially do the extend/restart of the transaction).

						- Ted

Re: More thoughts about xattrs, journal credits, and their location

[Andreas Dilger]

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

On Jul 9, 2017, at 14:01, Tahsin Erdogan <tahsin@google.com> wrote:

>> What we could do is have ext4_new_inode check to see if there are
>> enough credits to do add the xattr's (if necessary) in a single
>> commit.  If not, what we could do is to add the inode to the orphan
>> list, and then set an inode state flag indicating we have done this.
>> At this point, we *can* break the ext4_new_inode() operation into
>> multiple commits, because if we crash in the middle the inode will be
>> cleaned up when we do the orphan list processing.
> 
> This makes sense. Also, we currently add the worst case credit
> estimates of individual set xattr ops and start a journal handle with
> the sum of it. A slight optimization is to do this lazily.
> We can start with enough credits that can get us to a point where it
> is safe to start a new transaction (safe because of orphan addition).
> Then opportunistically extend the credits to get us to the next safe
> point, if that doesn't work, do the orphan add operation and start a
> new transaction. This should handle the worst case scenario and also
> optimize for common case. Also this should in general reduce the
> amount of allocated-but-unused credits which helps parallelism.

What about accumulating the total xattr size in the credits calculation? In
most cases we know the xattr sizes in advance, and if the transaction handle
tracks the total xattr size it can make a good estimate whether the xattrs
will fit in the inode or not rather than using worst-case credits all the time.

>> The downsides of this approach is that it causes the orphan list to be
>> a bottleneck.  So we would definitely not want to do this all time.
> 
> Yes and I think lazy extend/restart should mitigate this.

Jan had a patch to improve the orphan list performance that never made it
into the kernel by having a per-CPU orphan list or similar.

It recall it got hung up on running out of reserved inodes or similar, which
is an issue we should fix in any case.

Cheers, Andreas






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

Re: More thoughts about xattrs, journal credits, and their location

[Jan Kara]

On Fri 14-07-17 15:13:27, Andreas Dilger wrote:
> >> The downsides of this approach is that it causes the orphan list to be
> >> a bottleneck.  So we would definitely not want to do this all time.
> > 
> > Yes and I think lazy extend/restart should mitigate this.
> 
> Jan had a patch to improve the orphan list performance that never made it
> into the kernel by having a per-CPU orphan list or similar.
> 
> It recall it got hung up on running out of reserved inodes or similar, which
> is an issue we should fix in any case.

Yeah, and I've submitted patches for e2fsprogs to be able to set number of
reserved inodes however they never got merged. I guess I can refresh those
patches and resend if there's interest in the functionality and the
feature...

								Honza


-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR