* [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 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
* 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
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.