Re: [RFC PATCH v6 5/5] Btrfs: online data deduplication

[David Sterba <dsterba@suse.cz>]

I wanted to only comment on the ioctl and interface to userspace bits,
but found more things to comment in the kernel code.

On Thu, Aug 08, 2013 at 04:35:45PM +0800, Liu Bo wrote:
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -94,6 +95,9 @@ struct btrfs_ordered_sum;
>  /* for storing balance parameters in the root tree */
>  #define BTRFS_BALANCE_OBJECTID -4ULL
>  
> +/* dedup tree(experimental) */
> +#define BTRFS_DEDUP_TREE_OBJECTID 9ULL

9 now conflicts with the uuid tree, when you update the patch, please
put the dedup tree definition right next after the uuid tree.

I've noticed that the lockdep annotation is also missing from disk-io.c)
and the symbolic tree name in include/trace/events/btrfs.h as well.

> +enum btrfs_dedup_type {
> +	BTRFS_DEDUP_SHA256 = 0,
> +	BTRFS_DEDUP_LAST = 1,
> +};
> +
> +static int btrfs_dedup_lens[] = { 4, 0 };
> +static int btrfs_dedup_sizes[] = { 32, 0 };	/* 256bit, 32bytes */

This is a bit confusing, what's the difference between 'len' and 'size'
here.

> +struct btrfs_dedup_item {
> +	__le64 len;	/* disk length of dedup range */
> +	u8 type;
> +	u8 compression;
> +	u8 encryption;
> +	__le16 other_encoding; /* spare for later use */
> +
> +	/* hash/fingerprints go here */
> +} __attribute__ ((__packed__));
> +
> +struct btrfs_dedup_hash {
> +	/* hash algorithm */
> +	int type;
> +	u64 bytenr;
> +	u64 num_bytes;
> +	int compression;

Layout of this structure is a bit suboptimal, there is a 4 byte hole
after type and compression, and you can use u8 types for both, but as
this would leave another 6b hole after, using ints is ok, so the final
layout should preferrably look like

{
	u64 bytenr;
	u64 num_bytes;
	int type;
	int compression;

	u64 hash[];
};

> +
> +	/* last field is a variable length array of dedup hash */
> +	u64 hash[];
> +};
> +
> @@ -1967,6 +2027,7 @@ struct btrfs_ioctl_defrag_range_args {
>  #define BTRFS_MOUNT_CHECK_INTEGRITY	(1 << 20)
>  #define BTRFS_MOUNT_CHECK_INTEGRITY_INCLUDING_EXTENT_DATA (1 << 21)
>  #define BTRFS_MOUNT_PANIC_ON_FATAL_ERROR	(1 << 22)
> +#define BTRFS_MOUNT_DEDUP		(1 << 24)
>  
>  #define btrfs_clear_opt(o, opt)		((o) &= ~BTRFS_MOUNT_##opt)
>  #define btrfs_set_opt(o, opt)		((o) |= BTRFS_MOUNT_##opt)
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2184,6 +2194,8 @@ int open_ctree(struct super_block *sb,
>  	atomic64_set(&fs_info->tree_mod_seq, 0);
>  	fs_info->sb = sb;
>  	fs_info->max_inline = 8192 * 1024;
> +	fs_info->dedup_bs = 8192;
> +	fs_info->dedup_type = BTRFS_DEDUP_SHA256;
>  	fs_info->metadata_ratio = 0;
>  	fs_info->defrag_inodes = RB_ROOT;
>  	fs_info->free_chunk_space = 0;
> @@ -2282,6 +2294,7 @@ int open_ctree(struct super_block *sb,
>  	mutex_init(&fs_info->dev_replace.lock_finishing_cancel_unmount);
>  	mutex_init(&fs_info->dev_replace.lock_management_lock);
>  	mutex_init(&fs_info->dev_replace.lock);
> +	mutex_init(&fs_info->dedup_ioctl_mutex);
>  
>  	spin_lock_init(&fs_info->qgroup_lock);
>  	mutex_init(&fs_info->qgroup_ioctl_lock);
> @@ -2457,6 +2470,14 @@ int open_ctree(struct super_block *sb,
>  		goto fail_alloc;
>  	}
>  
> +	fs_info->dedup_driver = crypto_alloc_shash("sha256", 0, 0);

Isn't this supposed to use fs_info->dedup_type somehow? This is harcoded
to sha256 and it's the only one available now, but when the defines and
variables are there, please do use them

> +	if (IS_ERR(fs_info->dedup_driver)) {
> +		pr_info("Cannot load sha256 driver\n");
> +		err = PTR_ERR(fs_info->dedup_driver);
> +		fs_info->dedup_driver = NULL;
> +		goto fail_alloc;
> +	}
> +
>  	btrfs_init_workers(&fs_info->generic_worker,
>  			   "genwork", 1, NULL);
>  
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -1102,8 +1102,16 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
>  		key.offset = parent;
>  	} else {
>  		key.type = BTRFS_EXTENT_DATA_REF_KEY;
> -		key.offset = hash_extent_data_ref(root_objectid,
> -						  owner, offset);
> +
> +		/*
> +		 * we've not got the right offset and owner, so search by -1
> +		 * here.
> +		 */
> +		if (root_objectid == BTRFS_DEDUP_TREE_OBJECTID)
> +			key.offset = -1;

offset is u64

> +		else
> +			key.offset = hash_extent_data_ref(root_objectid,
> +							  owner, offset);
>  	}
>  again:
>  	recow = 0;
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> +static int run_locked_range(struct extent_io_tree *tree, struct inode *inode,
> +			    struct page *locked_page, u64 start, u64 end,
> +			    get_extent_t *get_extent, int mode,
> +			    struct btrfs_dedup_hash *hash)
> +{
> +	int page_started = 0;
> +	unsigned long nr_written = 0;
> +	int ret;
> +
> +	lock_extent(tree, start, end);
> +
> +	/* allocate blocks */
> +	ret = cow_file_range_dedup(inode, locked_page, start, end,
> +				   &page_started, &nr_written, 0, hash);
> +
> +	/* JDM XXX */

What? :)

> +
> +	/*
> +	 * if page_started, cow_file_range inserted an
> +	 * inline extent and took care of all the unlocking
> +	 * and IO for us.  Otherwise, we need to submit
> +	 * all those pages down to the drive.
> +	 */
> +	if (!page_started && !ret)
> +		extent_write_locked_range(tree, inode, start, end, get_extent,
> +					  mode);
> +	else if (ret)
> +		unlock_page(locked_page);
> +
> +	return ret;
> +}
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -361,6 +362,8 @@ static match_table_t tokens = {
>  	{Opt_check_integrity_including_extent_data, "check_int_data"},
>  	{Opt_check_integrity_print_mask, "check_int_print_mask=%d"},
>  	{Opt_fatal_errors, "fatal_errors=%s"},
> +	{Opt_dedup_bs, "dedup_bs=%s"},
> +	{Opt_dedup, "dedup"},

What's the expected usage? You have the ioctl to enable/disable dedup
for that, so if it's enabled via a mount option it is equivalent to
calling the 'dedup enable' after mount, ok?

I think dedup is kind of similar to quotas, you enable them and want
them to be persistent once enabled, or disabled upon an explicit
request.

Setting the blocksize via mount option looks more like a temporary
solution before we have persistent per-object properties, here it would
be eg. "filesystem.dedup_block_size".

But once the mount option is there, it's much harder to remove it so we
should not introduce it at all. For now I suggest to add another mode
for the dedup ioctl to set the block size.

Another question is whether to enable different block sizes for
different subvolumes, then this should be reflected in the ioctl
interface.


>  	{Opt_err, NULL},
>  };
>  
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -374,6 +374,10 @@ struct btrfs_ioctl_get_dev_stats {
>  	__u64 unused[128 - 2 - BTRFS_DEV_STAT_VALUES_MAX]; /* pad to 1k */
>  };
>  
> +/* deduplication control ioctl modes */
> +#define BTRFS_DEDUP_CTL_REG 1
> +#define BTRFS_DEDUP_CTL_UNREG 2

And the whole point of my mail was to tell you that for consistency with
the userspace tools I suggest to rename the ioctl defines to

BTRFS_DEDUP_CTL_REG -> BTRFS_DEDUP_CTL_ENABLE
BTRFS_DEDUP_CTL_UNREG -> BTRFS_DEDUP_CTL_DISABLE

but it turned out to be a bit longer :)

> +
>  #define BTRFS_QUOTA_CTL_ENABLE	1
>  #define BTRFS_QUOTA_CTL_DISABLE	2
>  #define BTRFS_QUOTA_CTL_RESCAN__NOTUSED	3