why are some low-level MM routines being exported?

perusing the code in mm/filemap.c and i'm curious as to why routines like, for example, add_to_page_cache_lru() are being exported. is it really expected that loadable modules might access routines like that directly?

Quoted 4 lines

perusing the code in mm/filemap.c and i'm curious as to why routines like, for example, add_to_page_cache_lru() are being exported. is it really expected that loadable modules might access routines like that directly?

It is added by 18bc0bbd162e3 for pohmelfs and now used by logfs, too. I didn't noticed that at that time.
With git log, any mm guys didn't add Signed-off-by or Reviewed-by.

I think it's not good for file system or module to use it directly. It would make LRU management harder.

Is it really needed? Let's think again.

Quoted 11 lines · Minchan Kim

Quoted 4 lines

perusing the code in mm/filemap.c and i'm curious as to why routines like, for example, add_to_page_cache_lru() are being exported. is it really expected that loadable modules might access routines like that directly?

It is added by 18bc0bbd162e3 for pohmelfs and now used by logfs, too. I didn't noticed that at that time.
With git log, any mm guys didn't add Signed-off-by or Reviewed-by.

I think it's not good for file system or module to use it directly. It would make LRU management harder.

How come?

Quoted 1 line

Is it really needed? Let's think again.

Yes, it is really needed. It is not a some king of low-level mm magic to export, but a useful interface to work with LRU lists instead of copy-paste it into own machinery.

Quoted 5 lines · Minchan Kim

Quoted 4 lines

perusing the code in mm/filemap.c and i'm curious as to why routines like, for example, add_to_page_cache_lru() are being exported. is it really expected that loadable modules might access routines like that directly?

Quoted 6 lines

It is added by 18bc0bbd162e3 for pohmelfs and now used by logfs, too. I didn't noticed that at that time.
With git log, any mm guys didn't add Signed-off-by or Reviewed-by.

I think it's not good for file system or module to use it directly. It would make LRU management harder.

How come?

Quoted 2 lines

Is it really needed? Let's think again.

Yes, it is really needed. It is not a some king of low-level mm magic to export, but a useful interface to work with LRU lists instead of copy-paste it into own machinery.

Quoted 4 lines

Quoted 4 lines

It is added by 18bc0bbd162e3 for pohmelfs and now used by logfs, too. I didn't noticed that at that time.
With git log, any mm guys didn't add Signed-off-by or Reviewed-by.

Quoted 4 lines

Quoted 2 lines

I think it's not good for file system or module to use it directly. It would make LRU management harder.

How come?

What I have a concern is that if file systems or some modules start to overuse it to manage pages LRU directly, some mistake of them would make system global LRU stupid and make system wrong.

Quoted 9 lines · Minchan Kim

Quoted 8 lines

Quoted 6 lines

It is added by 18bc0bbd162e3 for pohmelfs and now used by logfs, too. I didn't noticed that at that time.
With git log, any mm guys didn't add Signed-off-by or Reviewed-by.

I think it's not good for file system or module to use it directly. It would make LRU management harder.

How come?

Quoted 3 lines

What I have a concern is that if file systems or some modules start to overuse it to manage pages LRU directly, some mistake of them would make system global LRU stupid and make system wrong.

All filesystems already call it through find_or_create_page() or grab_page() invoked via read path. In some cases fs has more than one page grabbed via its internal path where data to be read is already placed, so it may want just to add those pages into mm lru.

Quoted 4 lines

All filesystems already call it through find_or_create_page() or grab_page() invoked via read path. In some cases fs has more than one page grabbed via its internal path where data to be read is already placed, so it may want just to add those pages into mm lru.

I understood why it does need that in pohmelfs.
AFAIU, other file system using general functions(ex, mpage_readpages or read_cache_pages) don't need direct LRU handling since it's hided. But pohmelfs doesn't use general functions.

Isn't pagevec_lru_add_file enough like other file system(ex, nfs, cifs)?

Quoted 11 lines · Minchan Kim

Quoted 4 lines

All filesystems already call it through find_or_create_page() or grab_page() invoked via read path. In some cases fs has more than one page grabbed via its internal path where data to be read is already placed, so it may want just to add those pages into mm lru.

I understood why it does need that in pohmelfs.
AFAIU, other file system using general functions(ex, mpage_readpages or read_cache_pages) don't need direct LRU handling since it's hided. But pohmelfs doesn't use general functions.

Isn't pagevec_lru_add_file enough like other file system(ex, nfs, cifs)?

This will force to reinvent add_to_page_cache_lru() by doing private function which will call add_to_page_cache() and pagevec_lru_add_file(), which is effectively what is being done for file backed pages in add_to_page_cache_lru().

--
       Evgeniy Polyakov

Quoted 1 line

Until now, other file system don't need it.
Why do you need?

Quoted 3 lines · Minchan Kim

Quoted 1 line

Until now, other file system don't need it.
Why do you need?

To avoid deadlocks.  You tell logfs to write out some locked page, logfs determines that it needs to run garbage collection first.  Garbage collection can read any page.  If it called find_or_create_page() for the locked page, you have a deadlock.

Quoted 5 lines · JA?rn Engel

Quoted 3 lines · Minchan Kim

Quoted 1 line

Until now, other file system don't need it.
Why do you need?

Quoted 4 lines

To avoid deadlocks. A You tell logfs to write out some locked page, logfs determines that it needs to run garbage collection first. A Garbage collection can read any page. A If it called find_or_create_page() for the locked page, you have a deadlock.

Could you do it with add_to_page_cache and pagevec_lru_add_file?

Quoted 6 lines · Minchan Kim

Quoted 5 lines · Jörn Engel

Quoted 3 lines · Minchan Kim

Quoted 1 line

Until now, other file system don't need it.
Why do you need?

Quoted 6 lines

Quoted 4 lines

To avoid deadlocks.  You tell logfs to write out some locked page, logfs determines that it needs to run garbage collection first.  Garbage collection can read any page.  If it called find_or_create_page() for the locked page, you have a deadlock.

Could you do it with add_to_page_cache and pagevec_lru_add_file?

Maybe.  But how would that be an improvement?

As I see it, logfs needs a variant of find_or_create_page() that does not block on any pages waiting for logfs GC.  Currently that variant lives under fs/logfs/ and uses add_to_page_cache_lru().  If there are valid reasons against exporting add_to_page_cache_lru(), the right solution is to move the logfs variant to mm/, not to rewrite it.

If you want to change the implementation from using add_to_page_cache_lru() to using add_to_page_cache() and pagevec_lru_add_file(), then you should have a better reason than not exporting add_to_page_cache_lru().  If the new implementation was any better, I would gladly take it.

Quoted 22 lines · Jörn Engel

Quoted 6 lines · Minchan Kim

Quoted 5 lines · Jörn Engel

Quoted 3 lines · Minchan Kim

Quoted 1 line

Until now, other file system don't need it.
Why do you need?

Quoted 6 lines

Quoted 4 lines

To avoid deadlocks.  You tell logfs to write out some locked page, logfs determines that it needs to run garbage collection first.  Garbage collection can read any page.  If it called find_or_create_page() for the locked page, you have a deadlock.

Could you do it with add_to_page_cache and pagevec_lru_add_file?

Maybe.  But how would that be an improvement?

As I see it, logfs needs a variant of find_or_create_page() that does not block on any pages waiting for logfs GC.  Currently that variant lives under fs/logfs/ and uses add_to_page_cache_lru().  If there are valid reasons against exporting add_to_page_cache_lru(), the right solution is to move the logfs variant to mm/, not to rewrite it.

Quoted 5 lines

If you want to change the implementation from using add_to_page_cache_lru() to using add_to_page_cache() and pagevec_lru_add_file(), then you should have a better reason than not exporting add_to_page_cache_lru().  If the new implementation was any better, I would gladly take it.

Previously I said, what I have a concern is that if file systems or some modules abuses
add_to_page_cache_lru, it might system LRU list wrong so then system go to hell.
Of course, if we use it carefully, it can be good but how do you make sure it?

I am not a file system expert but as I read comment of read_cache_pages "Hides the details of the LRU cache etc from the filesystem", I thought it is not good that
file system handle LRU list directly. At least, we have been trying for years.

If we can do it with current functions without big cost, I think it's rather good than exporting
new function. Until 18bc0bbd162e3, we didn't export that but all file systems works well.
In addition, when the patch is merged, any mm guys seem to be not reviewed it, too.

I hope just ring at the bell to remain record to justify why we need exporting new function
although we can do it with existing functions.

If any other mm guys don't oppose it, I would be not against that, either.

--
Kind regards,
Minchan Kim

Previously I said, what I have a concern is that if file systems or some modules abuses
add_to_page_cache_lru, it might system LRU list wrong so then system go to hell.
Of course, if we use it carefully, it can be good but how do you make sure it?

I am not a file system expert but as I read comment of read_cache_pages "Hides the details of the LRU cache etc from the filesystem", I thought it is not good that
file system handle LRU list directly. At least, we have been trying for years.

Quoted 6 lines · Minchan Kim

Previously I said, what I have a concern is that if file systems or some modules abuses
add_to_page_cache_lru, it might system LRU list wrong so then system go to hell.
Of course, if we use it carefully, it can be good but how do you make sure it?

Having access to the source code means you only have to read all callers.  This is not java, we don't have to add layers of anti-abuse wrappers.  We can simply flame the first offender to a crisp. :)

Quoted 4 lines

I am not a file system expert but as I read comment of read_cache_pages "Hides the details of the LRU cache etc from the filesystem", I thought it is not good that
file system handle LRU list directly. At least, we have been trying for years.

Only speaking for logfs, I need some variant of find_or_create_page where I can replace lock_page() with a custom function.  Whether that function lives in fs/logfs/ or mm/filemap.c doesn't matter much.

What we could do something roughly like the patch below, at least semantically.  I know the patch is crap in its current form, but it illustrates the general idea.

Jörn

--
The key to performance is elegance, not battalions of special cases. -- Jon Bentley and Doug McIlroy

mm/filemap.c +3 -11

index 045b31c..6d452eb 100644
@@ -646,27 +646,19 @@ repeat:
  }
  EXPORT_SYMBOL(find_get_page);
 
-/**
- * find_lock_page - locate, pin and lock a pagecache page
- * @mapping: the address_space to search
- * @offset: the page index
- *
- * Locates the desired pagecache page, locks it, increments its reference
- * count and returns its address.
- *
- * Returns zero if the page was not present. find_lock_page() may sleep.
- */
-struct page *find_lock_page(struct address_space *mapping, pgoff_t offset)
+static struct page *__find_lock_page(struct address_space *mapping,
+               pgoff_t offset, void(*lock)(struct page *),
+               void(*unlock)(struct page *))
  {
        struct page *page;
 

 repeat:
       page = find_get_page(mapping, offset); if (page) {
-               lock_page(page);
+               lock(page);
               /* Has the page been truncated? */ if (unlikely(page->mapping != mapping)) {
-                       unlock_page(page);
+                       unlock(page); page_cache_release(page); goto repeat;
               }

mm/filemap.c +15 -16

@@ -674,32 +666,31 @@ repeat:
        }
        return page;
  }
-EXPORT_SYMBOL(find_lock_page);
 
  /**
- * find_or_create_page - locate or add a pagecache page
- * @mapping: the page's address_space
- * @index: the page's index into the mapping
- * @gfp_mask: page allocation mode
- *
- * Locates a page in the pagecache.  If the page is not present, a new page
- * is allocated using @gfp_mask and is added to the pagecache and to the VM's
- * LRU list.  The returned page is locked and has its reference count
- * incremented.
+ * find_lock_page - locate, pin and lock a pagecache page
+ * @mapping: the address_space to search
+ * @offset: the page index
  *
- * find_or_create_page() may sleep, even if @gfp_flags specifies an atomic
- * allocation!
+ * Locates the desired pagecache page, locks it, increments its reference
+ * count and returns its address.
  *
- * find_or_create_page() returns the desired page's address, or zero on
- * memory exhaustion.
+ * Returns zero if the page was not present. find_lock_page() may sleep.
  */
-struct page *find_or_create_page(struct address_space *mapping,
-               pgoff_t index, gfp_t gfp_mask)
+struct page *find_lock_page(struct address_space *mapping, pgoff_t offset)
+{
+       return __find_lock_page(mapping, offset, lock_page, unlock_page);
+}
+EXPORT_SYMBOL(find_lock_page);
+
+static struct page *__find_or_create_page(struct address_space *mapping,
+               pgoff_t index, gfp_t gfp_mask, void(*lock)(struct page *),
+               void(*unlock)(struct page *))
  {
        struct page *page;
        int err;

 repeat:
-       page = find_lock_page(mapping, index);
+       page = __find_lock_page(mapping, index, lock, unlock); if (!page) {
               page = __page_cache_alloc(gfp_mask); if (!page)

mm/filemap.c +18 -0

@@ -721,6 +712,31 @@ repeat:
        }
        return page;
  }
+EXPORT_SYMBOL(__find_or_create_page);
+
+/**
+ * find_or_create_page - locate or add a pagecache page
+ * @mapping: the page's address_space
+ * @index: the page's index into the mapping
+ * @gfp_mask: page allocation mode
+ *
+ * Locates a page in the pagecache.  If the page is not present, a new page
+ * is allocated using @gfp_mask and is added to the pagecache and to the VM's
+ * LRU list.  The returned page is locked and has its reference count
+ * incremented.
+ *
+ * find_or_create_page() may sleep, even if @gfp_flags specifies an atomic
+ * allocation!
+ *
+ * find_or_create_page() returns the desired page's address, or zero on
+ * memory exhaustion.

Seem to be not bad idea. :)
But we have to justify new interface before. For doing it, we have to say why we can't do it by current functions(find_get_page, add_to_page_cache and pagevec_lru_add_xxx)

Pagevec_lru_add_xxx does batch so that it can reduce calling path and some overhead(ex, page_is_file_cache comparison, get/put_cpu_var(lru_add_pvecs)).

At least, it would be rather good than old for performance.