Ceph » Linux kernel client

From the call trace, we can see that cephfs wanted to free one page, but this page's PG_lru flag was set. According to our use case, the memory of this classification process is controlled by memory cgroup. So I think this page's PG_lru flag might be set when memory cgroup charges its memory usage.

I have compared both kernel version 3.10.x and 4.6. Above logic is similar and cephfs will also call ceph_release_page_vector to free pages when something wrong in start_read.

I think there might be 2 potential problems in start_read:

1. From both kernel version 3.10.x and 4.6, get_page will be called on the page if add_to_page_cache_lru successfully did its work. If something wrong in add_to_page_cache_lru, get_page won't be called on the page in 3.10.x and put_page will be called on the page in 4.6, so I think we don't need to call put_page in start_read.

2. We use ceph_release_page_vector in start_read to free rest pages in case of something wrong and ceph_release_page_vector calls __free_pages to free related pages. But since get_page is called in add_to_page_cache_lru, is it better to use corresponding put_page to release these pages? And put_page itself will check page's PG_lru flag and clear it in both 3.10.x and 4.6, but __free_pages won't.

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index d5b6f95..28b7c83 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -366,7 +366,6 @@ static int start_read(struct inode *inode, struct list_head *page_list, int max)
                if (add_to_page_cache_lru(page, &inode->i_data, page->index,
                                          GFP_KERNEL)) {
                        ceph_fscache_uncache_page(inode, page);
-                       put_page(page);
                        dout("start_read %p add_to_page_cache failed %p\n",
                             inode, page);
                        nr_pages = i;
@@ -387,7 +386,7 @@ static int start_read(struct inode *inode, struct list_head *page_list, int max)

 out_pages:
        ceph_unlock_page_vector(pages, nr_pages);
-       ceph_release_page_vector(pages, nr_pages);
+       ceph_put_page_vector(pages, nr_pages, false);
 out:
        ceph_osdc_put_request(req);
        return ret;

Hi Yan,

Could you pls help take a look? Is my understanding correct? Thanks.

I think you are right. Thank you for tracking this down

I think we still need the put_page. just need to replace ceph_release_page_vector with ceph_put_page_vector

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index d5b6f95..eadf273 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -298,14 +298,6 @@ unlock:
        kfree(osd_data->pages);
 }

-static void ceph_unlock_page_vector(struct page **pages, int num_pages)
-{
-       int i;
-
-       for (i = 0; i < num_pages; i++)
-               unlock_page(pages[i]);
-}
-
 /*
  * start an async read(ahead) operation.  return nr_pages we submitted
  * a read for on success, or negative error code.
@@ -370,6 +362,10 @@ static int start_read(struct inode *inode, struct list_head *page_list, int max)
                        dout("start_read %p add_to_page_cache failed %p\n",
                             inode, page);
                        nr_pages = i;
+                       if (nr_pages > 0) {
+                               len = nr_pages << PAGE_SHIFT;
+                               break;
+                       }
                        goto out_pages;
                }
                pages[i] = page;
@@ -386,8 +382,11 @@ static int start_read(struct inode *inode, struct list_head *page_list, int max)
        return nr_pages;

 out_pages:
-       ceph_unlock_page_vector(pages, nr_pages);
-       ceph_release_page_vector(pages, nr_pages);
+       for (i = 0; i < nr_pages; ++i) {
+               ceph_fscache_readpage_cancel(inode, pages[i]);
+               unlock_page(pages[i]);
+       }
+       ceph_release_put_vector(pages, nr_pages, false);
 out:
        ceph_osdc_put_request(req);
        return ret;

Hi Yan,

Thanks for the changes. I can understand what your changes try to do, except one thing: why does put_page still need? Could you pls give me some hint?

because the line "pages[i] = page;" is after "goto out_page"

Zheng Yan wrote:

because the line "pages[i] = page;" is after "goto out_page"

From my understanding, if one single page succeed to add_to_page_cache_lru, then this single page is added to pages array. If one single page failed to add_to_page_cache_lru, then this single page is not added to pages array and nr_pages would be reset to the number of pages which succeed to add_to_page_cache_lru before.

So I think put_page here was planed to use for the single page which failed to add_to_page_cache_lru. It doesn't seem to have something related to pages array.

What do you think? Thanks.

Zhi Zhang wrote:

So I think put_page here was planed to use for the single page which failed to add_to_page_cache_lru. It doesn't seem to have something related to pages array.

What do you think? Thanks.

yes, The failed page does not related to the pages array. But the page was removed from page list, so we need to put a reference.

Zheng Yan wrote:

Zhi Zhang wrote:

So I think put_page here was planed to use for the single page which failed to add_to_page_cache_lru. It doesn't seem to have something related to pages array.

What do you think? Thanks.

yes, The failed page does not related to the pages array. But the page was removed from page list, so we need to put a reference.

you're right! thumb!

I have read out the code where increased page's ref at the first place. Thanks.