Re: [Jfs-discussion] [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes

2017-06-19 Thread Jan Kara
On Fri 16-06-17 19:04:44, Tahsin Erdogan wrote:
> On Thu, Jun 15, 2017 at 2:10 AM, Jan Kara  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 
SUSE Labs, CR

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Jfs-discussion mailing list
Jfs-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/jfs-discussion


Re: [Jfs-discussion] [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes

2017-06-16 Thread Tahsin Erdogan via Jfs-discussion
On Thu, Jun 15, 2017 at 2:10 AM, Jan Kara  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.

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Jfs-discussion mailing list
Jfs-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/jfs-discussion


Re: [Jfs-discussion] [PATCH 30/31] ext4: eliminate xattr entry e_hash recalculation for removes

2017-06-15 Thread Jan Kara
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 
> ---
>  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]  = _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,
>