Re: a patch to improve cephfs direct io performance

[zhucaifeng <zhucaifeng@unissoft-nj.com>]

Hi, Yan

dio_get_pagevlen() calculate the length sum of multiple iovs that can
be combined into one page vector. If needed, the length sum may be
shortened by ceph_osdc_new_request() in ceph_sync_direct_write().
Nevertheless, the number of pages, derived from the length sum,
may expand across multiple iovs.

However, according to my understanding, iov_iter_get_pages() can only get
pages for one iov a time. To get pages for next iovs, iov_iter_advance()
must be called before calling iov_iter_get_pages() again. But calling
iov_iter_advance() without actual data IO is obviously unacceptable.
That is my reason why iov_iter APIs are unfit for combining mutli iovs
into one page vector.

Best Regards


> On Wed, Sep 30, 2015 at 5:40 PM, zhucaifeng <zhucaifeng@unissoft-nj.com> wrote:
>> Hi, Yan
>>
>> iov_iter APIs seems unsuitable for the direct io manipulation below.
>> iov_iter APIs
>> hide how to iterate over elements, whileas dio_xxx below explicitly control
>> over
>> the iteration. They conflict with each other in the principle.
> why? I think it's easy to replace dio_alloc_pagev() by
> iov_iter_get_pages(). We just need to use dio_get_pagevlen() to
> calculate how many page to get each time.
>
> Regards
> Yan, Zheng
>
>> The patch for the newest kernel branch is below.
>>
>> Best Regards
>>
>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>> index 8b79d87..3938ac9 100644
>> --- a/fs/ceph/file.c
>> +++ b/fs/ceph/file.c
>>
>> @@ -34,6 +34,115 @@
>>    * need to wait for MDS acknowledgement.
>>    */
>>
>> +/*
>> + * Calculate the length sum of direct io vectors that can
>> + * be combined into one page vector.
>> + */
>> +static int
>> +dio_get_pagevlen(const struct iov_iter *it)
>> +{
>> +    const struct iovec *iov = it->iov;
>> +    const struct iovec *iovend = iov + it->nr_segs;
>> +    int pagevlen;
>> +
>> +    pagevlen = iov->iov_len - it->iov_offset;
>> +    /*
>> +     * An iov can be page vectored when both the current tail
>> +     * and the next base are page aligned.
>> +     */
>> +    while (PAGE_ALIGNED((iov->iov_base + iov->iov_len)) &&
>> +           (++iov < iovend && PAGE_ALIGNED((iov->iov_base)))) {
>> +        pagevlen += iov->iov_len;
>> +    }
>> +    dout("dio_get_pagevlen len = %d\n", pagevlen);
>> +    return pagevlen;
>> +}
>> +
>> +/*
>> + * Grab @num_pages from the process vm space. These pages are
>> + * continuous and start from @data.
>> + */
>> +static int
>> +dio_grab_pages(const void *data, int num_pages, bool write_page,
>> +    struct page **pages)
>> +{
>> +    int got = 0;
>> +    int rc = 0;
>> +
>> +    down_read(&current->mm->mmap_sem);
>> +    while (got < num_pages) {
>> +        rc = get_user_pages(current, current->mm,
>> +            (unsigned long)data + ((unsigned long)got * PAGE_SIZE),
>> +            num_pages - got, write_page, 0, pages + got, NULL);
>> +        if (rc < 0)
>> +            break;
>> +        BUG_ON(rc == 0);
>> +        got += rc;
>> +    }
>> +    up_read(&current->mm->mmap_sem);
>> +    return got;
>> +}
>> +
>> +static void
>> +dio_free_pagev(struct page **pages, int num_pages, bool dirty)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < num_pages; i++) {
>> +        if (dirty)
>> +            set_page_dirty_lock(pages[i]);
>> +        put_page(pages[i]);
>> +    }
>> +    kfree(pages);
>> +}
>> +
>> +/*
>> + * Allocate a page vector based on (@it, @pagevlen).
>> + * The return value is the tuple describing a page vector,
>> + * that is (@pages, @pagevlen, @page_align, @num_pages).
>> + */
>> +static struct page **
>> +dio_alloc_pagev(const struct iov_iter *it, int pagevlen, bool write_page,
>> +    size_t *page_align, int *num_pages)
>> +{
>> +    const struct iovec *iov = it->iov;
>> +    struct page **pages;
>> +    int n, m, k, npages;
>> +    int align;
>> +    int len;
>> +    void *data;
>> +
>> +    data = iov->iov_base + it->iov_offset;
>> +    len = iov->iov_len - it->iov_offset;
>> +    align = ((ulong)data) & ~PAGE_MASK;
>> +    npages = calc_pages_for((ulong)data, pagevlen);
>> +    pages = kmalloc(sizeof(*pages) * npages, GFP_NOFS);
>> +    if (!pages)
>> +        return ERR_PTR(-ENOMEM);
>> +    for (n = 0; n < npages; n += m) {
>> +        m = calc_pages_for((ulong)data, len);
>> +        if (n + m > npages)
>> +            m = npages - n;
>> +        k = dio_grab_pages(data, m, write_page, pages + n);
>> +        if (k < m) {
>> +            n += k;
>> +            goto failed;
>> +        }
>> +
>> +        iov++;
>> +        data = iov->iov_base;
>> +        len = iov->iov_len;
>> +    }
>> +    *num_pages = npages;
>> +    *page_align = align;
>> +    dout("dio_alloc_pagev: alloc pages pages[0:%d], page align %d\n",
>> +        npages, align);
>> +    return pages;
>> +
>> +failed:
>> +    dio_free_pagev(pages, n, false);
>> +    return ERR_PTR(-ENOMEM);
>> +}
>>
>>   /*
>>    * Prepare an open request.  Preallocate ceph_cap to avoid an
>> @@ -462,17 +571,17 @@ static ssize_t ceph_sync_read(struct kiocb *iocb,
>> struct iov_iter *i,
>>               size_t start;
>>               ssize_t n;
>>
>> -            n = iov_iter_get_pages_alloc(i, &pages, INT_MAX, &start);
>> -            if (n < 0)
>> -                return n;
>> -
>> -            num_pages = (n + start + PAGE_SIZE - 1) / PAGE_SIZE;
>> +            n = dio_get_pagevlen(i);
>> +            pages = dio_alloc_pagev(i, n, true, &start,
>> +                &num_pages);
>> +            if (IS_ERR(pages))
>> +                return PTR_ERR(pages);
>>
>>               ret = striped_read(inode, off, n,
>>                          pages, num_pages, checkeof,
>>                          1, start);
>>
>> -            ceph_put_page_vector(pages, num_pages, true);
>> +            dio_free_pagev(pages, num_pages, true);
>>
>>               if (ret <= 0)
>>                   break;
>> @@ -596,7 +705,7 @@ ceph_sync_direct_write(struct kiocb *iocb, struct
>> iov_iter *from, loff_t pos,
>>           CEPH_OSD_FLAG_WRITE;
>>
>>       while (iov_iter_count(from) > 0) {
>> -        u64 len = iov_iter_single_seg_count(from);
>> +        u64 len = dio_get_pagevlen(from);
>>           size_t start;
>>           ssize_t n;
>>
>> @@ -615,14 +724,15 @@ ceph_sync_direct_write(struct kiocb *iocb, struct
>> iov_iter *from, loff_t pos,
>>
>>           osd_req_op_init(req, 1, CEPH_OSD_OP_STARTSYNC, 0);
>>
>> -        n = iov_iter_get_pages_alloc(from, &pages, len, &start);
>> -        if (unlikely(n < 0)) {
>> -            ret = n;
>> +        n = len;
>> +        pages = dio_alloc_pagev(from, len, false, &start,
>> +            &num_pages);
>> +        if (IS_ERR(pages)) {
>>               ceph_osdc_put_request(req);
>> +            ret = PTR_ERR(pages);
>>               break;
>>           }
>>
>> -        num_pages = (n + start + PAGE_SIZE - 1) / PAGE_SIZE;
>>           /*
>>            * throw out any page cache pages in this range. this
>>            * may block.
>> @@ -639,8 +749,7 @@ ceph_sync_direct_write(struct kiocb *iocb, struct
>> iov_iter *from, loff_t pos,
>>           if (!ret)
>>               ret = ceph_osdc_wait_request(&fsc->client->osdc, req);
>>
>> -        ceph_put_page_vector(pages, num_pages, false);
>> -
>> +        dio_free_pagev(pages, num_pages, false);
>>           ceph_osdc_put_request(req);
>>           if (ret)
>>               break;
>>
>>
>>
>>> Hi, zhucaifeng
>>>
>>> The patch generally looks good. But I think there is minor flaw, see
>>> my comment below. Besides, could you write a patch for the newest
>>> kernel (it's easier because there is iov_iter_get_pages() helper).
>>>
>>>
>>> On Fri, Sep 25, 2015 at 11:52 AM, zhucaifeng <zhucaifeng@unissoft-nj.com>
>>> wrote:
>>>> Hi, all
>>>>
>>>> When using cephfs, we find that cephfs direct io is very slow.
>>>> For example, installing Windows 7 takes more than 1 hour on a
>>>> virtual machine whose disk is a file in cephfs.
>>>>
>>>> The cause is that when doing direct io, both ceph_sync_direct_write
>>>> and ceph_sync_read iterate iov elements one by one, and send one
>>>> osd_op request for each iov that covers about 4K~8K bytes data. The
>>>> result is lots of small requests and replies shuttled among the kernel
>>>> client and OSDs.
>>>>
>>>> The fix tries to combine as many iovs as possible into one page vector,
>>>> and
>>>> then send one request for this page vector. In this way, small requests
>>>> and
>>>> replies are reduced; the OSD can serve a large read/write request one
>>>> time.
>>>>
>>>> We observe that the performance is improved by about 5~15 times,
>>>> dependent
>>>> on
>>>> different application workload.
>>>>
>>>> The fix is below. Some change notes are as follows:
>>>>       - function ceph_get_direct_page_vetor and ceph_put_page_vector
>>>>         are moved from net/ceph/pagevec.c to fs/ceph/file.c, and are
>>>>         renamed as dio_grab_pages and dio_free_pagev respectively.
>>>>
>>>>       - function dio_get_pagevlen and dio_alloc_pagev are newly
>>>> introduced.
>>>>
>>>>         dio_get_pagevlen is to calculate the page vector length in bytes
>>>>         from the io vectors. Except for the first and last one, an io
>>>> vector
>>>>         can be merged into a page vector iff its base and tail are page
>>>> aligned.
>>>>         for the first vector, only its tail should be page aligned; for
>>>> the
>>>> last
>>>>         vector, only its head should be page aligned.
>>>>
>>>>         dio_alloc_pagev allocates pages for each io vector and put these
>>>> pages
>>>>         together in one page pointer array.
>>>>
>>>> Best Regards!
>>>>
>>>> --- /home/linux-3.10.0-229.7.2.el7/fs/ceph/file.c    2015-05-16
>>>> 09:05:28.000000000 +0800
>>>> +++ ./fs/ceph/file.c    2015-09-17 10:43:44.798591846 +0800
>>>> @@ -34,6 +34,115 @@
>>>>     * need to wait for MDS acknowledgement.
>>>>     */
>>>>
>>>> +/*
>>>> + * Calculate the length sum of direct io vectors that can
>>>> + * be combined into one page vector.
>>>> + */
>>>> +static int
>>>> +dio_get_pagevlen(const struct iov_iter *it)
>>>> +{
>>>> +    const struct iovec *iov = it->iov;
>>>> +    const struct iovec *iovend = iov + it->nr_segs;
>>>> +    int pagevlen;
>>>> +
>>>> +    pagevlen = iov->iov_len - it->iov_offset;
>>>> +    /*
>>>> +     * An iov can be page vectored when both the current tail
>>>> +     * and the next base are page aligned.
>>>> +     */
>>>> +    while (PAGE_ALIGNED((iov->iov_base + iov->iov_len)) &&
>>>> +           (++iov < iovend && PAGE_ALIGNED((iov->iov_base)))) {
>>>> +        pagevlen += iov->iov_len;
>>>> +    }
>>>> +    dout("dio_get_pagevlen len = %d\n", pagevlen);
>>>> +    return pagevlen;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Grab @num_pages from the process vm space. These pages are
>>>> + * continuous and start from @data.
>>>> + */
>>>> +static int
>>>> +dio_grab_pages(const void *data, int num_pages, bool write_page,
>>>> +    struct page **pages)
>>>> +{
>>>> +    int got = 0;
>>>> +    int rc = 0;
>>>> +
>>>> +    down_read(&current->mm->mmap_sem);
>>>> +    while (got < num_pages) {
>>>> +        rc = get_user_pages(current, current->mm,
>>>> +            (unsigned long)data + ((unsigned long)got * PAGE_SIZE),
>>>> +            num_pages - got, write_page, 0, pages + got, NULL);
>>>> +        if (rc < 0)
>>>> +            break;
>>>> +        BUG_ON(rc == 0);
>>>> +        got += rc;
>>>> +    }
>>>> +    up_read(&current->mm->mmap_sem);
>>>> +    return got;
>>>> +}
>>>> +
>>>> +static void
>>>> +dio_free_pagev(struct page **pages, int num_pages, bool dirty)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < num_pages; i++) {
>>>> +        if (dirty)
>>>> +            set_page_dirty_lock(pages[i]);
>>>> +        put_page(pages[i]);
>>>> +    }
>>>> +    kfree(pages);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Allocate a page vector based on (@it, @pagevlen).
>>>> + * The return value is the tuple describing a page vector,
>>>> + * that is (@pages, @pagevlen, @page_align, @num_pages).
>>>> + */
>>>> +static struct page **
>>>> +dio_alloc_pagev(const struct iov_iter *it, int pagevlen, bool
>>>> write_page,
>>>> +    int *page_align, int *num_pages)
>>>> +{
>>>> +    const struct iovec *iov = it->iov;
>>>> +    struct page **pages;
>>>> +    int n, m, k, npages;
>>>> +    int align;
>>>> +    int len;
>>>> +    void *data;
>>>> +
>>>> +    data = iov->iov_base + it->iov_offset;
>>>> +    len = iov->iov_len - it->iov_offset;
>>>> +    align = ((ulong)data) & ~PAGE_MASK;
>>>> +    npages = calc_pages_for((ulong)data, pagevlen);
>>>> +    pages = kmalloc(sizeof(*pages) * npages, GFP_NOFS);
>>>> +    if (!pages)
>>>> +        return ERR_PTR(-ENOMEM);
>>>> +    for (n = 0; n < npages; n += m) {
>>>> +        m = calc_pages_for((ulong)data, len);
>>>> +        if (n + m > npages)
>>>> +            m = npages - n;
>>>> +        k = dio_grab_pages(data, m, write_page, pages + n);
>>>> +        if (k < m) {
>>>> +            n += k;
>>>> +            goto failed;
>>>> +        }
>>> if (align + len) is not page aligned, the loop should stop.
>>>
>>>> +
>>>> +        iov++;
>>>> +        data = iov->iov_base;
>>> if (unsigned long)data is not page aligned, the loop should stop
>>>
>>>
>>> Regards
>>> Yan, Zheng
>>>
>>>> +        len = iov->iov_len;
>>>> +    }
>>>> +    *num_pages = npages;
>>>> +    *page_align = align;
>>>> +    dout("dio_alloc_pagev: alloc pages pages[0:%d], page align %d\n",
>>>> +        npages, align);
>>>> +    return pages;
>>>> +
>>>> +failed:
>>>> +    dio_free_pagev(pages, n, false);
>>>> +    return ERR_PTR(-ENOMEM);
>>>> +}
>>>>
>>>>    /*
>>>>     * Prepare an open request.  Preallocate ceph_cap to avoid an
>>>> @@ -354,13 +463,14 @@ more:
>>>>        if (ret >= 0) {
>>>>            int didpages;
>>>>            if (was_short && (pos + ret < inode->i_size)) {
>>>> -            u64 tmp = min(this_len - ret,
>>>> +            u64 zlen = min(this_len - ret,
>>>>                        inode->i_size - pos - ret);
>>>> +            int zoff = (o_direct ? buf_align : io_align) +
>>>> +                   read + ret;
>>>>                dout(" zero gap %llu to %llu\n",
>>>> -                pos + ret, pos + ret + tmp);
>>>> -            ceph_zero_page_vector_range(page_align + read + ret,
>>>> -                            tmp, pages);
>>>> -            ret += tmp;
>>>> +                pos + ret, pos + ret + zlen);
>>>> +            ceph_zero_page_vector_range(zoff, zlen, pages);
>>>> +            ret += zlen;
>>>>            }
>>>>
>>>>            didpages = (page_align + ret) >> PAGE_CACHE_SHIFT;
>>>> @@ -421,19 +531,19 @@ static ssize_t ceph_sync_read(struct kio
>>>>
>>>>        if (file->f_flags & O_DIRECT) {
>>>>            while (iov_iter_count(i)) {
>>>> -            void __user *data = i->iov[0].iov_base + i->iov_offset;
>>>> -            size_t len = i->iov[0].iov_len - i->iov_offset;
>>>> -
>>>> -            num_pages = calc_pages_for((unsigned long)data, len);
>>>> -            pages = ceph_get_direct_page_vector(data,
>>>> -                                num_pages, true);
>>>> +            int page_align;
>>>> +            size_t len;
>>>> +
>>>> +            len = dio_get_pagevlen(i);
>>>> +            pages = dio_alloc_pagev(i, len, true, &page_align,
>>>> +                &num_pages);
>>>>                if (IS_ERR(pages))
>>>>                    return PTR_ERR(pages);
>>>>
>>>>                ret = striped_read(inode, off, len,
>>>>                           pages, num_pages, checkeof,
>>>> -                       1, (unsigned long)data & ~PAGE_MASK);
>>>> -            ceph_put_page_vector(pages, num_pages, true);
>>>> +                       1, page_align);
>>>> +            dio_free_pagev(pages, num_pages, true);
>>>>
>>>>                if (ret <= 0)
>>>>                    break;
>>>> @@ -570,10 +680,7 @@ ceph_sync_direct_write(struct kiocb *ioc
>>>>        iov_iter_init(&i, iov, nr_segs, count, 0);
>>>>
>>>>        while (iov_iter_count(&i) > 0) {
>>>> -        void __user *data = i.iov->iov_base + i.iov_offset;
>>>> -        u64 len = i.iov->iov_len - i.iov_offset;
>>>> -
>>>> -        page_align = (unsigned long)data & ~PAGE_MASK;
>>>> +        u64 len = dio_get_pagevlen(&i);
>>>>
>>>>            snapc = ci->i_snap_realm->cached_context;
>>>>            vino = ceph_vino(inode);
>>>> @@ -589,8 +696,7 @@ ceph_sync_direct_write(struct kiocb *ioc
>>>>                break;
>>>>            }
>>>>
>>>> -        num_pages = calc_pages_for(page_align, len);
>>>> -        pages = ceph_get_direct_page_vector(data, num_pages, false);
>>>> +        pages = dio_alloc_pagev(&i, len, false, &page_align,
>>>> &num_pages);
>>>>            if (IS_ERR(pages)) {
>>>>                ret = PTR_ERR(pages);
>>>>                goto out;
>>>> @@ -612,7 +718,7 @@ ceph_sync_direct_write(struct kiocb *ioc
>>>>            if (!ret)
>>>>                ret = ceph_osdc_wait_request(&fsc->client->osdc, req);
>>>>
>>>> -        ceph_put_page_vector(pages, num_pages, false);
>>>> +        dio_free_pagev(pages, num_pages, false);
>>>>
>>>>    out:
>>>>            ceph_osdc_put_request(req);
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>