On Jun 26, 2017, at 7:43 AM, Tahsin Erdogan <tahsin@xxxxxxxxxx> wrote:
>
> In original implementation of ea_inode feature, each xattr inode had
> a single parent. Child inode tracked the parent by storing its inode
> number in i_mtime field. Also, child's i_generation matched parent's
> i_generation.
>
> With deduplication support, xattr inodes can now be shared so a single
> backpointer is not sufficient to achieve strong binding. This is now
> replaced by hash validation.
>
> Signed-off-by: Tahsin Erdogan <tahsin@xxxxxxxxxx>
> ---
> e2fsck/pass1.c | 91 +++++++++++++++++++++++++++++++++------------------
> e2fsck/problem.c | 5 ---
> e2fsck/problem.h | 7 ++--
> lib/ext2fs/ext2fs.h | 3 ++
> lib/ext2fs/ext_attr.c | 67 +++++++++++++++++++++++++++++++++++--
> 5 files changed, 128 insertions(+), 45 deletions(-)
>
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 1c68af8990b4..32152f3ec926 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -2439,6 +2466,16 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
> pctx))
> goto clear_extattr;
> }
> +
> + hash = ext2fs_ext_attr_hash_entry(entry, block_buf +
> + entry->e_value_offs);
> +
> + if (entry->e_hash != hash) {
> + pctx->num = entry->e_hash;
> + if (fix_problem(ctx, PR_1_ATTR_HASH, pctx))
> + goto clear_extattr;
Shouldn't this be "if (!fix_problem(...))" ?
> diff --git a/e2fsck/problem.h b/e2fsck/problem.h
> index dad608c94fd0..8e07c10895c1 100644
> --- a/e2fsck/problem.h
> +++ b/e2fsck/problem.h
> @@ -678,15 +678,12 @@ struct problem_context {
> /* Inode has illegal EA value inode */
> #define PR_1_ATTR_VALUE_EA_INODE 0x010083
>
> -/* Invalid backpointer from EA inode to parent inode */
> -#define PR_1_ATTR_INVAL_EA_INODE 0x010084
> -
> /* Parent inode has invalid EA entry. EA inode does not have
> * EXT4_EA_INODE_FL flag. Delete EA entry? */
> -#define PR_1_ATTR_NO_EA_INODE_FL 0x010085
> +#define PR_1_ATTR_NO_EA_INODE_FL 0x010084
>
> /* EA inode for parent inode does not have EXT4_EA_INODE_FL flag */
> -#define PR_1_ATTR_SET_EA_INODE_FL 0x010086
> +#define PR_1_ATTR_SET_EA_INODE_FL 0x010085
It would also be OK to keep the old values, and just skip 0x10084.
> diff --git a/lib/ext2fs/ext_attr.c b/lib/ext2fs/ext_attr.c
> index 14d05c7e57fb..d48ab5c55270 100644
> --- a/lib/ext2fs/ext_attr.c
> +++ b/lib/ext2fs/ext_attr.c
> @@ -914,9 +955,29 @@ static errcode_t read_xattrs_from_buffer(struct ext2_xattr_handle *handle,
> void *data = (entry->e_value_inum != 0) ?
> 0 : value_start + entry->e_value_offs;
>
> - hash = ext2fs_ext_attr_hash_entry(entry, data);
> - if (entry->e_hash != hash)
> - return EXT2_ET_BAD_EA_HASH;
> + err = ext2fs_ext_attr_hash_entry2(handle->fs, entry,
> + data, &hash);
> + if (err)
> + return err;
> + if (entry->e_hash != hash) {
> + struct ext2_inode parent, child;
> +
> + /* Check whether this is an old Lustre-style
> + * ea_inode reference.
> + */
> + err = ext2fs_read_inode(handle->fs, handle->ino,
> + &parent);
Rather than reading the inode again here, it would make more sense to pass the
parent inode from the caller.
> + if (err)
> + return err;
> + err = ext2fs_read_inode(handle->fs,
> + entry->e_value_inum,
> + &child);
> + if (err)
> + return err;
> + if (child.i_mtime != handle->ino ||
> + child.i_generation != parent.i_generation)
> + return EXT2_ET_BAD_EA_HASH;
> + }
> }
>
> x++;
> --
> 2.13.1.611.g7e3b11ae1-goog
>
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
