Re: [PATCH] btrfs: skip compression property for anything other than files and dirs

[Anand Jain <anand.jain@oracle.com>]

On 21/04/2022 18:01, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> The compression property only has effect on regular files and directories
> (so that it's propagated to files and subdirectories created inside a
> directory). For any other inode type (symlink, fifo, device, socket),
> it's pointless to set the compression property because it does nothing

Hm. symlink propagates the compression xattrs to the target file/dir.

  A symlink to a directory

  $ /btrfs$ ls -la | grep test-029
drwxr-xr-x.  1 root root    0 Apr 12 13:07 test-029
lrwxrwxrwx.  1 root root   10 Apr 21 20:00 test-029-link -> ./test-029


  $ btrfs prop get ./test-029 compression
  $ btrfs prop get ./test-029-link compression

  Set xattr compression to the symlink

  $ btrfs prop set ./test-029-link compression lzo

  The target directory also gets it.

  $ btrfs prop get ./test-029 compression
compression=lzo
  $ btrfs prop get ./test-029 compression
compression=lzo

  This patch affects the change in semantics. No?

Thanks, Anand


> and ends up unnecessarily wasting leaf space due to the pointless xattr
> (75 or 76 bytes, depending on the compression value). Symlinks in
> particular are very common (for example, I have almost 10k symlinks under
> /etc, /usr and /var alone) and therefore it's worth to avoid wasting
> leaf space with the compression xattr.
> 
> For example, the compression property can end up on a symlink or character
> device implicitly, through inheritance from a parent directory
> 
>    $ mkdir /mnt/testdir
>    $ btrfs property set /mnt/testdir compression lzo
> 
>    $ ln -s yadayada /mnt/testdir/lnk
>    $ mknod /mnt/testdir/dev c 0 0
> 
> Or explicitly like this:
> 
>    $ ln -s yadayda /mnt/lnk
>    $ setfattr -h -n btrfs.compression -v lzo /mnt/lnk
> 
> So skip the compression property on inodes that are neither a regular
> file nor a directory.




> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>   fs/btrfs/props.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>   fs/btrfs/props.h |  1 +
>   fs/btrfs/xattr.c |  3 +++
>   3 files changed, 47 insertions(+)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index f5565c296898..7a0038797015 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -20,6 +20,7 @@ struct prop_handler {
>   	int (*validate)(const char *value, size_t len);
>   	int (*apply)(struct inode *inode, const char *value, size_t len);
>   	const char *(*extract)(struct inode *inode);
> +	bool (*ignore)(const struct btrfs_inode *inode);
>   	int inheritable;
>   };
>   
> @@ -72,6 +73,28 @@ int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
>   	return handler->validate(value, value_len);
>   }
>   
> +/*
> + * Check if a property should be ignored (not set) for an inode.
> + *
> + * @inode:     The target inode.
> + * @name:      The property's name.
> + *
> + * The caller must be sure the given property name is valid, for example by
> + * having previously called btrfs_validate_prop().
> + *
> + * Returns:    true if the property should be ignored for the given inode
> + *             false if the property must not be ignored for the given inode
> + */
> +bool btrfs_ignore_prop(const struct btrfs_inode *inode, const char *name)
> +{
> +	const struct prop_handler *handler;
> +
> +	handler = find_prop_handler(name, NULL);
> +	ASSERT(handler != NULL);
> +
> +	return handler->ignore(inode);
> +}
> +
>   int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>   		   const char *name, const char *value, size_t value_len,
>   		   int flags)
> @@ -310,6 +333,22 @@ static int prop_compression_apply(struct inode *inode, const char *value,
>   	return 0;
>   }
>   
> +static bool prop_compression_ignore(const struct btrfs_inode *inode)
> +{
> +	/*
> +	 * Compression only has effect for regular files, and for directories
> +	 * we set it just to propagate it to new files created inside them.
> +	 * Everything else (symlinks, devices, sockets, fifos) is pointless as
> +	 * it will do nothing, so don't waste metadata space on a compression
> +	 * xattr for anything that is neither a file nor a directory.
> +	 */
> +	if (!S_ISREG(inode->vfs_inode.i_mode) &&
> +	    !S_ISDIR(inode->vfs_inode.i_mode))
> +		return true;
> +
> +	return false;
> +}
> +
>   static const char *prop_compression_extract(struct inode *inode)
>   {
>   	switch (BTRFS_I(inode)->prop_compress) {
> @@ -330,6 +369,7 @@ static struct prop_handler prop_handlers[] = {
>   		.validate = prop_compression_validate,
>   		.apply = prop_compression_apply,
>   		.extract = prop_compression_extract,
> +		.ignore = prop_compression_ignore,
>   		.inheritable = 1
>   	},
>   };
> @@ -355,6 +395,9 @@ int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,
>   		if (!h->inheritable)
>   			continue;
>   
> +		if (h->ignore(BTRFS_I(inode)))
> +			continue;
> +
>   		value = h->extract(parent);
>   		if (!value)
>   			continue;
> diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
> index 1dcd5daa3b22..09bf1702bb34 100644
> --- a/fs/btrfs/props.h
> +++ b/fs/btrfs/props.h
> @@ -14,6 +14,7 @@ int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>   		   const char *name, const char *value, size_t value_len,
>   		   int flags);
>   int btrfs_validate_prop(const char *name, const char *value, size_t value_len);
> +bool btrfs_ignore_prop(const struct btrfs_inode *inode, const char *name);
>   
>   int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path);
>   
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index b96ffd775b41..f9d22ff3567f 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -389,6 +389,9 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
>   	if (ret)
>   		return ret;
>   
> +	if (btrfs_ignore_prop(BTRFS_I(inode), name))
> +		return 0;
> +
>   	trans = btrfs_start_transaction(root, 2);
>   	if (IS_ERR(trans))
>   		return PTR_ERR(trans);