Re: [RFC][PATCH] btrfs: add an autodefrag property

[Nikolay Borisov <nborisov@suse.com>]

On 17.07.20 г. 23:42 ч., Josef Bacik wrote:
> Autodefrag is very useful for somethings, like the 9000 sqllite files
> that Firefox uses, but is way less useful for virt images.
> Unfortunately this is only available currently as a whole mount option.
> Fix this by adding an "autodefrag" property, that users can set on a per
> file or per directory basis.  Thus allowing them to control where
> exactly the extra write activity is going to occur.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> - This is an RFC because I want to make sure we're ok with this before I go and
>   add btrfs-progs support for this.  I'm not married to the name or the value,
>   but I think the core goal is valuable.
> 
>  fs/btrfs/btrfs_inode.h |  1 +
>  fs/btrfs/file.c        | 16 ++++++++++------
>  fs/btrfs/props.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index e7d709505cb1..4f04f0535f90 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -31,6 +31,7 @@ enum {
>  	BTRFS_INODE_READDIO_NEED_LOCK,
>  	BTRFS_INODE_HAS_PROPS,
>  	BTRFS_INODE_SNAPSHOT_FLUSH,
> +	BTRFS_INODE_AUTODEFRAG,
>  };
>  
>  /* in memory btrfs inode */
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 841c516079a9..cac2092bdcdf 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -116,12 +116,16 @@ static int __btrfs_add_inode_defrag(struct btrfs_inode *inode,
>  	return 0;
>  }
>  
> -static inline int __need_auto_defrag(struct btrfs_fs_info *fs_info)
> +static inline int __need_auto_defrag(struct btrfs_fs_info *fs_info,
> +				     struct btrfs_inode *inode)
>  {
> -	if (!btrfs_test_opt(fs_info, AUTO_DEFRAG))
> +	if (btrfs_fs_closing(fs_info))
>  		return 0;
>  
> -	if (btrfs_fs_closing(fs_info))
> +	if (inode && test_bit(BTRFS_INODE_AUTODEFRAG, &inode->runtime_flags))
> +		return 1;

Qu already made a comment about accessing fs_info from inode, but since
you check for the presence of inode here, when do you expect for it to
be NULL ? If not, then that check is redundant.
> +
> +	if (!btrfs_test_opt(fs_info, AUTO_DEFRAG))
>  		return 0;
>  
>  	return 1;

nit: while at it use the occasion to convert the function to proper bool

<snip>