From: Jan Kara <jack@suse.cz>
To: Tahsin Erdogan <tahsin@google.com>
Cc: Jan Kara <jack@suse.com>, Theodore Ts'o <tytso@mit.edu>,
Andreas Dilger <adilger.kernel@dilger.ca>,
Dave Kleikamp <shaggy@kernel.org>,
Alexander Viro <viro@zeniv.linux.org.uk>,
Mark Fasheh <mfasheh@versity.com>,
Joel Becker <jlbec@evilplan.org>, Jens Axboe <axboe@fb.com>,
Deepa Dinamani <deepa.kernel@gmail.com>,
Mike Christie <mchristi@redhat.com>,
Fabian Frederick <fabf@skynet.be>,
linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
jfs-discussion@lists.sourceforge.net,
linux-fsdevel@vger.kernel.org, ocfs2-devel@oss.oracle.com,
reiserfs-devel@vger.kernel.org
Subject: Re: [PATCH 23/28] mbcache: make mbcache more generic
Date: Thu, 15 Jun 2017 09:41:58 +0200 [thread overview]
Message-ID: <20170615074158.GA1764@quack2.suse.cz> (raw)
In-Reply-To: <20170531081517.11438-23-tahsin@google.com>
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 <tahsin@google.com>
> ---
> 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,
> * ext4_xattr_block_set() to reliably detect modified
> * block
> */
> - mb_cache_entry_delete_block(ext4_mb_cache, hash,
> - bs->bh->b_blocknr);
> + 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) {
> @@ -2238,10 +2238,10 @@ ext4_xattr_cache_find(struct inode *inode, struct ext4_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) {
> EXT4_ERROR_INODE(inode, "block %lu read error",
> - (unsigned long) ce->e_block);
> + (unsigned long) ce->e_value);
> } else if (ext4_xattr_cmp(header, BHDR(bh)) == 0) {
> *pce = ce;
> return bh;
> diff --git a/fs/mbcache.c b/fs/mbcache.c
> index b19be429d655..77a5b99d8f92 100644
> --- a/fs/mbcache.c
> +++ b/fs/mbcache.c
> @@ -10,7 +10,7 @@
> /*
> * Mbcache is a simple key-value store. Keys need not be unique, however
> * key-value pairs are expected to be unique (we use this fact in
> - * mb_cache_entry_delete_block()).
> + * mb_cache_entry_delete()).
> *
> * Ext2 and ext4 use this cache for deduplication of extended attribute blocks.
> * They use hash of a block contents as a key and block number as a value.
> @@ -62,15 +62,15 @@ static inline struct hlist_bl_head *mb_cache_entry_head(struct mb_cache *cache,
> * @cache - cache where the entry should be created
> * @mask - gfp mask with which the entry should be allocated
> * @key - key of the entry
> - * @block - block that contains data
> - * @reusable - is the block reusable by other inodes?
> + * @value - value of the entry
> + * @reusable - is the entry reusable by others?
> *
> - * Creates entry in @cache with key @key and records that data is stored in
> - * block @block. The function returns -EBUSY if entry with the same key
> - * and for the same block already exists in cache. Otherwise 0 is returned.
> + * Creates entry in @cache with key @key and value @value. The function returns
> + * -EBUSY if entry with the same key and value already exists in cache.
> + * Otherwise 0 is returned.
> */
> int mb_cache_entry_create(struct mb_cache *cache, gfp_t mask, u32 key,
> - sector_t block, bool reusable)
> + cache_value_t value, bool reusable)
> {
> struct mb_cache_entry *entry, *dup;
> struct hlist_bl_node *dup_node;
> @@ -91,12 +91,12 @@ int mb_cache_entry_create(struct mb_cache *cache, gfp_t mask, u32 key,
> /* One ref for hash, one ref returned */
> atomic_set(&entry->e_refcnt, 1);
> entry->e_key = key;
> - entry->e_block = block;
> + entry->e_value = value;
> entry->e_reusable = reusable;
> head = mb_cache_entry_head(cache, key);
> hlist_bl_lock(head);
> hlist_bl_for_each_entry(dup, dup_node, head, e_hash_list) {
> - if (dup->e_key == key && dup->e_block == block) {
> + if (dup->e_key == key && dup->e_value == value) {
> hlist_bl_unlock(head);
> kmem_cache_free(mb_entry_cache, entry);
> return -EBUSY;
> @@ -187,13 +187,13 @@ struct mb_cache_entry *mb_cache_entry_find_next(struct mb_cache *cache,
> EXPORT_SYMBOL(mb_cache_entry_find_next);
>
> /*
> - * mb_cache_entry_get - get a cache entry by block number (and key)
> + * mb_cache_entry_get - get a cache entry by value (and key)
> * @cache - cache we work with
> - * @key - key of block number @block
> - * @block - block number
> + * @key - key
> + * @value - value
> */
> struct mb_cache_entry *mb_cache_entry_get(struct mb_cache *cache, u32 key,
> - sector_t block)
> + cache_value_t value)
> {
> struct hlist_bl_node *node;
> struct hlist_bl_head *head;
> @@ -202,7 +202,7 @@ struct mb_cache_entry *mb_cache_entry_get(struct mb_cache *cache, u32 key,
> head = mb_cache_entry_head(cache, key);
> hlist_bl_lock(head);
> hlist_bl_for_each_entry(entry, node, head, e_hash_list) {
> - if (entry->e_key == key && entry->e_block == block) {
> + if (entry->e_key == key && entry->e_value == value) {
> atomic_inc(&entry->e_refcnt);
> goto out;
> }
> @@ -214,15 +214,14 @@ struct mb_cache_entry *mb_cache_entry_get(struct mb_cache *cache, u32 key,
> }
> EXPORT_SYMBOL(mb_cache_entry_get);
>
> -/* mb_cache_entry_delete_block - remove information about block from cache
> +/* mb_cache_entry_delete - remove a cache entry
> * @cache - cache we work with
> - * @key - key of block @block
> - * @block - block number
> + * @key - key
> + * @value - value
> *
> - * Remove entry from cache @cache with key @key with data stored in @block.
> + * Remove entry from cache @cache with key @key and value @value.
> */
> -void mb_cache_entry_delete_block(struct mb_cache *cache, u32 key,
> - sector_t block)
> +void mb_cache_entry_delete(struct mb_cache *cache, u32 key, cache_value_t value)
> {
> struct hlist_bl_node *node;
> struct hlist_bl_head *head;
> @@ -231,7 +230,7 @@ void mb_cache_entry_delete_block(struct mb_cache *cache, u32 key,
> head = mb_cache_entry_head(cache, key);
> hlist_bl_lock(head);
> hlist_bl_for_each_entry(entry, node, head, e_hash_list) {
> - if (entry->e_key == key && entry->e_block == block) {
> + if (entry->e_key == key && entry->e_value == value) {
> /* We keep hash list reference to keep entry alive */
> hlist_bl_del_init(&entry->e_hash_list);
> hlist_bl_unlock(head);
> @@ -248,7 +247,7 @@ void mb_cache_entry_delete_block(struct mb_cache *cache, u32 key,
> }
> hlist_bl_unlock(head);
> }
> -EXPORT_SYMBOL(mb_cache_entry_delete_block);
> +EXPORT_SYMBOL(mb_cache_entry_delete);
>
> /* mb_cache_entry_touch - cache entry got used
> * @cache - cache the entry belongs to
> diff --git a/include/linux/mbcache.h b/include/linux/mbcache.h
> index 86c9a8b480c5..e2d9f2f926a4 100644
> --- a/include/linux/mbcache.h
> +++ b/include/linux/mbcache.h
> @@ -9,6 +9,8 @@
>
> struct mb_cache;
>
> +typedef sector_t cache_value_t;
> +
> struct mb_cache_entry {
> /* List of entries in cache - protected by cache->c_list_lock */
> struct list_head e_list;
> @@ -19,15 +21,15 @@ struct mb_cache_entry {
> u32 e_key;
> u32 e_referenced:1;
> u32 e_reusable:1;
> - /* Block number of hashed block - stable during lifetime of the entry */
> - sector_t e_block;
> + /* User provided value - stable during lifetime of the entry */
> + cache_value_t e_value;
> };
>
> struct mb_cache *mb_cache_create(int bucket_bits);
> void mb_cache_destroy(struct mb_cache *cache);
>
> int mb_cache_entry_create(struct mb_cache *cache, gfp_t mask, u32 key,
> - sector_t block, bool reusable);
> + cache_value_t value, bool reusable);
> void __mb_cache_entry_free(struct mb_cache_entry *entry);
> static inline int mb_cache_entry_put(struct mb_cache *cache,
> struct mb_cache_entry *entry)
> @@ -38,10 +40,10 @@ static inline int mb_cache_entry_put(struct mb_cache *cache,
> return 1;
> }
>
> -void mb_cache_entry_delete_block(struct mb_cache *cache, u32 key,
> - sector_t block);
> +void mb_cache_entry_delete(struct mb_cache *cache, u32 key,
> + cache_value_t value);
> struct mb_cache_entry *mb_cache_entry_get(struct mb_cache *cache, u32 key,
> - sector_t block);
> + cache_value_t value);
> struct mb_cache_entry *mb_cache_entry_find_first(struct mb_cache *cache,
> u32 key);
> struct mb_cache_entry *mb_cache_entry_find_next(struct mb_cache *cache,
> --
> 2.13.0.219.gdb65acc882-goog
>
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2017-06-15 7:41 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-31 8:14 [PATCH 01/28] ext4: xattr-in-inode support Tahsin Erdogan
2017-05-31 8:14 ` [PATCH 02/28] ext4: fix lockdep warning about recursive inode locking Tahsin Erdogan
2017-05-31 8:14 ` [PATCH 03/28] ext4: lock inode before calling ext4_orphan_add() Tahsin Erdogan
2017-05-31 8:14 ` [PATCH 04/28] ext4: do not set posix acls on xattr inodes Tahsin Erdogan
2017-05-31 8:14 ` [PATCH 05/28] ext4: attach jinode after creation of xattr inode Tahsin Erdogan
2017-05-31 8:14 ` [PATCH 06/28] ext4: ea_inode owner should be the same as the inode owner Tahsin Erdogan
2017-05-31 8:14 ` [PATCH 07/28] ext4: call journal revoke when freeing ea_inode blocks Tahsin Erdogan
2017-05-31 16:12 ` Darrick J. Wong
2017-05-31 21:01 ` Tahsin Erdogan
2017-05-31 8:14 ` [PATCH 08/28] ext4: fix ref counting for ea_inode Tahsin Erdogan
2017-05-31 8:14 ` [PATCH 09/28] ext4: extended attribute value size limit is enforced by vfs Tahsin Erdogan
2017-05-31 16:03 ` Darrick J. Wong
2017-05-31 16:13 ` Tahsin Erdogan
2017-05-31 8:14 ` [PATCH 10/28] ext4: change ext4_xattr_inode_iget() signature Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 11/28] ext4: clean up ext4_xattr_inode_get() Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 12/28] ext4: add missing le32_to_cpu(e_value_inum) conversions Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 13/28] ext4: ext4_xattr_value_same() should return false for external data Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 14/28] ext4: fix ext4_xattr_make_inode_space() value size calculation Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 15/28] ext4: fix ext4_xattr_move_to_block() Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 16/28] ext4: fix ext4_xattr_cmp() Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 17/28] ext4: fix credits calculation for xattr inode Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 18/28] ext4: retry storing value in external inode with xattr block too Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 19/28] ext4: ext4_xattr_delete_inode() should return accurate errors Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 20/28] ext4: improve journal credit handling in set xattr paths Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 21/28] ext4: modify ext4_xattr_ino_array to hold struct inode * Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 22/28] ext4: move struct ext4_xattr_inode_array to xattr.h Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 23/28] mbcache: make mbcache more generic Tahsin Erdogan
2017-06-15 7:41 ` Jan Kara [this message]
2017-06-15 18:25 ` Tahsin Erdogan
2017-06-19 8:50 ` Jan Kara
2017-05-31 8:15 ` [PATCH 24/28] ext4: rename mb block cache functions Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 25/28] ext4: add ext4_is_quota_file() Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 26/28] ext4: cleanup transaction restarts during inode deletion Tahsin Erdogan
2017-06-14 14:17 ` [PATCH v2 " Tahsin Erdogan
2017-06-15 0:11 ` Andreas Dilger
2017-05-31 8:15 ` [PATCH 27/28] ext4: xattr inode deduplication Tahsin Erdogan
2017-05-31 15:40 ` kbuild test robot
2017-05-31 15:50 ` kbuild test robot
2017-05-31 16:00 ` Darrick J. Wong
2017-05-31 22:33 ` [PATCH v2 " Tahsin Erdogan
2017-06-02 5:41 ` Darrick J. Wong
2017-06-02 12:46 ` Tahsin Erdogan
2017-06-02 17:59 ` Darrick J. Wong
2017-06-02 23:35 ` [PATCH v3 " Tahsin Erdogan
2017-06-14 14:34 ` [PATCH v4 " Tahsin Erdogan
2017-05-31 8:15 ` [PATCH 28/28] quota: add extra inode count to dquot transfer functions Tahsin Erdogan
2017-06-15 7:57 ` Jan Kara
2017-06-17 1:50 ` Tahsin Erdogan
2017-06-19 9:03 ` Jan Kara
2017-06-19 11:46 ` Tahsin Erdogan
2017-06-19 12:36 ` Jan Kara
2017-06-20 9:53 ` Tahsin Erdogan
2017-05-31 16:42 ` [PATCH 01/28] ext4: xattr-in-inode support Darrick J. Wong
2017-05-31 19:59 ` Tahsin Erdogan
2017-06-01 15:50 ` [PATCH v2 " Tahsin Erdogan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170615074158.GA1764@quack2.suse.cz \
--to=jack@suse.cz \
--cc=adilger.kernel@dilger.ca \
--cc=axboe@fb.com \
--cc=deepa.kernel@gmail.com \
--cc=fabf@skynet.be \
--cc=jack@suse.com \
--cc=jfs-discussion@lists.sourceforge.net \
--cc=jlbec@evilplan.org \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mchristi@redhat.com \
--cc=mfasheh@versity.com \
--cc=ocfs2-devel@oss.oracle.com \
--cc=reiserfs-devel@vger.kernel.org \
--cc=shaggy@kernel.org \
--cc=tahsin@google.com \
--cc=tytso@mit.edu \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).