On Jun 29, 2017, at 7:36 PM, Tahsin Erdogan <tahsin@xxxxxxxxxx> wrote:
>
> Current way of determining whether a symlink is in fast symlink
> format is to call ext2fs_inode_data_blocks2(). If number of data
> blocks is zero and EXT4_INLINE_DATA_FL flag is not set, then symlink
> data must be in inode->i_block.
>
> This heuristic is becoming increasingly hard to maintain because
> inode->i_blocks count can also be incremented for blocks used by
> extended attributes. Before ea_inode feature, extra block could come
> from xattr block, now more blocks can be added because of xattr
> inodes.
>
> To address the issue, add a ext2fs_is_fast_symlink() function that
> gives a direct answer based on inode->i_size field. This is
> equivalent to kernel's ext4_inode_is_fast_symlink() function.
>
> This patch also fixes a few issues related to fast symlink handling:
>
> - Both rdump_symlink() and follow_link() interpreted symlinks with
> 0 data blocks to always mean fast symlinks. This is incorrect
> because symlinks that are stored as inline data also have
> 0 data blocks. Thus, they try to read everything from
> inode->i_block and miss the symlink suffix in inode extra area.
>
> - e2fsck_pass1_check_symlink() had code to handle inode with
> EXT4_INLINE_DATA_FL flag twice. The first if block always returns
> from the function so the second one is unreachable code.
This looks mostly good, one question below.
> Suggested-by: Andreas Dilger <adilger@xxxxxxxxx>
> Signed-off-by: Tahsin Erdogan <tahsin@xxxxxxxxxx>
> ---
> v2: Added Suggested-by to commit message
>
> debugfs/debugfs.c | 55 +++++++++++++++++++++++-----------------------------
> debugfs/dump.c | 4 +---
> e2fsck/pass1.c | 42 ++++++++-------------------------------
> lib/ext2fs/alloc.c | 9 +++++----
> lib/ext2fs/ext2fs.h | 1 +
> lib/ext2fs/namei.c | 20 ++++++++++++++++---
> lib/ext2fs/swapfs.c | 46 +++++++++++++++++++++----------------------
> lib/ext2fs/symlink.c | 11 +++++++++++
> misc/fuse2fs.c | 9 ++++-----
> 9 files changed, 94 insertions(+), 103 deletions(-)
>
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 422a3d699111..dd650427795c 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
>
> @@ -221,12 +220,15 @@ int e2fsck_pass1_check_symlink(ext2_filsys fs, ext2_ino_t ino,
> return 1;
> }
>
> - blocks = ext2fs_inode_data_blocks2(fs, inode);
> - if (blocks) {
> - if (inode->i_flags & EXT4_INLINE_DATA_FL)
> + if (ext2fs_is_fast_symlink(inode)) {
> + if (inode->i_size >= sizeof(inode->i_block))
> + return 0;
If this is a fast symlink, which is now determined by the file size, I don't see
how this i_size check can ever be true?
> +
> + len = strnlen((char *)inode->i_block, sizeof(inode->i_block));
> + if (len == sizeof(inode->i_block))
> return 0;
> + } else {
> if ((inode->i_size >= fs->blocksize) ||
> (inode->i_block[0] < fs->super->s_first_data_block) ||
> (inode->i_block[0] >= ext2fs_blocks_count(fs->super)))
> return 0;
> diff --git a/lib/ext2fs/symlink.c b/lib/ext2fs/symlink.c
> index 0e6f9a9fd14e..271aa1ccc134 100644
> --- a/lib/ext2fs/symlink.c
> +++ b/lib/ext2fs/symlink.c
> @@ -174,3 +174,14 @@ cleanup:
> ext2fs_free_mem(&block_buf);
> return retval;
> }
> +
> +/*
> + * Test whether an inode is a fast symlink.
> + *
> + * A fast symlink has its symlink data stored in inode->i_block.
> + */
> +int ext2fs_is_fast_symlink(struct ext2_inode *inode)
> +{
> + return LINUX_S_ISLNK(inode->i_mode) && EXT2_I_SIZE(inode) &&
> + EXT2_I_SIZE(inode) < sizeof(inode->i_block);
> +}
> \ No newline at end of file
Need to add newline here?
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
