Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1666087 > unrolled thread

[PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes

Started byTahsin Erdogan <tahsin@google.com>
First post2017-06-14 19:30 +0200
Last post2017-06-19 10:20 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes Tahsin Erdogan <tahsin@google.com> - 2017-06-14 19:30 +0200
    Re: [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation  for removes Jan Kara <jack@suse.cz> - 2017-06-15 11:20 +0200
      Re: [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation  for removes Tahsin Erdogan <tahsin@google.com> - 2017-06-17 04:10 +0200
        Re: [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation  for removes Jan Kara <jack@suse.cz> - 2017-06-19 10:20 +0200

#1666087 — [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes

FromTahsin Erdogan <tahsin@google.com>
Date2017-06-14 19:30 +0200
Subject[PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes
Message-ID<tSkhb-85J-9@gated-at.bofh.it>
When an extended attribute block is modified, ext4_xattr_hash_entry()
recalculates e_hash for the entry that is pointed by s->here. This is
unnecessary if the modification is to remove an entry.

Currently, if the removed entry is the last one and there are other
entries remaining, hash calculation targets the just erased entry which
has been filled with zeroes and effectively does nothing. If the removed
entry is not the last one and there are more entries, this time it will
recalculate hash on the next entry which is totally unnecessary.

Fix these by moving the decision on when to recalculate hash to
ext4_xattr_set_entry().

Signed-off-by: Tahsin Erdogan <tahsin@google.com>
---
 fs/ext4/xattr.c | 50 ++++++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index c9579d220a0c..6c6dce2f874e 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -77,8 +77,9 @@ static void ext4_xattr_block_cache_insert(struct mb_cache *,
 static struct buffer_head *
 ext4_xattr_block_cache_find(struct inode *, struct ext4_xattr_header *,
 			    struct mb_cache_entry **);
-static void ext4_xattr_rehash(struct ext4_xattr_header *,
-			      struct ext4_xattr_entry *);
+static void ext4_xattr_hash_entry(struct ext4_xattr_entry *entry,
+				  void *value_base);
+static void ext4_xattr_rehash(struct ext4_xattr_header *);
 
 static const struct xattr_handler * const ext4_xattr_handler_map[] = {
 	[EXT4_XATTR_INDEX_USER]		     = &ext4_xattr_user_handler,
@@ -1436,7 +1437,8 @@ static int ext4_xattr_inode_lookup_create(handle_t *handle, struct inode *inode,
 
 static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
 				struct ext4_xattr_search *s,
-				handle_t *handle, struct inode *inode)
+				handle_t *handle, struct inode *inode,
+				bool is_block)
 {
 	struct ext4_xattr_entry *last;
 	struct ext4_xattr_entry *here = s->here;
@@ -1500,8 +1502,8 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
 		 * attribute block so that a long value does not occupy the
 		 * whole space and prevent futher entries being added.
 		 */
-		if (ext4_has_feature_ea_inode(inode->i_sb) && new_size &&
-		    (s->end - s->base) == i_blocksize(inode) &&
+		if (ext4_has_feature_ea_inode(inode->i_sb) &&
+		    new_size && is_block &&
 		    (min_offs + old_size - new_size) <
 					EXT4_XATTR_BLOCK_RESERVE(inode)) {
 			ret = -ENOSPC;
@@ -1631,6 +1633,13 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
 		}
 		here->e_value_size = cpu_to_le32(i->value_len);
 	}
+
+	if (is_block) {
+		if (s->not_found || i->value)
+			ext4_xattr_hash_entry(here, s->base);
+		ext4_xattr_rehash((struct ext4_xattr_header *)s->base);
+	}
+
 	ret = 0;
 out:
 	iput(old_ea_inode);
@@ -1720,14 +1729,11 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 			mb_cache_entry_delete(ext4_mb_cache, hash,
 					      bs->bh->b_blocknr);
 			ea_bdebug(bs->bh, "modifying in-place");
-			error = ext4_xattr_set_entry(i, s, handle, inode);
-			if (!error) {
-				if (!IS_LAST_ENTRY(s->first))
-					ext4_xattr_rehash(header(s->base),
-							  s->here);
+			error = ext4_xattr_set_entry(i, s, handle, inode,
+						     true /* is_block */);
+			if (!error)
 				ext4_xattr_block_cache_insert(ext4_mb_cache,
 							      bs->bh);
-			}
 			ext4_xattr_block_csum_set(inode, bs->bh);
 			unlock_buffer(bs->bh);
 			if (error == -EFSCORRUPTED)
@@ -1787,7 +1793,7 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 		s->end = s->base + sb->s_blocksize;
 	}
 
-	error = ext4_xattr_set_entry(i, s, handle, inode);
+	error = ext4_xattr_set_entry(i, s, handle, inode, true /* is_block */);
 	if (error == -EFSCORRUPTED)
 		goto bad_block;
 	if (error)
@@ -1810,9 +1816,6 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 		}
 	}
 
-	if (!IS_LAST_ENTRY(s->first))
-		ext4_xattr_rehash(header(s->base), s->here);
-
 inserted:
 	if (!IS_LAST_ENTRY(s->first)) {
 		new_bh = ext4_xattr_block_cache_find(inode, header(s->base),
@@ -2045,7 +2048,7 @@ 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);
+	error = ext4_xattr_set_entry(i, s, handle, inode, false /* is_block */);
 	if (error) {
 		if (error == -ENOSPC &&
 		    ext4_has_inline_data(inode)) {
@@ -2057,7 +2060,8 @@ int ext4_xattr_ibody_inline_set(handle_t *handle, struct inode *inode,
 			error = ext4_xattr_ibody_find(inode, i, is);
 			if (error)
 				return error;
-			error = ext4_xattr_set_entry(i, s, handle, inode);
+			error = ext4_xattr_set_entry(i, s, handle, inode,
+						     false /* is_block */);
 		}
 		if (error)
 			return error;
@@ -2083,7 +2087,7 @@ static int ext4_xattr_ibody_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);
+	error = ext4_xattr_set_entry(i, s, handle, inode, false /* is_block */);
 	if (error)
 		return error;
 	header = IHDR(inode, ext4_raw_inode(&is->iloc));
@@ -2909,8 +2913,8 @@ ext4_xattr_block_cache_find(struct inode *inode,
  *
  * Compute the hash of an extended attribute.
  */
-static inline void ext4_xattr_hash_entry(struct ext4_xattr_header *header,
-					 struct ext4_xattr_entry *entry)
+static void ext4_xattr_hash_entry(struct ext4_xattr_entry *entry,
+				  void *value_base)
 {
 	__u32 hash = 0;
 	char *name = entry->e_name;
@@ -2923,7 +2927,7 @@ static inline void ext4_xattr_hash_entry(struct ext4_xattr_header *header,
 	}
 
 	if (!entry->e_value_inum && entry->e_value_size) {
-		__le32 *value = (__le32 *)((char *)header +
+		__le32 *value = (__le32 *)((char *)value_base +
 			le16_to_cpu(entry->e_value_offs));
 		for (n = (le32_to_cpu(entry->e_value_size) +
 		     EXT4_XATTR_ROUND) >> EXT4_XATTR_PAD_BITS; n; n--) {
@@ -2945,13 +2949,11 @@ static inline void ext4_xattr_hash_entry(struct ext4_xattr_header *header,
  *
  * Re-compute the extended attribute hash value after an entry has changed.
  */
-static void ext4_xattr_rehash(struct ext4_xattr_header *header,
-			      struct ext4_xattr_entry *entry)
+static void ext4_xattr_rehash(struct ext4_xattr_header *header)
 {
 	struct ext4_xattr_entry *here;
 	__u32 hash = 0;
 
-	ext4_xattr_hash_entry(header, entry);
 	here = ENTRY(header+1);
 	while (!IS_LAST_ENTRY(here)) {
 		if (!here->e_hash) {
-- 
2.13.1.508.gb3defc5cc-goog

[toc] | [next] | [standalone]

#1666589 — Re: [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes

FromJan Kara <jack@suse.cz>
Date2017-06-15 11:20 +0200
SubjectRe: [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes
Message-ID<tSz6x-C2-1@gated-at.bofh.it>
In reply to#1666087
On Wed 14-06-17 10:23:40, Tahsin Erdogan wrote:
> When an extended attribute block is modified, ext4_xattr_hash_entry()
> recalculates e_hash for the entry that is pointed by s->here. This is
> unnecessary if the modification is to remove an entry.
> 
> Currently, if the removed entry is the last one and there are other
> entries remaining, hash calculation targets the just erased entry which
> has been filled with zeroes and effectively does nothing. If the removed
> entry is not the last one and there are more entries, this time it will
> recalculate hash on the next entry which is totally unnecessary.
> 
> Fix these by moving the decision on when to recalculate hash to
> ext4_xattr_set_entry().

I agree with moving ext4_xattr_rehash_entry() out of ext4_xattr_rehash().
However how about just keeping ext4_xattr_rehash() in
ext4_xattr_block_set() (so that you don't have to pass aditional argument
to ext4_xattr_set_entry()) and calling ext4_xattr_rehash_entry() when
i->value != NULL? That would seem easier and cleaner as well...

								Honza
> 
> Signed-off-by: Tahsin Erdogan <tahsin@google.com>
> ---
>  fs/ext4/xattr.c | 50 ++++++++++++++++++++++++++------------------------
>  1 file changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index c9579d220a0c..6c6dce2f874e 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -77,8 +77,9 @@ static void ext4_xattr_block_cache_insert(struct mb_cache *,
>  static struct buffer_head *
>  ext4_xattr_block_cache_find(struct inode *, struct ext4_xattr_header *,
>  			    struct mb_cache_entry **);
> -static void ext4_xattr_rehash(struct ext4_xattr_header *,
> -			      struct ext4_xattr_entry *);
> +static void ext4_xattr_hash_entry(struct ext4_xattr_entry *entry,
> +				  void *value_base);
> +static void ext4_xattr_rehash(struct ext4_xattr_header *);
>  
>  static const struct xattr_handler * const ext4_xattr_handler_map[] = {
>  	[EXT4_XATTR_INDEX_USER]		     = &ext4_xattr_user_handler,
> @@ -1436,7 +1437,8 @@ static int ext4_xattr_inode_lookup_create(handle_t *handle, struct inode *inode,
>  
>  static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
>  				struct ext4_xattr_search *s,
> -				handle_t *handle, struct inode *inode)
> +				handle_t *handle, struct inode *inode,
> +				bool is_block)
>  {
>  	struct ext4_xattr_entry *last;
>  	struct ext4_xattr_entry *here = s->here;
> @@ -1500,8 +1502,8 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
>  		 * attribute block so that a long value does not occupy the
>  		 * whole space and prevent futher entries being added.
>  		 */
> -		if (ext4_has_feature_ea_inode(inode->i_sb) && new_size &&
> -		    (s->end - s->base) == i_blocksize(inode) &&
> +		if (ext4_has_feature_ea_inode(inode->i_sb) &&
> +		    new_size && is_block &&
>  		    (min_offs + old_size - new_size) <
>  					EXT4_XATTR_BLOCK_RESERVE(inode)) {
>  			ret = -ENOSPC;
> @@ -1631,6 +1633,13 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
>  		}
>  		here->e_value_size = cpu_to_le32(i->value_len);
>  	}
> +
> +	if (is_block) {
> +		if (s->not_found || i->value)
> +			ext4_xattr_hash_entry(here, s->base);
> +		ext4_xattr_rehash((struct ext4_xattr_header *)s->base);
> +	}
> +
>  	ret = 0;
>  out:
>  	iput(old_ea_inode);
> @@ -1720,14 +1729,11 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>  			mb_cache_entry_delete(ext4_mb_cache, hash,
>  					      bs->bh->b_blocknr);
>  			ea_bdebug(bs->bh, "modifying in-place");
> -			error = ext4_xattr_set_entry(i, s, handle, inode);
> -			if (!error) {
> -				if (!IS_LAST_ENTRY(s->first))
> -					ext4_xattr_rehash(header(s->base),
> -							  s->here);
> +			error = ext4_xattr_set_entry(i, s, handle, inode,
> +						     true /* is_block */);
> +			if (!error)
>  				ext4_xattr_block_cache_insert(ext4_mb_cache,
>  							      bs->bh);
> -			}
>  			ext4_xattr_block_csum_set(inode, bs->bh);
>  			unlock_buffer(bs->bh);
>  			if (error == -EFSCORRUPTED)
> @@ -1787,7 +1793,7 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>  		s->end = s->base + sb->s_blocksize;
>  	}
>  
> -	error = ext4_xattr_set_entry(i, s, handle, inode);
> +	error = ext4_xattr_set_entry(i, s, handle, inode, true /* is_block */);
>  	if (error == -EFSCORRUPTED)
>  		goto bad_block;
>  	if (error)
> @@ -1810,9 +1816,6 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>  		}
>  	}
>  
> -	if (!IS_LAST_ENTRY(s->first))
> -		ext4_xattr_rehash(header(s->base), s->here);
> -
>  inserted:
>  	if (!IS_LAST_ENTRY(s->first)) {
>  		new_bh = ext4_xattr_block_cache_find(inode, header(s->base),
> @@ -2045,7 +2048,7 @@ 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);
> +	error = ext4_xattr_set_entry(i, s, handle, inode, false /* is_block */);
>  	if (error) {
>  		if (error == -ENOSPC &&
>  		    ext4_has_inline_data(inode)) {
> @@ -2057,7 +2060,8 @@ int ext4_xattr_ibody_inline_set(handle_t *handle, struct inode *inode,
>  			error = ext4_xattr_ibody_find(inode, i, is);
>  			if (error)
>  				return error;
> -			error = ext4_xattr_set_entry(i, s, handle, inode);
> +			error = ext4_xattr_set_entry(i, s, handle, inode,
> +						     false /* is_block */);
>  		}
>  		if (error)
>  			return error;
> @@ -2083,7 +2087,7 @@ static int ext4_xattr_ibody_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);
> +	error = ext4_xattr_set_entry(i, s, handle, inode, false /* is_block */);
>  	if (error)
>  		return error;
>  	header = IHDR(inode, ext4_raw_inode(&is->iloc));
> @@ -2909,8 +2913,8 @@ ext4_xattr_block_cache_find(struct inode *inode,
>   *
>   * Compute the hash of an extended attribute.
>   */
> -static inline void ext4_xattr_hash_entry(struct ext4_xattr_header *header,
> -					 struct ext4_xattr_entry *entry)
> +static void ext4_xattr_hash_entry(struct ext4_xattr_entry *entry,
> +				  void *value_base)
>  {
>  	__u32 hash = 0;
>  	char *name = entry->e_name;
> @@ -2923,7 +2927,7 @@ static inline void ext4_xattr_hash_entry(struct ext4_xattr_header *header,
>  	}
>  
>  	if (!entry->e_value_inum && entry->e_value_size) {
> -		__le32 *value = (__le32 *)((char *)header +
> +		__le32 *value = (__le32 *)((char *)value_base +
>  			le16_to_cpu(entry->e_value_offs));
>  		for (n = (le32_to_cpu(entry->e_value_size) +
>  		     EXT4_XATTR_ROUND) >> EXT4_XATTR_PAD_BITS; n; n--) {
> @@ -2945,13 +2949,11 @@ static inline void ext4_xattr_hash_entry(struct ext4_xattr_header *header,
>   *
>   * Re-compute the extended attribute hash value after an entry has changed.
>   */
> -static void ext4_xattr_rehash(struct ext4_xattr_header *header,
> -			      struct ext4_xattr_entry *entry)
> +static void ext4_xattr_rehash(struct ext4_xattr_header *header)
>  {
>  	struct ext4_xattr_entry *here;
>  	__u32 hash = 0;
>  
> -	ext4_xattr_hash_entry(header, entry);
>  	here = ENTRY(header+1);
>  	while (!IS_LAST_ENTRY(here)) {
>  		if (!here->e_hash) {
> -- 
> 2.13.1.508.gb3defc5cc-goog
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1668131 — Re: [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes

FromTahsin Erdogan <tahsin@google.com>
Date2017-06-17 04:10 +0200
SubjectRe: [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes
Message-ID<tTblv-ho-1@gated-at.bofh.it>
In reply to#1666589
On Thu, Jun 15, 2017 at 2:10 AM, Jan Kara <jack@suse.cz> wrote:
> I agree with moving ext4_xattr_rehash_entry() out of ext4_xattr_rehash().
> However how about just keeping ext4_xattr_rehash() in
> ext4_xattr_block_set() (so that you don't have to pass aditional argument
> to ext4_xattr_set_entry()) and calling ext4_xattr_rehash_entry() when
> i->value != NULL? That would seem easier and cleaner as well...

The is_block parameter is also used to decide whether block reserve
check should be performed:

@@ -1500,8 +1502,8 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
                 * attribute block so that a long value does not occupy the
                 * whole space and prevent futher entries being added.
                 */
-               if (ext4_has_feature_ea_inode(inode->i_sb) && new_size &&
-                   (s->end - s->base) == i_blocksize(inode) &&
+               if (ext4_has_feature_ea_inode(inode->i_sb) &&
+                   new_size && is_block &&
                    (min_offs + old_size - new_size) <
                                        EXT4_XATTR_BLOCK_RESERVE(inode)) {
                        ret = -ENOSPC;

Because of that, I think moving ext4_xattr_rehash to caller makes it
bit more complicated. Let me know if you disagree.

[toc] | [prev] | [next] | [standalone]

#1668888 — Re: [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes

FromJan Kara <jack@suse.cz>
Date2017-06-19 10:20 +0200
SubjectRe: [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes
Message-ID<tU04F-7z-3@gated-at.bofh.it>
In reply to#1668131
On Fri 16-06-17 19:04:44, Tahsin Erdogan wrote:
> On Thu, Jun 15, 2017 at 2:10 AM, Jan Kara <jack@suse.cz> wrote:
> > I agree with moving ext4_xattr_rehash_entry() out of ext4_xattr_rehash().
> > However how about just keeping ext4_xattr_rehash() in
> > ext4_xattr_block_set() (so that you don't have to pass aditional argument
> > to ext4_xattr_set_entry()) and calling ext4_xattr_rehash_entry() when
> > i->value != NULL? That would seem easier and cleaner as well...
> 
> The is_block parameter is also used to decide whether block reserve
> check should be performed:
> 
> @@ -1500,8 +1502,8 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
>                  * attribute block so that a long value does not occupy the
>                  * whole space and prevent futher entries being added.
>                  */
> -               if (ext4_has_feature_ea_inode(inode->i_sb) && new_size &&
> -                   (s->end - s->base) == i_blocksize(inode) &&
> +               if (ext4_has_feature_ea_inode(inode->i_sb) &&
> +                   new_size && is_block &&
>                     (min_offs + old_size - new_size) <
>                                         EXT4_XATTR_BLOCK_RESERVE(inode)) {
>                         ret = -ENOSPC;
> 
> Because of that, I think moving ext4_xattr_rehash to caller makes it
> bit more complicated. Let me know if you disagree.

What I dislike is the leakage of information about particular type of
storage into ext4_xattr_set_entry(). However I agree that it would be
cumbersome to handle this reservation check differently so ok.

								Honza

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web