lkml.org 
[lkml]   [2012]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [Patch] tmpfs: clean up shmem_find_get_pages_and_swap()
    On Tue, 24 Jan 2012, Cong Wang wrote:

    > This patch cleans up shmem_find_get_pages_and_swap() interface:
    >
    > a) Pass struct pagevec* instead of ->pages
    > b) Check if nr_pages is greater than PAGEVEC_SIZE inside the function
    > c) Return the result via ->nr instead of using return value
    >
    > Compiling test only.
    >
    > Cc: Hugh Dickins <hughd@google.com>
    > Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

    I don't see any cleanup: just a difference in taste, I suppose.

    shmem_find_get_pages_and_swap() is modelled on find_get_pages():
    I'd prefer to keep it that way unless there's good reason to diverge.

    I do see a slight change in behaviour, where you've undone the range
    limitation (coming from invalidate_inode_pages2_range() originally, but
    now in several functions in mm/truncate.c). I think you misunderstood

    > - min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1,

    When doing a range, that's trying to stop the radix_tree gang lookup
    looking further than could ever be necessary. Not a big deal, it is
    imperfect anyway - makes more sense when the range is 1 than larger;
    but shouldn't be undone without justification.

    Hugh

    >
    > ---
    > diff --git a/mm/shmem.c b/mm/shmem.c
    > index 269d049..c4e08e2 100644
    > --- a/mm/shmem.c
    > +++ b/mm/shmem.c
    > @@ -312,15 +312,19 @@ static void shmem_delete_from_page_cache(struct page *page, void *radswap)
    > /*
    > * Like find_get_pages, but collecting swap entries as well as pages.
    > */
    > -static unsigned shmem_find_get_pages_and_swap(struct address_space *mapping,
    > +static void shmem_find_get_pages_and_swap(struct address_space *mapping,
    > pgoff_t start, unsigned int nr_pages,
    > - struct page **pages, pgoff_t *indices)
    > + struct pagevec *pvec, pgoff_t *indices)
    > {
    > unsigned int i;
    > unsigned int ret;
    > unsigned int nr_found;
    > + struct page **pages = pvec->pages;
    >
    > rcu_read_lock();
    > +
    > + if (nr_pages > PAGEVEC_SIZE)
    > + nr_pages = PAGEVEC_SIZE;
    > restart:
    > nr_found = radix_tree_gang_lookup_slot(&mapping->page_tree,
    > (void ***)pages, indices, start, nr_pages);
    > @@ -357,7 +361,7 @@ export:
    > if (unlikely(!ret && nr_found))
    > goto restart;
    > rcu_read_unlock();
    > - return ret;
    > + pvec->nr = ret;
    > }
    >
    > /*
    > @@ -409,8 +413,8 @@ void shmem_unlock_mapping(struct address_space *mapping)
    > * Avoid pagevec_lookup(): find_get_pages() returns 0 as if it
    > * has finished, if it hits a row of PAGEVEC_SIZE swap entries.
    > */
    > - pvec.nr = shmem_find_get_pages_and_swap(mapping, index,
    > - PAGEVEC_SIZE, pvec.pages, indices);
    > + shmem_find_get_pages_and_swap(mapping, index,
    > + PAGEVEC_SIZE, &pvec, indices);
    > if (!pvec.nr)
    > break;
    > index = indices[pvec.nr - 1] + 1;
    > @@ -442,9 +446,8 @@ void shmem_truncate_range(struct inode *inode, loff_t lstart, loff_t lend)
    > pagevec_init(&pvec, 0);
    > index = start;
    > while (index <= end) {
    > - pvec.nr = shmem_find_get_pages_and_swap(mapping, index,
    > - min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1,
    > - pvec.pages, indices);
    > + shmem_find_get_pages_and_swap(mapping, index,
    > + end - index + 1, &pvec, indices);
    > if (!pvec.nr)
    > break;
    > mem_cgroup_uncharge_start();
    > @@ -490,9 +493,8 @@ void shmem_truncate_range(struct inode *inode, loff_t lstart, loff_t lend)
    > index = start;
    > for ( ; ; ) {
    > cond_resched();
    > - pvec.nr = shmem_find_get_pages_and_swap(mapping, index,
    > - min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1,
    > - pvec.pages, indices);
    > + shmem_find_get_pages_and_swap(mapping, index,
    > + end - index + 1, &pvec, indices);
    > if (!pvec.nr) {
    > if (index == start)
    > break;


    \
     
     \ /
      Last update: 2012-01-26 00:27    [W:4.288 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site