Re: [PATCH 23/28] mbcache: make mbcache more generic

[Jan Kara]

On Thu 15-06-17 11:25:02, Tahsin Erdogan wrote:
> On Thu, Jun 15, 2017 at 12:41 AM, Jan Kara  wrote:
> > Can you explain a bit more what do you mean by "make it more generic" as it
> > seems you just rename a couple of things here...
> 
> The change is really just that, having names that are more generic
> which do not limit use cases to block sharing. In a subsequent patch
> in the series ("[PATCH v4 27/28] ext4: xattr inode deduplication"), we
> start using the mbcache code to share xattr inodes. With that patch,
> old mb_cache_entry.e_block field could be holding either a block
> number or an inode number, so I renamed things to make them more
> generic.

OK, then I'd suggest to change title to "mbcache: make mbcache naming more
generic" and explain what you wrote here in the changelog. Thanks!

Honza
-- 
Jan Kara 
SUSE Labs, CR

Re: [PATCH 23/28] mbcache: make mbcache more generic

[Jan Kara]

On Thu 15-06-17 11:25:02, Tahsin Erdogan wrote:
> On Thu, Jun 15, 2017 at 12:41 AM, Jan Kara  wrote:
> > Can you explain a bit more what do you mean by "make it more generic" as it
> > seems you just rename a couple of things here...
> 
> The change is really just that, having names that are more generic
> which do not limit use cases to block sharing. In a subsequent patch
> in the series ("[PATCH v4 27/28] ext4: xattr inode deduplication"), we
> start using the mbcache code to share xattr inodes. With that patch,
> old mb_cache_entry.e_block field could be holding either a block
> number or an inode number, so I renamed things to make them more
> generic.

OK, then I'd suggest to change title to "mbcache: make mbcache naming more
generic" and explain what you wrote here in the changelog. Thanks!

Honza
-- 
Jan Kara 
SUSE Labs, CR

Re: [PATCH 23/28] mbcache: make mbcache more generic

[Tahsin Erdogan]

On Thu, Jun 15, 2017 at 12:41 AM, Jan Kara  wrote:
> Can you explain a bit more what do you mean by "make it more generic" as it
> seems you just rename a couple of things here...

The change is really just that, having names that are more generic
which do not limit use cases to block sharing. In a subsequent patch
in the series ("[PATCH v4 27/28] ext4: xattr inode deduplication"), we
start using the mbcache code to share xattr inodes. With that patch,
old mb_cache_entry.e_block field could be holding either a block
number or an inode number, so I renamed things to make them more
generic.

Re: [PATCH 23/28] mbcache: make mbcache more generic

[Tahsin Erdogan]

On Thu, Jun 15, 2017 at 12:41 AM, Jan Kara  wrote:
> Can you explain a bit more what do you mean by "make it more generic" as it
> seems you just rename a couple of things here...

The change is really just that, having names that are more generic
which do not limit use cases to block sharing. In a subsequent patch
in the series ("[PATCH v4 27/28] ext4: xattr inode deduplication"), we
start using the mbcache code to share xattr inodes. With that patch,
old mb_cache_entry.e_block field could be holding either a block
number or an inode number, so I renamed things to make them more
generic.

Re: [PATCH 23/28] mbcache: make mbcache more generic

[Jan Kara]

On Wed 31-05-17 01:15:12, Tahsin Erdogan wrote:
> Large xattr feature would like to use the mbcache for xattr value
> deduplication. Current implementation is geared towards xattr block
> deduplication. Make it more generic so that it can be used by both.

Can you explain a bit more what do you mean by "make it more generic" as it
seems you just rename a couple of things here...

Honza

> 
> Signed-off-by: Tahsin Erdogan 
> ---
>  fs/ext2/xattr.c | 18 +-
>  fs/ext4/xattr.c | 10 +-
>  fs/mbcache.c| 43 +--
>  include/linux/mbcache.h | 14 --
>  4 files changed, 43 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index fbdb8f171893..1e5f76070580 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -493,8 +493,8 @@ bad_block:ext2_error(sb, "ext2_xattr_set",
>* This must happen under buffer lock for
>* ext2_xattr_set2() to reliably detect modified block
>*/
> - mb_cache_entry_delete_block(EXT2_SB(sb)->s_mb_cache,
> - hash, bh->b_blocknr);
> + mb_cache_entry_delete(EXT2_SB(sb)->s_mb_cache, hash,
> +   bh->b_blocknr);
>  
>   /* keep the buffer locked while modifying it. */
>   } else {
> @@ -721,8 +721,8 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head 
> *old_bh,
>* This must happen under buffer lock for
>* ext2_xattr_set2() to reliably detect freed block
>*/
> - mb_cache_entry_delete_block(ext2_mb_cache,
> - hash, old_bh->b_blocknr);
> + mb_cache_entry_delete(ext2_mb_cache, hash,
> +   old_bh->b_blocknr);
>   /* Free the old block. */
>   ea_bdebug(old_bh, "freeing");
>   ext2_free_blocks(inode, old_bh->b_blocknr, 1);
> @@ -795,8 +795,8 @@ ext2_xattr_delete_inode(struct inode *inode)
>* This must happen under buffer lock for ext2_xattr_set2() to
>* reliably detect freed block
>*/
> - mb_cache_entry_delete_block(EXT2_SB(inode->i_sb)->s_mb_cache,
> - hash, bh->b_blocknr);
> + mb_cache_entry_delete(EXT2_SB(inode->i_sb)->s_mb_cache, hash,
> +   bh->b_blocknr);
>   ext2_free_blocks(inode, EXT2_I(inode)->i_file_acl, 1);
>   get_bh(bh);
>   bforget(bh);
> @@ -907,11 +907,11 @@ ext2_xattr_cache_find(struct inode *inode, struct 
> ext2_xattr_header *header)
>   while (ce) {
>   struct buffer_head *bh;
>  
> - bh = sb_bread(inode->i_sb, ce->e_block);
> + bh = sb_bread(inode->i_sb, ce->e_value);
>   if (!bh) {
>   ext2_error(inode->i_sb, "ext2_xattr_cache_find",
>   "inode %ld: block %ld read error",
> - inode->i_ino, (unsigned long) ce->e_block);
> + inode->i_ino, (unsigned long) ce->e_value);
>   } else {
>   lock_buffer(bh);
>   /*
> @@ -931,7 +931,7 @@ ext2_xattr_cache_find(struct inode *inode, struct 
> ext2_xattr_header *header)
>   } else if (le32_to_cpu(HDR(bh)->h_refcount) >
>  EXT2_XATTR_REFCOUNT_MAX) {
>   ea_idebug(inode, "block %ld refcount %d>%d",
> -   (unsigned long) ce->e_block,
> +   (unsigned long) ce->e_value,
> le32_to_cpu(HDR(bh)->h_refcount),
> EXT2_XATTR_REFCOUNT_MAX);
>   } else if (!ext2_xattr_cmp(header, HDR(bh))) {
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 886d06e409b6..772948f168c3 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -678,7 +678,7 @@ ext4_xattr_release_block(handle_t *handle, struct inode 
> *inode,
>* This must happen under buffer lock for
>* ext4_xattr_block_set() to reliably detect freed block
>*/
> - mb_cache_entry_delete_block(ext4_mb_cache, hash, bh->b_blocknr);
> + mb_cache_entry_delete(ext4_mb_cache, hash, bh->b_blocknr);
>   get_bh(bh);
>   unlock_buffer(bh);
>   ext4_free_blocks(handle, inode, bh, 0, 1,
> @@ -1115,8 +1115,8 @@ ext4_xattr_block_set(handle_t *handle, struct inode 

Re: [PATCH 23/28] mbcache: make mbcache more generic

[Jan Kara]

On Wed 31-05-17 01:15:12, Tahsin Erdogan wrote:
> Large xattr feature would like to use the mbcache for xattr value
> deduplication. Current implementation is geared towards xattr block
> deduplication. Make it more generic so that it can be used by both.

Can you explain a bit more what do you mean by "make it more generic" as it
seems you just rename a couple of things here...

Honza

> 
> Signed-off-by: Tahsin Erdogan 
> ---
>  fs/ext2/xattr.c | 18 +-
>  fs/ext4/xattr.c | 10 +-
>  fs/mbcache.c| 43 +--
>  include/linux/mbcache.h | 14 --
>  4 files changed, 43 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index fbdb8f171893..1e5f76070580 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -493,8 +493,8 @@ bad_block:ext2_error(sb, "ext2_xattr_set",
>* This must happen under buffer lock for
>* ext2_xattr_set2() to reliably detect modified block
>*/
> - mb_cache_entry_delete_block(EXT2_SB(sb)->s_mb_cache,
> - hash, bh->b_blocknr);
> + mb_cache_entry_delete(EXT2_SB(sb)->s_mb_cache, hash,
> +   bh->b_blocknr);
>  
>   /* keep the buffer locked while modifying it. */
>   } else {
> @@ -721,8 +721,8 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head 
> *old_bh,
>* This must happen under buffer lock for
>* ext2_xattr_set2() to reliably detect freed block
>*/
> - mb_cache_entry_delete_block(ext2_mb_cache,
> - hash, old_bh->b_blocknr);
> + mb_cache_entry_delete(ext2_mb_cache, hash,
> +   old_bh->b_blocknr);
>   /* Free the old block. */
>   ea_bdebug(old_bh, "freeing");
>   ext2_free_blocks(inode, old_bh->b_blocknr, 1);
> @@ -795,8 +795,8 @@ ext2_xattr_delete_inode(struct inode *inode)
>* This must happen under buffer lock for ext2_xattr_set2() to
>* reliably detect freed block
>*/
> - mb_cache_entry_delete_block(EXT2_SB(inode->i_sb)->s_mb_cache,
> - hash, bh->b_blocknr);
> + mb_cache_entry_delete(EXT2_SB(inode->i_sb)->s_mb_cache, hash,
> +   bh->b_blocknr);
>   ext2_free_blocks(inode, EXT2_I(inode)->i_file_acl, 1);
>   get_bh(bh);
>   bforget(bh);
> @@ -907,11 +907,11 @@ ext2_xattr_cache_find(struct inode *inode, struct 
> ext2_xattr_header *header)
>   while (ce) {
>   struct buffer_head *bh;
>  
> - bh = sb_bread(inode->i_sb, ce->e_block);
> + bh = sb_bread(inode->i_sb, ce->e_value);
>   if (!bh) {
>   ext2_error(inode->i_sb, "ext2_xattr_cache_find",
>   "inode %ld: block %ld read error",
> - inode->i_ino, (unsigned long) ce->e_block);
> + inode->i_ino, (unsigned long) ce->e_value);
>   } else {
>   lock_buffer(bh);
>   /*
> @@ -931,7 +931,7 @@ ext2_xattr_cache_find(struct inode *inode, struct 
> ext2_xattr_header *header)
>   } else if (le32_to_cpu(HDR(bh)->h_refcount) >
>  EXT2_XATTR_REFCOUNT_MAX) {
>   ea_idebug(inode, "block %ld refcount %d>%d",
> -   (unsigned long) ce->e_block,
> +   (unsigned long) ce->e_value,
> le32_to_cpu(HDR(bh)->h_refcount),
> EXT2_XATTR_REFCOUNT_MAX);
>   } else if (!ext2_xattr_cmp(header, HDR(bh))) {
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 886d06e409b6..772948f168c3 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -678,7 +678,7 @@ ext4_xattr_release_block(handle_t *handle, struct inode 
> *inode,
>* This must happen under buffer lock for
>* ext4_xattr_block_set() to reliably detect freed block
>*/
> - mb_cache_entry_delete_block(ext4_mb_cache, hash, bh->b_blocknr);
> + mb_cache_entry_delete(ext4_mb_cache, hash, bh->b_blocknr);
>   get_bh(bh);
>   unlock_buffer(bh);
>   ext4_free_blocks(handle, inode, bh, 0, 1,
> @@ -1115,8 +1115,8 @@ ext4_xattr_block_set(handle_t *handle, struct inode 
> *inode,
>