[RFC] ext2: drop cached block when detecting corruption

Currently ext2 uses mdcache for deduplication of extended
attribution blocks. However, there is lack of handling for
corrupted blocks, so newly created EAs may still links to
corrupted blocks. This patch tries to drop cached block
when detecting corruption to mitigate the effect.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/ext2/xattr.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

On 6/3/20 5:44 PM, Chengguang Xu wrote:
> Currently ext2 uses mdcache for deduplication of extended
> attribution blocks. However, there is lack of handling for
> corrupted blocks, so newly created EAs may still links to
> corrupted blocks. This patch tries to drop cached block
> when detecting corruption to mitigate the effect.

ext2_xattr_cmp() will carefully check every entry in the block,
so there is no chance to link to corrupted block, maybe we can
improve the speed of cache related operations by dropping
corrupted blocks.


Thanks,
cgxu

On Jun 3, 2020, at 03:44, Chengguang Xu <cgxu519@mykernel.net> wrote:
> 
> Currently ext2 uses mdcache for deduplication of extended

(typo) this should be "mbcache"


> attribution blocks. However, there is lack of handling for
> corrupted blocks, so newly created EAs may still links to
> corrupted blocks. This patch tries to drop cached block
> when detecting corruption to mitigate the effect.
> 
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
> fs/ext2/xattr.c | 25 ++++++++++++++++++++++---
> 1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 943cc469f42f..969521e39753 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -93,6 +93,8 @@ static int ext2_xattr_set2(struct inode *, struct buffer_head *,
>               struct ext2_xattr_header *);
> 
> static int ext2_xattr_cache_insert(struct mb_cache *, struct buffer_head *);
> +static void ext2_xattr_cache_remove(struct mb_cache *cache,
> +                    struct buffer_head *bh);
> static struct buffer_head *ext2_xattr_cache_find(struct inode *,
>                         struct ext2_xattr_header *);
> static void ext2_xattr_rehash(struct ext2_xattr_header *,
> @@ -237,8 +239,10 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
>    entry = FIRST_ENTRY(bh);
>    while (!IS_LAST_ENTRY(entry)) {
>        if (!ext2_xattr_entry_valid(entry, end,
> -            inode->i_sb->s_blocksize))
> +            inode->i_sb->s_blocksize)) {
> +            ext2_xattr_cache_remove(ea_block_cache, bh);
>            goto bad_block;
> +        }
> 
>        not_found = ext2_xattr_cmp_entry(name_index, name_len, name,
>                         entry);
> @@ -323,8 +327,10 @@ ext2_xattr_list(struct dentry *dentry, char *buffer, size_t buffer_size)
>    entry = FIRST_ENTRY(bh);
>    while (!IS_LAST_ENTRY(entry)) {
>        if (!ext2_xattr_entry_valid(entry, end,
> -            inode->i_sb->s_blocksize))
> +            inode->i_sb->s_blocksize)) {
> +            ext2_xattr_cache_remove(ea_block_cache, bh);
>            goto bad_block;
> +        }
>        entry = EXT2_XATTR_NEXT(entry);
>    }
>    if (ext2_xattr_cache_insert(ea_block_cache, bh))
> @@ -407,6 +413,7 @@ int
> ext2_xattr_set(struct inode *inode, int name_index, const char *name,
>           const void *value, size_t value_len, int flags)
> {
> +    struct mb_cache *ea_block_cache = EA_BLOCK_CACHE(inode);
>    struct super_block *sb = inode->i_sb;
>    struct buffer_head *bh = NULL;
>    struct ext2_xattr_header *header = NULL;
> @@ -464,8 +471,11 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
>         */
>        last = FIRST_ENTRY(bh);
>        while (!IS_LAST_ENTRY(last)) {
> -            if (!ext2_xattr_entry_valid(last, end, sb->s_blocksize))
> +            if (!ext2_xattr_entry_valid(last, end,
> +                sb->s_blocksize)) {
> +                ext2_xattr_cache_remove(ea_block_cache, bh);
>                goto bad_block;
> +            }
>            if (last->e_value_size) {
>                size_t offs = le16_to_cpu(last->e_value_offs);
>                if (offs < min_offs)
> @@ -881,6 +891,15 @@ ext2_xattr_cache_insert(struct mb_cache *cache, struct buffer_head *bh)
>    return error;
> }
> 
> +static void
> +ext2_xattr_cache_remove(struct mb_cache *cache, struct buffer_head *bh)
> +{
> +    lock_buffer(bh);
> +    mb_cache_entry_delete(cache, le32_to_cpu(HDR(bh)->h_hash),
> +                  bh->b_blocknr);
> +    unlock_buffer(bh);
> +}
> +
> /*
>  * ext2_xattr_cmp()
>  *
> -- 
> 2.20.1
> 
>

On Thu 04-06-20 14:26:49, cgxu wrote:
> On 6/3/20 5:44 PM, Chengguang Xu wrote:
> > Currently ext2 uses mdcache for deduplication of extended
> > attribution blocks. However, there is lack of handling for
> > corrupted blocks, so newly created EAs may still links to
> > corrupted blocks. This patch tries to drop cached block
> > when detecting corruption to mitigate the effect.
> 
> ext2_xattr_cmp() will carefully check every entry in the block,
> so there is no chance to link to corrupted block, maybe we can
> improve the speed of cache related operations by dropping
> corrupted blocks.

Thanks for the back but as you write, I don't see the point of your patch
because corrupted blocks shouldn't get to mbcache in the first place (and
we check block consistency only when loading block from disk, not from
mbcache). And performance in case of corrupted xattr block doesn't really
matter much so I don't want to complicate the code for that.

								Honza