Re: [PATCH 29/31] ext4: reserve space for xattr entries/names

On Wed, Jun 14, 2017 at 10:23:26AM -0700, Tahsin Erdogan wrote:
> New ea_inode feature allows putting large xattr values into external
> inodes. struct ext4_xattr_entry and the attribute name however have to
> remain in the inode extra space or external attribute block. Once that
> space is exhausted, no further entries can be added. Some of that space
> could also be used by values that fit in there at the time of addition.
> 
> So, a single xattr entry whose value barely fits in the external block
> could prevent further entries being added.
> 
> To mitigate the problem, this patch introduces a notion of reserve in
> the
> external attribute block that cannot be used by value data. This reserve
> is enforced when ea_inode feature is enabled. The amount of reserve is
> arbitrarily chosen to be min(block_size/8, 1024). The table below shows
> how much space is reserved for each block size and the guaranteed
> mininum
> number of entries that can be placed in the external attribute block.
> 
> block size     reserved bytes  entries (name length = 16)
>  1k            128              3
>  2k            256              7
>  4k            512             15

Why not just spill the values into their own ea_inodes if we need the
space?  I guess that has the disadvantage that now we need to reserve
quite a few more journal credits ((1 inode block, 1 bbitmap block, 1
ibitmap block, 1 data block) * nr_inline_values) just in case we end up
spilling all the values.

--D

>  8k            1024            31
> 16k            1024            31
> 32k            1024            31
> 64k            1024            31
> 
> Signed-off-by: Tahsin Erdogan <[email protected]>
> ---
>  fs/ext4/xattr.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 3ad1fc62cbf0..c9579d220a0c 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1428,6 +1428,12 @@ static int ext4_xattr_inode_lookup_create(handle_t *handle, struct inode *inode,
>  	return 0;
>  }
>  
> +/*
> + * Reserve min(block_size/8, 1024) bytes for xattr entries/names if ea_inode
> + * feature is enabled.
> + */
> +#define EXT4_XATTR_BLOCK_RESERVE(inode)	min(i_blocksize(inode)/8, 1024U)
> +
>  static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
>  				struct ext4_xattr_search *s,
>  				handle_t *handle, struct inode *inode)
> @@ -1487,6 +1493,20 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
>  			ret = -ENOSPC;
>  			goto out;
>  		}
> +
> +		/*
> +		 * If storing the value in an external inode is an option,
> +		 * reserve space for xattr entries/names in the external
> +		 * attribute block so that a long value does not occupy the
> +		 * whole space and prevent futher entries being added.
> +		 */
> +		if (ext4_has_feature_ea_inode(inode->i_sb) && new_size &&
> +		    (s->end - s->base) == i_blocksize(inode) &&
> +		    (min_offs + old_size - new_size) <
> +					EXT4_XATTR_BLOCK_RESERVE(inode)) {
> +			ret = -ENOSPC;
> +			goto out;
> +		}
>  	}
>  
>  	/*
> -- 
> 2.13.1.508.gb3defc5cc-goog
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot