Jan Kara
2017-Jun-15 09:10 UTC
[Ocfs2-devel] [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes
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 at 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 at suse.com> SUSE Labs, CR