On Thu, Nov 19, 2015 at 5:55 PM, Brian Foster <bfoster@xxxxxxxxxx> wrote:
> On Wed, Nov 18, 2015 at 12:46:21AM +0200, Octavian Purdila wrote:
>> Naive implementation for non-mmu architectures: allocate physically
>> contiguous xfs buffers with alloc_pages. Terribly inefficient with
>> memory and fragmentation on high I/O loads but it may be good enough
>> for basic usage (which most non-mmu architectures will need).
>>
>> This patch was tested with lklfuse [1] and basic operations seems to
>> work even with 16MB allocated for LKL.
>>
>> [1] https://github.com/lkl/linux
>>
>> Signed-off-by: Octavian Purdila <octavian.purdila@xxxxxxxxx>
>> ---
>
> Interesting, though this makes me wonder why we couldn't have a new
> _XBF_VMEM (for example) buffer type that uses vmalloc(). I'm not
> familiar with mmu-less context, but I see that mm/nommu.c has a
> __vmalloc() interface that looks like it ultimately translates into an
> alloc_pages() call. Would that accomplish what this patch is currently
> trying to do?
>
Hi Brian,
Ah, that sounds nice! We could get rid of the #ifdefs and use a common
path in that case. vmalloc should work on non-mmu and it seems that
there is a vmalloc_to_page() we can use to get the physical pages.
I'll give it a try.
Is there a central place where we could enable the new _XBF_VMEM to be
the default for non-mmu arches?
> I ask because it seems like that would help clean up the code a bit, for
> one. It might also facilitate some degree of testing of the XFS bits
> (even if utilized sparingly in DEBUG mode if it weren't suitable enough
> for generic/mmu use). We currently allocate and map the buffer pages
> separately and I'm not sure if there's any particular reasons for doing
> that outside of some congestion handling in the allocation code and
> XBF_UNMAPPED buffers, the latter probably being irrelevant for nommu.
> Any other thoughts on that?
>
>> fs/xfs/xfs_buf.c | 27 +++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
>> index 8ecffb3..50b5246 100644
>> --- a/fs/xfs/xfs_buf.c
>> +++ b/fs/xfs/xfs_buf.c
> ...
>> @@ -816,11 +835,19 @@ xfs_buf_get_uncached(
>> if (error)
>> goto fail_free_buf;
>>
>> +#ifdef CONFIG_MMU
>> for (i = 0; i < page_count; i++) {
>> bp->b_pages[i] = alloc_page(xb_to_gfp(flags));
>> if (!bp->b_pages[i])
>> goto fail_free_mem;
>> }
>> +#else
>> + bp->b_pages[0] = alloc_pages(flags, order_base_2(page_count));
>> + if (!bp->b_pages[0])
>> + goto fail_free_buf;
>> + for (i = 1; i < page_count; i++)
>> + bp->b_pages[i] = bp->b_pages[i-1] + 1;
>> +#endif
>
> We still have a path into __free_page() further down in this function if
> _xfs_buf_map_pages() fails. Granted, _xfs_buf_map_pages() should
> probably never fail in this case, but it still looks like a land mine at
> the very least.
>
OK. Adding a i = 1; right before the #endif should take care of that,
if the vmalloc approach does not work.
|