* [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data
@ 2018-05-11 15:57 David Sterba
2018-05-11 16:25 ` Filipe Manana
0 siblings, 1 reply; 7+ messages in thread
From: David Sterba @ 2018-05-11 15:57 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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 <dsterba@suse.com>
---
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(-)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index b572e38b4b64..a7f517009cd7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -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);
--
2.16.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data
2018-05-11 15:57 [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data David Sterba
@ 2018-05-11 16:25 ` Filipe Manana
2018-05-11 16:49 ` David Sterba
0 siblings, 1 reply; 7+ messages in thread
From: Filipe Manana @ 2018-05-11 16:25 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
On Fri, May 11, 2018 at 4:57 PM, David Sterba <dsterba@suse.com> wrote:
> 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 <dsterba@suse.com>
> ---
>
> 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(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index b572e38b4b64..a7f517009cd7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -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?
> 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data
2018-05-11 16:25 ` Filipe Manana
@ 2018-05-11 16:49 ` David Sterba
2018-05-11 17:16 ` Filipe Manana
2018-05-11 17:31 ` Omar Sandoval
0 siblings, 2 replies; 7+ messages in thread
From: David Sterba @ 2018-05-11 16:49 UTC (permalink / raw)
To: Filipe Manana; +Cc: David Sterba, linux-btrfs
On Fri, May 11, 2018 at 05:25:50PM +0100, Filipe Manana wrote:
> On Fri, May 11, 2018 at 4:57 PM, David Sterba <dsterba@suse.com> wrote:
> > 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 <dsterba@suse.com>
> > ---
> >
> > 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(-)
> >
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index b572e38b4b64..a7f517009cd7 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -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.
> Also, aren't the corresponding kvfree() calls missing?
Yes, thanks for catching it. The updated version:
From: David Sterba <dsterba@suse.com>
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 <dsterba@suse.com>
---
fs/btrfs/ioctl.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index b572e38b4b64..4fcfa05ed960 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -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;
}
--
2.16.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data
2018-05-11 16:49 ` David Sterba
@ 2018-05-11 17:16 ` Filipe Manana
2018-05-11 21:01 ` David Sterba
2018-05-11 17:31 ` Omar Sandoval
1 sibling, 1 reply; 7+ messages in thread
From: Filipe Manana @ 2018-05-11 17:16 UTC (permalink / raw)
To: dsterba, Filipe Manana, David Sterba, linux-btrfs
On Fri, May 11, 2018 at 5:49 PM, David Sterba <dsterba@suse.cz> wrote:
> On Fri, May 11, 2018 at 05:25:50PM +0100, Filipe Manana wrote:
>> On Fri, May 11, 2018 at 4:57 PM, David Sterba <dsterba@suse.com> wrote:
>> > 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 <dsterba@suse.com>
>> > ---
>> >
>> > 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(-)
>> >
>> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> > index b572e38b4b64..a7f517009cd7 100644
>> > --- a/fs/btrfs/ioctl.c
>> > +++ b/fs/btrfs/ioctl.c
>> > @@ -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.
>
>> Also, aren't the corresponding kvfree() calls missing?
>
> Yes, thanks for catching it. The updated version:
>
> From: David Sterba <dsterba@suse.com>
> 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 <dsterba@suse.com>
> ---
> fs/btrfs/ioctl.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index b572e38b4b64..4fcfa05ed960 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -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);
kvfree() missing at btrfs_cmp_data_free() too.
>
> return ret;
> }
> --
> 2.16.2
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data
2018-05-11 16:49 ` David Sterba
2018-05-11 17:16 ` Filipe Manana
@ 2018-05-11 17:31 ` Omar Sandoval
2018-05-11 17:47 ` Timofey Titovets
1 sibling, 1 reply; 7+ messages in thread
From: Omar Sandoval @ 2018-05-11 17:31 UTC (permalink / raw)
To: dsterba, Filipe Manana, David Sterba, linux-btrfs
On Fri, May 11, 2018 at 06:49:16PM +0200, David Sterba wrote:
> On Fri, May 11, 2018 at 05:25:50PM +0100, Filipe Manana wrote:
> > On Fri, May 11, 2018 at 4:57 PM, David Sterba <dsterba@suse.com> wrote:
> > > 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 <dsterba@suse.com>
> > > ---
> > >
> > > 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(-)
> > >
> > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > > index b572e38b4b64..a7f517009cd7 100644
> > > --- a/fs/btrfs/ioctl.c
> > > +++ b/fs/btrfs/ioctl.c
> > > @@ -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.
>
> > Also, aren't the corresponding kvfree() calls missing?
>
> Yes, thanks for catching it. The updated version:
>
> From: David Sterba <dsterba@suse.com>
> 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 <dsterba@suse.com>
> ---
> fs/btrfs/ioctl.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index b572e38b4b64..4fcfa05ed960 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -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?
> 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;
> }
> --
> 2.16.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data
2018-05-11 17:31 ` Omar Sandoval
@ 2018-05-11 17:47 ` Timofey Titovets
0 siblings, 0 replies; 7+ messages in thread
From: Timofey Titovets @ 2018-05-11 17:47 UTC (permalink / raw)
To: osandov; +Cc: David Sterba, Filipe Manana, David Sterba, linux-btrfs
пт, 11 мая 2018 г. в 20:32, Omar Sandoval <osandov@osandov.com>:
> On Fri, May 11, 2018 at 06:49:16PM +0200, David Sterba wrote:
> > On Fri, May 11, 2018 at 05:25:50PM +0100, Filipe Manana wrote:
> > > On Fri, May 11, 2018 at 4:57 PM, David Sterba <dsterba@suse.com>
wrote:
> > > > 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 <dsterba@suse.com>
> > > > ---
> > > >
> > > > 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(-)
> > > >
> > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > > > index b572e38b4b64..a7f517009cd7 100644
> > > > --- a/fs/btrfs/ioctl.c
> > > > +++ b/fs/btrfs/ioctl.c
> > > > @@ -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.
> >
> > > Also, aren't the corresponding kvfree() calls missing?
> >
> > Yes, thanks for catching it. The updated version:
> >
> > From: David Sterba <dsterba@suse.com>
> > 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 <dsterba@suse.com>
> > ---
> > fs/btrfs/ioctl.c | 16 +++++++++-------
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index b572e38b4b64..4fcfa05ed960 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -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?
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.
> > 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;
> > }
> > --
> > 2.16.2
> >
--
Have a nice day,
Timofey.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data
2018-05-11 17:16 ` Filipe Manana
@ 2018-05-11 21:01 ` David Sterba
0 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2018-05-11 21:01 UTC (permalink / raw)
To: Filipe Manana; +Cc: dsterba, David Sterba, linux-btrfs
On Fri, May 11, 2018 at 06:16:15PM +0100, Filipe Manana wrote:
> On Fri, May 11, 2018 at 5:49 PM, David Sterba <dsterba@suse.cz> wrote:
> > On Fri, May 11, 2018 at 05:25:50PM +0100, Filipe Manana wrote:
> >> On Fri, May 11, 2018 at 4:57 PM, David Sterba <dsterba@suse.com> wrote:
> >> > 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 <dsterba@suse.com>
> >> > ---
> >> >
> >> > 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(-)
> >> >
> >> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> >> > index b572e38b4b64..a7f517009cd7 100644
> >> > --- a/fs/btrfs/ioctl.c
> >> > +++ b/fs/btrfs/ioctl.c
> >> > @@ -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.
> >
> >> Also, aren't the corresponding kvfree() calls missing?
> >
> > Yes, thanks for catching it. The updated version:
> >
> > From: David Sterba <dsterba@suse.com>
> > 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 <dsterba@suse.com>
> > ---
> > fs/btrfs/ioctl.c | 16 +++++++++-------
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index b572e38b4b64..4fcfa05ed960 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -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);
>
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-05-11 21:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-11 15:57 [PATCH] btrfs: use kvzalloc for EXTENT_SAME temporary data David Sterba
2018-05-11 16:25 ` Filipe Manana
2018-05-11 16:49 ` David Sterba
2018-05-11 17:16 ` Filipe Manana
2018-05-11 21:01 ` David Sterba
2018-05-11 17:31 ` Omar Sandoval
2018-05-11 17:47 ` Timofey Titovets
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.