On 2015/11/07 1:17, mhocko@xxxxxxxxxx wrote:
> From: Michal Hocko <mhocko@xxxxxxxx>
>
> jbd2_alloc is explicit about its allocation preferences wrt. the
> allocation size. Sub page allocations go to the slab allocator
> and larger are using either the page allocator or vmalloc. This
> is all good but the logic is unnecessarily complex. Requests larger
> than order-3 are doing the vmalloc directly while smaller go to the
> page allocator with __GFP_REPEAT. The flag doesn't do anything useful
> for those because they are smaller than PAGE_ALLOC_COSTLY_ORDER.
>
> Let's simplify the code flow and use kmalloc for sub-page requests
> and the page allocator for others with fallback to vmalloc if the
> allocation fails.
>
> Cc: "Theodore Ts'o" <tytso@xxxxxxx>
> Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>
> ---
> fs/jbd2/journal.c | 35 ++++++++++++-----------------------
> 1 file changed, 12 insertions(+), 23 deletions(-)
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 81e622681c82..2945c96f171f 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -2299,18 +2299,15 @@ void *jbd2_alloc(size_t size, gfp_t flags)
>
> BUG_ON(size & (size-1)); /* Must be a power of 2 */
>
> - flags |= __GFP_REPEAT;
> - if (size == PAGE_SIZE)
> - ptr = (void *)__get_free_pages(flags, 0);
> - else if (size > PAGE_SIZE) {
> + if (size < PAGE_SIZE)
> + ptr = kmem_cache_alloc(get_slab(size), flags);
> + else {
> int order = get_order(size);
>
> - if (order < 3)
> - ptr = (void *)__get_free_pages(flags, order);
> - else
> + ptr = (void *)__get_free_pages(flags, order);
I thought that we can add __GFP_NOWARN for this __get_free_pages() call.
But I noticed more important problem. See below.
> + if (!ptr)
> ptr = vmalloc(size);
> - } else
> - ptr = kmem_cache_alloc(get_slab(size), flags);
> + }
>
> /* Check alignment; SLUB has gotten this wrong in the past,
> * and this can lead to user data corruption! */
> @@ -2321,20 +2318,12 @@ void *jbd2_alloc(size_t size, gfp_t flags)
>
> void jbd2_free(void *ptr, size_t size)
> {
> - if (size == PAGE_SIZE) {
> - free_pages((unsigned long)ptr, 0);
> - return;
> - }
> - if (size > PAGE_SIZE) {
> - int order = get_order(size);
> -
> - if (order < 3)
> - free_pages((unsigned long)ptr, order);
> - else
> - vfree(ptr);
> - return;
> - }
> - kmem_cache_free(get_slab(size), ptr);
> + if (size < PAGE_SIZE)
> + kmem_cache_free(get_slab(size), ptr);
> + else if (is_vmalloc_addr(ptr))
> + vfree(ptr);
> + else
> + free_pages((unsigned long)ptr, get_order(size));
> };
>
> /*
>
All jbd2_alloc() callers seem to pass GFP_NOFS. Therefore, use of
vmalloc() which implicitly passes GFP_KERNEL | __GFP_HIGHMEM can cause
deadlock, can't it? This vmalloc(size) call needs to be replaced with
__vmalloc(size, flags).
We need to check all vmalloc() callers in case they are calling vmalloc()
under GFP_KERNEL-unsafe context. For example, I think that __aa_kvmalloc()
needs to use __vmalloc() too.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>