xfs: cap the length of deduplication requests
PATCH 1/1 xfs: cap the length of deduplication requests
From: Darrick J. Wong <[email protected]>
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").
--- fs/xfs/xfs_file.c | 9 +++++++++ 1 file changed, 9 insertions(+)
fs/xfs/xfs_file.c +9 -0
@@ -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)
Quoted 8 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
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 commit1ddae54555b62("common/rc: add missing 'local' keywords").
Looks fine. btrfs limits to 16MB, I guess we don't want to go that low?
Quoted 11 lines · Christoph Hellwig
Quoted 8 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
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 commit1ddae54555b62("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
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/1 xfs: cap the length of deduplication requests
In reply to Darrick J. WongFrom: Darrick J. Wong <[email protected]>
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").
---
v2: halve the amount so that we do MAX_RW_COUNT IO max
--- fs/xfs/xfs_file.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
fs/xfs/xfs_file.c +10 -0
@@ -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)
Quoted 9 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
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.
Quoted 12 lines · Amir Goldstein
Quoted 9 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
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").
Quoted 3 lines
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.
Quoted 2 lines
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
Quoted 6 lines
Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 10 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
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 <[email protected]>
Looks good,
Quoted 33 lines
---
v2: halve the amount so that we doMAX_RW_COUNTIO max--- fs/xfs/xfs_file.c | 10 ++++++++++ 1 file changed, 10 insertions(+)fs/xfs/xfs_file.c +10
-1@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Carlos