btrfs: use kvzalloc for EXTENT_SAME temporary data

PATCH Superseded David Sterba 5 participants 7 messages May 11, 2018 Btrfs file system

Share & tools

David Sterba

author maintainer

2018-05-11 16:00 UTC #0 lore raw

PATCH 1/1 btrfs: use kvzalloc for EXTENT_SAME temporary data

The dedupe range is 16 MiB, with 4KiB pages and 8 byte pointers, the arrays can be 32KiB large. To avoid allocation failures due to fragmented memory, use the allocation with fallback to vmalloc.

Signed-off-by David Sterba <[email protected]>

---

This depends on the patches that remove the 16MiB restriction in the dedupe ioctl, but contextually can be applied to the current code too.

https://patchwork.kernel.org/patch/10374941/

 fs/btrfs/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/btrfs/ioctl.c +2 -2

index b572e38b4b64..a7f517009cd7 100644
@@ -3178,8 +3178,8 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
 	 * locking. We use an array for the page pointers. Size of the array is
 	 * bounded by len, which is in turn bounded by BTRFS_MAX_DEDUPE_LEN.
 	 */
-	cmp.src_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
-	cmp.dst_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+	cmp.src_pages = kvzalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+	cmp.dst_pages = kvzalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
 	if (!cmp.src_pages || !cmp.dst_pages) {
 		kfree(cmp.src_pages);
 		kfree(cmp.dst_pages);

Signature · 1 line

2.16.2

Filipe Manana

2018-05-11 16:25 UTC #1 lore raw

Quoted 20 lines · David Sterba

The dedupe range is 16 MiB, with 4KiB pages and 8 byte pointers, the arrays can be 32KiB large. To avoid allocation failures due to fragmented memory, use the allocation with fallback to vmalloc.
Signed-off-by David Sterba <[email protected]>
---
This depends on the patches that remove the 16MiB restriction in the dedupe ioctl, but contextually can be applied to the current code too.
https://patchwork.kernel.org/patch/10374941/
 fs/btrfs/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/btrfs/ioctl.c +0 -0
index b572e38b4b64..a7f517009cd7 100644

fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, · line 3182

Quoted 7 lines

fs/btrfs/ioctl.c +2 -2

@@ -3178,8 +3178,8 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
         * locking. We use an array for the page pointers. Size of the array is
         * bounded by len, which is in turn bounded by BTRFS_MAX_DEDUPE_LEN.
         */
-       cmp.src_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
-       cmp.dst_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+       cmp.src_pages = kvzalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+       cmp.dst_pages = kvzalloc(num_pages, sizeof(struct page *), GFP_KERNEL);

Kvzalloc should take 2 parameters and not 3. Also, aren't the corresponding kvfree() calls missing?

Quoted 10 lines

fs/btrfs/ioctl.c +0 -1
        if (!cmp.src_pages || !cmp.dst_pages) {
                kfree(cmp.src_pages);
                kfree(cmp.dst_pages);
--
2.16.2
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

David Sterba

2018-05-11 16:51 UTC #2 lore raw

Quoted 16 lines · Filipe Manana

Quoted 15 lines · David Sterba
The dedupe range is 16 MiB, with 4KiB pages and 8 byte pointers, the arrays can be 32KiB large. To avoid allocation failures due to fragmented memory, use the allocation with fallback to vmalloc.
Signed-off-by David Sterba <[email protected]>
---
This depends on the patches that remove the 16MiB restriction in the dedupe ioctl, but contextually can be applied to the current code too.
https://patchwork.kernel.org/patch/10374941/
 fs/btrfs/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Quoted 14 lines

fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, · line 3182

Quoted 12 lines

fs/btrfs/ioctl.c +2 -2

index b572e38b4b64..a7f517009cd7 100644
@@ -3178,8 +3178,8 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
         * locking. We use an array for the page pointers. Size of the array is
         * bounded by len, which is in turn bounded by BTRFS_MAX_DEDUPE_LEN.
         */
-       cmp.src_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
-       cmp.dst_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+       cmp.src_pages = kvzalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+       cmp.dst_pages = kvzalloc(num_pages, sizeof(struct page *), GFP_KERNEL);

Kvzalloc should take 2 parameters and not 3.

And the right function is kvmalloc_array.

Quoted 1 line

Also, aren't the corresponding kvfree() calls missing?

Yes, thanks for catching it. The updated version:

From: David Sterba <[email protected]>
Subject: [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data

The dedupe range is 16 MiB, with 4KiB pages and 8 byte pointers, the arrays can be 32KiB large. To avoid allocation failures due to fragmented memory, use the allocation with fallback to vmalloc.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ioctl.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

fs/btrfs/ioctl.c +9 -7

index b572e38b4b64..4fcfa05ed960 100644
@@ -3178,12 +3178,13 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
 	 * locking. We use an array for the page pointers. Size of the array is
 	 * bounded by len, which is in turn bounded by BTRFS_MAX_DEDUPE_LEN.
 	 */
-	cmp.src_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
-	cmp.dst_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+	cmp.src_pages = kvmalloc_array(num_pages, sizeof(struct page *),
+				       GFP_KERNEL);
+	cmp.dst_pages = kvmalloc_array(num_pages, sizeof(struct page *),
+				       GFP_KERNEL);
 	if (!cmp.src_pages || !cmp.dst_pages) {
-		kfree(cmp.src_pages);
-		kfree(cmp.dst_pages);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out_free;
 	}
 
 	if (same_inode)

@@ -3211,8 +3212,9 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
 	else
 		btrfs_double_inode_unlock(src, dst);
 
-	kfree(cmp.src_pages);
-	kfree(cmp.dst_pages);
+out_free:
+	kvfree(cmp.src_pages);
+	kvfree(cmp.dst_pages);
 
 	return ret;
 }

Signature · 1 line

2.16.2

Filipe Manana

2018-05-11 17:16 UTC #3 lore raw

Quoted 74 lines · David Sterba

Quoted 16 lines · Filipe Manana
Quoted 15 lines · David Sterba
The dedupe range is 16 MiB, with 4KiB pages and 8 byte pointers, the arrays can be 32KiB large. To avoid allocation failures due to fragmented memory, use the allocation with fallback to vmalloc.
Signed-off-by David Sterba <[email protected]>
---
This depends on the patches that remove the 16MiB restriction in the dedupe ioctl, but contextually can be applied to the current code too.
https://patchwork.kernel.org/patch/10374941/
 fs/btrfs/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 14 lines
fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, · line 3182
Quoted 12 lines
fs/btrfs/ioctl.c +2 -2
index b572e38b4b64..a7f517009cd7 100644
@@ -3178,8 +3178,8 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
         * locking. We use an array for the page pointers. Size of the array is
         * bounded by len, which is in turn bounded by BTRFS_MAX_DEDUPE_LEN.
         */
-       cmp.src_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
-       cmp.dst_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+       cmp.src_pages = kvzalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+       cmp.dst_pages = kvzalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
Kvzalloc should take 2 parameters and not 3.
And the right function is kvmalloc_array.
Quoted 1 line
Also, aren't the corresponding kvfree() calls missing?
Yes, thanks for catching it. The updated version:
From: David Sterba <[email protected]>
Subject: [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data
The dedupe range is 16 MiB, with 4KiB pages and 8 byte pointers, the arrays can be 32KiB large. To avoid allocation failures due to fragmented memory, use the allocation with fallback to vmalloc.
Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ioctl.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
fs/btrfs/ioctl.c +6 -5
index b572e38b4b64..4fcfa05ed960 100644
@@ -3178,12 +3178,13 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
         * locking. We use an array for the page pointers. Size of the array is
         * bounded by len, which is in turn bounded by BTRFS_MAX_DEDUPE_LEN.
         */
-       cmp.src_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
-       cmp.dst_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+       cmp.src_pages = kvmalloc_array(num_pages, sizeof(struct page *),
+                                      GFP_KERNEL);
+       cmp.dst_pages = kvmalloc_array(num_pages, sizeof(struct page *),
+                                      GFP_KERNEL);
        if (!cmp.src_pages || !cmp.dst_pages) {
-               kfree(cmp.src_pages);
-               kfree(cmp.dst_pages);
-               return -ENOMEM;
+               ret = -ENOMEM;
+               goto out_free;
        }
 
        if (same_inode)

fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, · line 3217

Quoted 8 lines

fs/btrfs/ioctl.c +3 -2

@@ -3211,8 +3212,9 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
        else
                btrfs_double_inode_unlock(src, dst);
 
-       kfree(cmp.src_pages);
-       kfree(cmp.dst_pages);
+out_free:
+       kvfree(cmp.src_pages);
+       kvfree(cmp.dst_pages);

kvfree() missing at btrfs_cmp_data_free() too.

Quoted 6 lines

fs/btrfs/ioctl.c +0 -1
        return ret;
 }
--
2.16.2

Signature · 3 lines

Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

David Sterba

2018-05-11 21:03 UTC #4 lore raw

Quoted 78 lines · Filipe Manana

Quoted 77 lines · David Sterba
Quoted 16 lines · Filipe Manana
Quoted 15 lines · David Sterba
The dedupe range is 16 MiB, with 4KiB pages and 8 byte pointers, the arrays can be 32KiB large. To avoid allocation failures due to fragmented memory, use the allocation with fallback to vmalloc.
Signed-off-by David Sterba <[email protected]>
---
This depends on the patches that remove the 16MiB restriction in the dedupe ioctl, but contextually can be applied to the current code too.
https://patchwork.kernel.org/patch/10374941/
 fs/btrfs/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 14 lines
fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, · line 3182
Quoted 12 lines
fs/btrfs/ioctl.c +2 -2
index b572e38b4b64..a7f517009cd7 100644
@@ -3178,8 +3178,8 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
         * locking. We use an array for the page pointers. Size of the array is
         * bounded by len, which is in turn bounded by BTRFS_MAX_DEDUPE_LEN.
         */
-       cmp.src_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
-       cmp.dst_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+       cmp.src_pages = kvzalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+       cmp.dst_pages = kvzalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
Kvzalloc should take 2 parameters and not 3.
And the right function is kvmalloc_array.
Quoted 1 line
Also, aren't the corresponding kvfree() calls missing?
Yes, thanks for catching it. The updated version:
From: David Sterba <[email protected]>
Subject: [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data
The dedupe range is 16 MiB, with 4KiB pages and 8 byte pointers, the arrays can be 32KiB large. To avoid allocation failures due to fragmented memory, use the allocation with fallback to vmalloc.
Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ioctl.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
fs/btrfs/ioctl.c +6 -5
index b572e38b4b64..4fcfa05ed960 100644
@@ -3178,12 +3178,13 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
         * locking. We use an array for the page pointers. Size of the array is
         * bounded by len, which is in turn bounded by BTRFS_MAX_DEDUPE_LEN.
         */
-       cmp.src_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
-       cmp.dst_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+       cmp.src_pages = kvmalloc_array(num_pages, sizeof(struct page *),
+                                      GFP_KERNEL);
+       cmp.dst_pages = kvmalloc_array(num_pages, sizeof(struct page *),
+                                      GFP_KERNEL);
        if (!cmp.src_pages || !cmp.dst_pages) {
-               kfree(cmp.src_pages);
-               kfree(cmp.dst_pages);
-               return -ENOMEM;
+               ret = -ENOMEM;
+               goto out_free;
        }
 
        if (same_inode)
@@ -3211,8 +3212,9 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
        else
                btrfs_double_inode_unlock(src, dst);
 

Quoted 7 lines

fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, · line 3217
Quoted 5 lines
fs/btrfs/ioctl.c +3 -2
-       kfree(cmp.src_pages);
-       kfree(cmp.dst_pages);
+out_free:
+       kvfree(cmp.src_pages);
+       kvfree(cmp.dst_pages);
kvfree() missing at btrfs_cmp_data_free() too.

Bah, no more quick fixes from me today. I'll drop the patch from the branch and will have a fresh look another day.

Omar Sandoval

2018-05-11 17:31 UTC #5 lore raw

In reply to David Sterba

Quoted 55 lines · David Sterba

Quoted 16 lines · Filipe Manana
Quoted 15 lines · David Sterba
The dedupe range is 16 MiB, with 4KiB pages and 8 byte pointers, the arrays can be 32KiB large. To avoid allocation failures due to fragmented memory, use the allocation with fallback to vmalloc.
Signed-off-by David Sterba <[email protected]>
---
This depends on the patches that remove the 16MiB restriction in the dedupe ioctl, but contextually can be applied to the current code too.
https://patchwork.kernel.org/patch/10374941/
 fs/btrfs/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 14 lines
fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, · line 3182
Quoted 12 lines
fs/btrfs/ioctl.c +2 -2
index b572e38b4b64..a7f517009cd7 100644
@@ -3178,8 +3178,8 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
         * locking. We use an array for the page pointers. Size of the array is
         * bounded by len, which is in turn bounded by BTRFS_MAX_DEDUPE_LEN.
         */
-       cmp.src_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
-       cmp.dst_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+       cmp.src_pages = kvzalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+       cmp.dst_pages = kvzalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
Kvzalloc should take 2 parameters and not 3.
And the right function is kvmalloc_array.
Quoted 1 line
Also, aren't the corresponding kvfree() calls missing?
Yes, thanks for catching it. The updated version:
From: David Sterba <[email protected]>
Subject: [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data
The dedupe range is 16 MiB, with 4KiB pages and 8 byte pointers, the arrays can be 32KiB large. To avoid allocation failures due to fragmented memory, use the allocation with fallback to vmalloc.
Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ioctl.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
fs/btrfs/ioctl.c +0 -0
index b572e38b4b64..4fcfa05ed960 100644

fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, · line 3184

Quoted 9 lines

fs/btrfs/ioctl.c +4 -2

@@ -3178,12 +3178,13 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
 	 * locking. We use an array for the page pointers. Size of the array is
 	 * bounded by len, which is in turn bounded by BTRFS_MAX_DEDUPE_LEN.
 	 */
-	cmp.src_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
-	cmp.dst_pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+	cmp.src_pages = kvmalloc_array(num_pages, sizeof(struct page *),
+				       GFP_KERNEL);
+	cmp.dst_pages = kvmalloc_array(num_pages, sizeof(struct page *),
+				       GFP_KERNEL);

kcalloc() implies __GFP_ZERO, do we need that here?

Quoted 28 lines

fs/btrfs/ioctl.c +5 -5

 	if (!cmp.src_pages || !cmp.dst_pages) {
-		kfree(cmp.src_pages);
-		kfree(cmp.dst_pages);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out_free;
 	}
 
 	if (same_inode)

@@ -3211,8 +3212,9 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
 	else
 		btrfs_double_inode_unlock(src, dst);
 
-	kfree(cmp.src_pages);
-	kfree(cmp.dst_pages);
+out_free:
+	kvfree(cmp.src_pages);
+	kvfree(cmp.dst_pages);
 
 	return ret;
 }

Signature · 6 lines

2.16.2

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Timofey Titovets

2018-05-11 17:48 UTC #6 lore raw

пт, 11 мая 2018 г. в 20:32, Omar Sandoval <[email protected]>:

Quoted 3 lines · David Sterba

Quoted 2 lines · Filipe Manana
Quoted 1 line
On Fri, May 11, 2018 at 4:57 PM, David Sterba <[email protected]>

Quoted 9 lines · wrote:

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
The dedupe range is 16 MiB, with 4KiB pages and 8 byte pointers, the arrays can be 32KiB large. To avoid allocation failures due to fragmented memory, use the allocation with fallback to vmalloc.
Signed-off-by David Sterba <[email protected]>
---
This depends on the patches that remove the 16MiB restriction in the dedupe ioctl, but contextually can be applied to the current code

too.

Quoted 11 lines

Quoted 11 lines
Quoted 11 lines
fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode
Quoted 11 lines
https://patchwork.kernel.org/patch/10374941/
 fs/btrfs/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/btrfs/ioctl.c +0 -0
index b572e38b4b64..a7f517009cd7 100644
@@ -3178,8 +3178,8 @@ static int btrfs_extent_same(struct inode

*src, u64 loff, u64 olen,

Quoted 1 line

Quoted 1 line
Quoted 1 line
fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode · line 3178
Quoted 1 line
fs/btrfs/ioctl.c +0 -0
         * locking. We use an array for the page pointers. Size of

the array is

Quoted 1 line

Quoted 1 line
Quoted 1 line
fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode · line 3179
Quoted 1 line
fs/btrfs/ioctl.c +0 -0
         * bounded by len, which is in turn bounded by

BTRFS_MAX_DEDUPE_LEN.

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode · line 3181
Quoted 2 lines
fs/btrfs/ioctl.c +0 -1
         */
-       cmp.src_pages = kcalloc(num_pages, sizeof(struct page *),

GFP_KERNEL);

Quoted 1 line

Quoted 1 line
Quoted 1 line
fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode · line 3182
Quoted 1 line
fs/btrfs/ioctl.c +0 -1
-       cmp.dst_pages = kcalloc(num_pages, sizeof(struct page *),

GFP_KERNEL);

Quoted 1 line

Quoted 1 line
Quoted 1 line
fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode · line 3181
Quoted 1 line
fs/btrfs/ioctl.c +1 -0
+       cmp.src_pages = kvzalloc(num_pages, sizeof(struct page *),

GFP_KERNEL);

Quoted 1 line

Quoted 1 line
Quoted 1 line
fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode · line 3182
Quoted 1 line
fs/btrfs/ioctl.c +1 -0
+       cmp.dst_pages = kvzalloc(num_pages, sizeof(struct page *),

GFP_KERNEL);

Quoted 21 lines

Quoted 21 lines
Quoted 2 lines
Kvzalloc should take 2 parameters and not 3.
And the right function is kvmalloc_array.
Quoted 1 line
Also, aren't the corresponding kvfree() calls missing?
Yes, thanks for catching it. The updated version:
From: David Sterba <[email protected]>
Subject: [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data
The dedupe range is 16 MiB, with 4KiB pages and 8 byte pointers, the arrays can be 32KiB large. To avoid allocation failures due to fragmented memory, use the allocation with fallback to vmalloc.
Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ioctl.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Quoted 5 lines

fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src,
Quoted 5 lines
fs/btrfs/ioctl.c +0 -0
index b572e38b4b64..4fcfa05ed960 100644
@@ -3178,12 +3178,13 @@ static int btrfs_extent_same(struct inode *src,

u64 loff, u64 olen,

Quoted 1 line

fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, · line 3178
Quoted 1 line
fs/btrfs/ioctl.c +0 -0
       * locking. We use an array for the page pointers. Size of the

array is

Quoted 1 line

fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, · line 3179
Quoted 1 line
fs/btrfs/ioctl.c +0 -0
       * bounded by len, which is in turn bounded by

BTRFS_MAX_DEDUPE_LEN.

Quoted 2 lines

fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, · line 3181
Quoted 2 lines
fs/btrfs/ioctl.c +0 -1
       */
-     cmp.src_pages = kcalloc(num_pages, sizeof(struct page *),

GFP_KERNEL);

Quoted 1 line

fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, · line 3182
Quoted 1 line
fs/btrfs/ioctl.c +0 -1
-     cmp.dst_pages = kcalloc(num_pages, sizeof(struct page *),

GFP_KERNEL);

Quoted 6 lines

fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, · line 3184

Quoted 4 lines

fs/btrfs/ioctl.c +4 -0

+     cmp.src_pages = kvmalloc_array(num_pages, sizeof(struct page *),
+                                    GFP_KERNEL);
+     cmp.dst_pages = kvmalloc_array(num_pages, sizeof(struct page *),
+                                    GFP_KERNEL);

kcalloc() implies __GFP_ZERO, do we need that here?

AFAIK, yes, because:
btrfs_cmp_data_free():
...
pg = cmp->src_pages[i];
if (pg) {...}
..

And we will catch that, if errors happens in gather_extent_pages().

Thanks.

Quoted 10 lines

fs/btrfs/ioctl.c static int btrfs_extent_same(struct inode *src,

Quoted 10 lines

fs/btrfs/ioctl.c +2 -3

      if (!cmp.src_pages || !cmp.dst_pages) {
-             kfree(cmp.src_pages);
-             kfree(cmp.dst_pages);
-             return -ENOMEM;
+             ret = -ENOMEM;
+             goto out_free;
      }
 
      if (same_inode)

@@ -3211,8 +3212,9 @@ static int btrfs_extent_same(struct inode *src,

u64 loff, u64 olen,

Quoted 14 lines

fs/btrfs/ioctl.c +3 -3

      else
              btrfs_double_inode_unlock(src, dst);
 
-     kfree(cmp.src_pages);
-     kfree(cmp.dst_pages);
+out_free:
+     kvfree(cmp.src_pages);
+     kvfree(cmp.dst_pages);
 
      return ret;
 }
--

2.16.2

Signature · 2 lines

Have a nice day,
Timofey.