Re: [PATCH] ext4: fast symlink test should not rely on i_blocks

[Andreas Dilger]

> On Jun 27, 2017, at 6:34 PM, Tahsin Erdogan <tahsin@xxxxxxxxxx> wrote:
> 
> ext4_inode_info->i_data is the storage area for 4 types of data:
> 
>  a) Extents data
>  b) Inline data
>  c) Block map
>  d) Fast symlink data (symlink length < 60)
> 
> Extents data case is positively identified by EXT4_INODE_EXTENTS flag.
> Inline data case is also obvious because of EXT4_INODE_INLINE_DATA
> flag.
> 
> Distinguishing c) and d) however requires additional logic. This
> currently relies on i_blocks count. After subtracting external xattr
> block from i_blocks, if it is greater than 0 then we know that some
> data blocks exist, so there must be a block map.
> 
> This logic got broken after ea_inode feature was added. That feature
> charges the data blocks of external xattr inodes to the referencing
> inode and so adds them to the i_blocks. To fix this, we could subtract
> ea_inode blocks by iterating through all xattr entries and then check
> whether remaining i_blocks count is zero. Besides being complicated,
> this won't change the fact that the current way of distinguishing
> between c) and d) is fragile.
> 
> The alternative solution is to test whether i_size is less than 60 to
> determine fast symlink case. ext4_symlink() uses the same test to decide
> whether to store the symlink in i_data. There is one caveat to address
> before this can work though.
> 
> If an inode's i_nlink is zero during eviction, its i_size is set to
> zero and its data is truncated. If system crashes before inode is removed
> from the orphan list, next boot orphan cleanup may find the inode with
> zero i_size. So, a symlink that had its data stored in a block may now
> appear to be a fast symlink. The solution used in this patch is to treat
> i_size = 0 as a non-fast symlink case. A zero sized symlink is not legal
> so the only time this can happen is the mentioned scenario. This is also
> logically correct because a i_size = 0 symlink has no data stored in
> i_data.
> 
> Fixes: 74c5bfa651af ("ext4: xattr inode deduplication")
> 
> Suggested-by: Andreas Dilger <adilger@xxxxxxxxx>
> Signed-off-by: Tahsin Erdogan <tahsin@xxxxxxxxxx>

The unfortunate bit is that this makes the inode impossible to undelete, but
I don't think that is a huge concern for symlinks.

Reviewed-by: Andreas Dilger <adilger@xxxxxxxxx>

> ---
> fs/ext4/inode.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index daed9b38362a..3c600f02673f 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -144,16 +144,12 @@ static int ext4_meta_trans_blocks(struct inode *inode, int lblocks,
> 
> /*
>  * Test whether an inode is a fast symlink.
> + * A fast symlink has its symlink data stored in ext4_inode_info->i_data.
>  */
> int ext4_inode_is_fast_symlink(struct inode *inode)
> {
> -        int ea_blocks = EXT4_I(inode)->i_file_acl ?
> -		EXT4_CLUSTER_SIZE(inode->i_sb) >> 9 : 0;
> -
> -	if (ext4_has_inline_data(inode))
> -		return 0;
> -
> -	return (S_ISLNK(inode->i_mode) && inode->i_blocks - ea_blocks == 0);
> +	return S_ISLNK(inode->i_mode) && inode->i_size &&
> +	       (inode->i_size < EXT4_N_BLOCKS * 4);
> }
> 
> /*
> @@ -261,6 +257,16 @@ void ext4_evict_inode(struct inode *inode)
> 
> 	if (IS_SYNC(inode))
> 		ext4_handle_sync(handle);
> +
> +	/*
> +	 * Set inode->i_size to 0 before calling ext4_truncate(). We need
> +	 * special handling of symlinks here because i_size is used to
> +	 * determine whether ext4_inode_info->i_data contains symlink data or
> +	 * block mappings. Setting i_size to 0 will remove its fast symlink
> +	 * status. Erase i_data so that it becomes a valid empty block map.
> +	 */
> +	if (ext4_inode_is_fast_symlink(inode))
> +		memset(EXT4_I(inode)->i_data, 0, sizeof(EXT4_I(inode)->i_data));
> 	inode->i_size = 0;
> 	err = ext4_mark_inode_dirty(handle, inode);
> 	if (err) {
> --
> 2.13.2.725.g09c95d1e9-goog
> 


Cheers, Andreas

Attachment: signature.asc
Description: Message signed with OpenPGP