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

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

On 4/21/22 21:51, Filipe Manana wrote:
> On Thu, Apr 21, 2022 at 2:42 PM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> 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?
> 
> In your examples you are setting/getting the property not to/from the
> symlink inode itself but to/from the inode it points at.
> 
> "btrfs property set/get" follows symlinks.
> That's why in my example I used setfattr with -h (don't follow symlinks).

(my vacation in between; sorry for the delay).

Yep. I got it.

Reviewed-by: Anand Jain <anand.jain@oracle.com>


> 
>>
>> 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);
>>