[PATCH] xfs: cap the length of deduplication requests

[PATCH] xfs: cap the length of deduplication requests

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Since deduplication potentially has to read in all the pages in both
files in order to compare the contents, cap the deduplication request
length at MAX_RW_COUNT (roughly 2GB) so that we have /some/ upper bound
on the request length and can't just lock up the kernel forever.  Found
by running generic/304 after commit 1ddae54555b62 ("common/rc: add
missing 'local' keywords").

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_file.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 299aee4..9fd9dd7 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -876,8 +876,17 @@ xfs_file_dedupe_range(
 	struct file	*dst_file,
 	u64		dst_loff)
 {
+	struct inode	*srci = file_inode(src_file);
+	u64		max_dedupe;
 	int		error;
 
+	/*
+	 * Since we have to read all these pages in to compare them, cut
+	 * it off at MAX_RW_COUNT rounded down to the nearest block.
+	 */
+	max_dedupe = MAX_RW_COUNT & ~(i_blocksize(srci) - 1);
+	if (len > max_dedupe)
+		len = max_dedupe;
 	error = xfs_reflink_remap_range(src_file, loff, dst_file, dst_loff,
 				     len, true);
 	if (error)

Re: [PATCH] xfs: cap the length of deduplication requests

[Christoph Hellwig]

On Mon, Apr 16, 2018 at 10:19:18PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Since deduplication potentially has to read in all the pages in both
> files in order to compare the contents, cap the deduplication request
> length at MAX_RW_COUNT (roughly 2GB) so that we have /some/ upper bound
> on the request length and can't just lock up the kernel forever.  Found
> by running generic/304 after commit 1ddae54555b62 ("common/rc: add
> missing 'local' keywords").

Looks fine.  btrfs limits to 16MB, I guess we don't want to go that low?

Re: [PATCH] xfs: cap the length of deduplication requests

[Darrick J. Wong]

On Mon, Apr 16, 2018 at 11:58:00PM -0700, Christoph Hellwig wrote:
> On Mon, Apr 16, 2018 at 10:19:18PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Since deduplication potentially has to read in all the pages in both
> > files in order to compare the contents, cap the deduplication request
> > length at MAX_RW_COUNT (roughly 2GB) so that we have /some/ upper bound
> > on the request length and can't just lock up the kernel forever.  Found
> > by running generic/304 after commit 1ddae54555b62 ("common/rc: add
> > missing 'local' keywords").
> 
> Looks fine.  btrfs limits to 16MB, I guess we don't want to go that low?

I've wondered about that -- the btrfs developers opine that you're
unlikely to hit a 16+ MB extent that can be deduplicated, but seeing as
the usage model seems to be "copy files onto fs, let dedupe program
target files in the background", I have my doubts.

Specifically, most of the usage models I envision are backup programs
wanting to dedupe files that are almost but not quite the same; and VM
farms.  The cross-host VM migration tools I've seen tend to copy the
image as fast as possible to minimize transition time and worry about
whether or not there are shared blocks later.

OTOH I do worry a little about the prospect of doing 4GB of IO per
kernel call, so maybe this should be (MAX_RW_COUNT/2)?.

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] xfs: cap the length of deduplication requests

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Since deduplication potentially has to read in all the pages in both
files in order to compare the contents, cap the deduplication request
length at MAX_RW_COUNT/2 (roughly 1GB) so that we have /some/ upper bound
on the request length and can't just lock up the kernel forever.  Found
by running generic/304 after commit 1ddae54555b62a ("common/rc: add
missing 'local' keywords").

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: halve the amount so that we do MAX_RW_COUNT IO max
---
 fs/xfs/xfs_file.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 299aee4..9cafad4 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -876,8 +876,18 @@ xfs_file_dedupe_range(
 	struct file	*dst_file,
 	u64		dst_loff)
 {
+	struct inode	*srci = file_inode(src_file);
+	u64		max_dedupe;
 	int		error;
 
+	/*
+	 * Since we have to read all these pages in to compare them, cut
+	 * it off at MAX_RW_COUNT/2 rounded down to the nearest block.
+	 * That means we won't do more than MAX_RW_COUNT IO per request.
+	 */
+	max_dedupe = (MAX_RW_COUNT >> 1) & ~(i_blocksize(srci) - 1);
+	if (len > max_dedupe)
+		len = max_dedupe;
 	error = xfs_reflink_remap_range(src_file, loff, dst_file, dst_loff,
 				     len, true);
 	if (error)

Re: [PATCH v2] xfs: cap the length of deduplication requests

[Amir Goldstein]

On Wed, Apr 18, 2018 at 5:57 AM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Since deduplication potentially has to read in all the pages in both
> files in order to compare the contents, cap the deduplication request
> length at MAX_RW_COUNT/2 (roughly 1GB) so that we have /some/ upper bound
> on the request length and can't just lock up the kernel forever.  Found
> by running generic/304 after commit 1ddae54555b62a ("common/rc: add
> missing 'local' keywords").
>

With this patch generic/304 doesn't soft lock the kernel, but it doesn't
seem 'quick' at all. I stopped waiting after 5 minutes. Is that expected?
If so, best get the test out of the quick group.

Can you shed some light on how the "missing 'local' keywords" change
exposed this behavior?

Thanks,
Amir.

Re: [PATCH v2] xfs: cap the length of deduplication requests

[Darrick J. Wong]

On Sun, Apr 22, 2018 at 05:57:32AM +0300, Amir Goldstein wrote:
> On Wed, Apr 18, 2018 at 5:57 AM, Darrick J. Wong
> <darrick.wong@oracle.com> wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Since deduplication potentially has to read in all the pages in both
> > files in order to compare the contents, cap the deduplication request
> > length at MAX_RW_COUNT/2 (roughly 1GB) so that we have /some/ upper bound
> > on the request length and can't just lock up the kernel forever.  Found
> > by running generic/304 after commit 1ddae54555b62a ("common/rc: add
> > missing 'local' keywords").
> >
> 
> With this patch generic/304 doesn't soft lock the kernel, but it doesn't
> seem 'quick' at all. I stopped waiting after 5 minutes. Is that expected?
> If so, best get the test out of the quick group.

No, that is incorrect behavior; the test is buggy.

> Can you shed some light on how the "missing 'local' keywords" change
> exposed this behavior?

See the patch I sent to the fstests lists to fix generic/304.

--D

> Thanks,
> Amir.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: cap the length of deduplication requests

[Carlos Maiolino]

On Tue, Apr 17, 2018 at 07:57:55PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Since deduplication potentially has to read in all the pages in both
> files in order to compare the contents, cap the deduplication request
> length at MAX_RW_COUNT/2 (roughly 1GB) so that we have /some/ upper bound
> on the request length and can't just lock up the kernel forever.  Found
> by running generic/304 after commit 1ddae54555b62a ("common/rc: add
> missing 'local' keywords").
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> ---
> v2: halve the amount so that we do MAX_RW_COUNT IO max
> ---
>  fs/xfs/xfs_file.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 299aee4..9cafad4 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -876,8 +876,18 @@ xfs_file_dedupe_range(
>  	struct file	*dst_file,
>  	u64		dst_loff)
>  {
> +	struct inode	*srci = file_inode(src_file);
> +	u64		max_dedupe;
>  	int		error;
>  
> +	/*
> +	 * Since we have to read all these pages in to compare them, cut
> +	 * it off at MAX_RW_COUNT/2 rounded down to the nearest block.
> +	 * That means we won't do more than MAX_RW_COUNT IO per request.
> +	 */
> +	max_dedupe = (MAX_RW_COUNT >> 1) & ~(i_blocksize(srci) - 1);
> +	if (len > max_dedupe)
> +		len = max_dedupe;
>  	error = xfs_reflink_remap_range(src_file, loff, dst_file, dst_loff,
>  				     len, true);
>  	if (error)
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos