linux-kernel - Re: [PATCH v2 08/10] fsdax: Dedup file range to use a compare function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 03 Mar 2021 00:20:14 -0800
From:   Joe Perches <joe@...ches.com>
To:     Shiyang Ruan <ruansy.fnst@...itsu.com>,
        linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org,
        linux-nvdimm@...ts.01.org, linux-fsdevel@...r.kernel.org
Cc:     darrick.wong@...cle.com, dan.j.williams@...el.com,
        willy@...radead.org, jack@...e.cz, viro@...iv.linux.org.uk,
        linux-btrfs@...r.kernel.org, ocfs2-devel@....oracle.com,
        david@...morbit.com, hch@....de, rgoldwyn@...e.de,
        Goldwyn Rodrigues <rgoldwyn@...e.com>
Subject: Re: [PATCH v2 08/10] fsdax: Dedup file range to use a compare
 function

On Fri, 2021-02-26 at 08:20 +0800, Shiyang Ruan wrote:
> With dax we cannot deal with readpage() etc. So, we create a dax
> comparison funciton which is similar with
> vfs_dedupe_file_range_compare().
> And introduce dax_remap_file_range_prep() for filesystem use.
[]
> diff --git a/fs/dax.c b/fs/dax.c
[]
> @@ -1856,3 +1856,54 @@ vm_fault_t dax_finish_sync_fault(struct vm_fault *vmf,l
>  	return dax_insert_pfn_mkwrite(vmf, pfn, order);
>  }
>  EXPORT_SYMBOL_GPL(dax_finish_sync_fault);
> +
> +static loff_t dax_range_compare_actor(struct inode *ino1, loff_t pos1,
> +		struct inode *ino2, loff_t pos2, loff_t len, void *data,
> +		struct iomap *smap, struct iomap *dmap)
> +{
> +	void *saddr, *daddr;
> +	bool *same = data;
> +	int ret;
> +
> +	while (len) {
> +		if (smap->type == IOMAP_HOLE && dmap->type == IOMAP_HOLE)
> +			goto next;
> +
> +		if (smap->type == IOMAP_HOLE || dmap->type == IOMAP_HOLE) {
> +			*same = false;
> +			break;
> +		}
> +
> +		ret = dax_iomap_direct_access(smap, pos1,
> +			ALIGN(pos1 + len, PAGE_SIZE), &saddr, NULL);
> +		if (ret < 0)
> +			return -EIO;
> +
> +		ret = dax_iomap_direct_access(dmap, pos2,
> +			ALIGN(pos2 + len, PAGE_SIZE), &daddr, NULL);
> +		if (ret < 0)
> +			return -EIO;
> +
> +		*same = !memcmp(saddr, daddr, len);
> +		if (!*same)
> +			break;
> +next:
> +		len -= len;
> +	}
> +
> +	return 0;
> +}

This code looks needlessly complex.

len is never decremented inside the while loop so the while loop
itself looks unnecessary.  Is there some missing decrement of len
or some other reason to use a while loop?

Is dax_iomap_direct_access some ugly macro that modifies a hidden len?

Why not remove the while loop and use straightforward code without
unnecessary indentatation?

{
	void *saddr;
	void *daddr;
	bool *same = data;
	int ret;

	if (!len ||
	    (smap->type == IOMAP_HOLE && dmap->type == IOMAP_HOLE))
		return 0;

	if (smap->type == IOMAP_HOLE || dmap->type == IOMAP_HOLE) {
		*same = false;
		return 0;
	}

	ret = dax_iomap_direct_access(smap, pos1, ALIGN(pos1 + len, PAGE_SIZE),
				      &saddr, NULL);
	if (ret < 0)
		return -EIO;

	ret = dax_iomap_direct_access(dmap, pos2, ALIGN(pos2 + len, PAGE_SIZE),
				      &daddr, NULL);
	if (ret < 0)
		return -EIO;

	*same = !memcmp(saddr, daddr, len);

	return 0;
}	

I didn't look at the rest.