PATCH: rewrite of invalidate_inode_pages

PATCH: rewrite of invalidate_inode_pages

[Juan J. Quintela]

Hi
        this patch does a rewrite of invalidate_inode_pages, the
improvements over the actual are:

- we don't do busy waiting (original version did)
- we define a new macro __lru_cache_del (old lru_cache_del without
  doing the locking)
- we define the function __remove_inode_page() (old remove_inode_page
  without sanity checks) they are done for the caller.
- we take the two locks (pagecache_lock and pagemap_lru_lock),  at the 
  begining and we do all the operations without more locking.
- we change one page_cache_release to put_page in truncate_inode_pages
  (people find lost when they see a get_page without the correspondent
  put_page, and put_page and page_cache_release are synonimops)
- It removes a small window for races in truncate_inode_pages for
  calling get_page after droping the spinlock.
- The number of ITERATIONS before droping the locks is to limit
  latency (it could be better other number).

This patch was discussed/made between Dave Jones, Rik van Riel, Arjan
van de Ven and me in the IRC channel #kernelnewbies (server
irc.openprojects.net). 


Comments anyone?  (the nfs/smb people are the ones that call that
function, comments form them are very apreciated).


Later, Juan.

diff -u -urN --exclude-from=exclude pre7-9/include/linux/swap.h remove_inode/include/linux/swap.h
--- pre7-9/include/linux/swap.h	Thu May 11 02:24:03 2000
+++ remove_inode/include/linux/swap.h	Thu May 11 18:00:27 2000
@@ -171,13 +171,18 @@
 	spin_unlock(&pagemap_lru_lock);		\
 } while (0)
 
+#define	__lru_cache_del(page)			\
+do {						\
+	list_del(&(page)->lru);			\
+	nr_lru_pages--;				\
+} while (0)
+
 #define	lru_cache_del(page)			\
 do {						\
 	if (!PageLocked(page))			\
 		BUG();				\
 	spin_lock(&pagemap_lru_lock);		\
-	list_del(&(page)->lru);			\
-	nr_lru_pages--;				\
+	__lru_cache_del(page);			\
 	spin_unlock(&pagemap_lru_lock);		\
 } while (0)
 
diff -u -urN --exclude-from=exclude pre7-9/mm/filemap.c remove_inode/mm/filemap.c
--- pre7-9/mm/filemap.c	Thu May 11 02:24:03 2000
+++ remove_inode/mm/filemap.c	Thu May 11 20:13:24 2000
@@ -67,7 +67,7 @@
 		PAGE_BUG(page);
 }
 
-static void remove_page_from_hash_queue(struct page * page)
+static inline void remove_page_from_hash_queue(struct page * page)
 {
 	if(page->pprev_hash) {
 		if(page->next_hash)
@@ -92,44 +92,71 @@
  * sure the page is locked and that nobody else uses it - or that usage
  * is safe.
  */
+static inline void __remove_inode_page(struct page *page)
+{
+	remove_page_from_inode_queue(page);
+	remove_page_from_hash_queue(page);
+	page->mapping = NULL;
+}
+
 void remove_inode_page(struct page *page)
 {
 	if (!PageLocked(page))
 		PAGE_BUG(page);
 
 	spin_lock(&pagecache_lock);
-	remove_page_from_inode_queue(page);
-	remove_page_from_hash_queue(page);
-	page->mapping = NULL;
+        __remove_inode_page(page);
 	spin_unlock(&pagecache_lock);
 }
 
+#define ITERATIONS 100
+
 void invalidate_inode_pages(struct inode * inode)
 {
 	struct list_head *head, *curr;
 	struct page * page;
+        int count;
 
- repeat:
 	head = &inode->i_mapping->pages;
-	spin_lock(&pagecache_lock);
-	curr = head->next;
-
-	while (curr != head) {
-		page = list_entry(curr, struct page, list);
-		curr = curr->next;
 
-		/* We cannot invalidate a locked page */
-		if (TryLockPage(page))
-			continue;
-		spin_unlock(&pagecache_lock);
-
-		lru_cache_del(page);
-		remove_inode_page(page);
-		UnlockPage(page);
-		page_cache_release(page);
-		goto repeat;
-	}
-	spin_unlock(&pagecache_lock);
+        while (head != head->next) {
+                spin_lock(&pagecache_lock);
+                spin_lock(&pagemap_lru_lock);
+                head = &inode->i_mapping->pages;
+                curr = head->next;
+                count = 0;
+
+                while ((curr != head) && (count++ < ITERATIONS)) {
+                        page = list_entry(curr, struct page, list);
+                        curr = curr->next;
+
+                        /* We cannot invalidate a locked page */
+                        if (TryLockPage(page))
+                                continue;
+
+                        __lru_cache_del(page);
+                        __remove_inode_page(page);
+                        UnlockPage(page);
+                        page_cache_release(page);
+                }
+
+                /* At this stage we have passed through the list
+                 * once, and there may still be locked pages. */
+
+                if (head->next!=head) {
+                        page = list_entry(head->next, struct page, list);
+                        get_page(page);
+                        spin_unlock(&pagemap_lru_lock);
+                        spin_unlock(&pagecache_lock);
+                        /* We need to block */
+                        lock_page(page);
+                        UnlockPage(page);
+                        put_page(page);
+                } else {                                         
+                        spin_unlock(&pagemap_lru_lock);
+                        spin_unlock(&pagecache_lock);
+                }
+        }
 }
 
 /*
@@ -160,8 +187,8 @@
 		/* page wholly truncated - free it */
 		if (offset >= start) {
 			if (TryLockPage(page)) {
-				spin_unlock(&pagecache_lock);
 				get_page(page);
+				spin_unlock(&pagecache_lock);
 				wait_on_page(page);
 				put_page(page);
 				goto repeat;
@@ -184,7 +211,7 @@
 
 			UnlockPage(page);
 			page_cache_release(page);
-			page_cache_release(page);
+			put_page(page);
 
 			/*
 			 * We have done things without the pagecache lock,
@@ -323,9 +350,7 @@
 		/* is it a page-cache page? */
 		if (page->mapping) {
 			if (!PageDirty(page) && !pgcache_under_min()) {
-				remove_page_from_inode_queue(page);
-				remove_page_from_hash_queue(page);
-				page->mapping = NULL;
+                                __remove_inode_page(page);
 				spin_unlock(&pagecache_lock);
 				goto made_inode_progress;
 			}

-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Linus Torvalds]

On 11 May 2000, Juan J. Quintela wrote:
> - we change one page_cache_release to put_page in truncate_inode_pages
>   (people find lost when they see a get_page without the correspondent
>   put_page, and put_page and page_cache_release are synonimops)

put_page() is _not_ synonymous with page_cache_release()!

Imagine a time in the not too distant future when the page cache
granularity is 8kB or 16kB due to better IO performance (possibly
controlled by a config option), and page_cache_release() will do an
"order=1" or "order=2" page free..

		Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Juan J. Quintela]

>>>>> "linus" == Linus Torvalds <torvalds@transmeta.com> writes:

linus> On 11 May 2000, Juan J. Quintela wrote:
>> - we change one page_cache_release to put_page in truncate_inode_pages
>> (people find lost when they see a get_page without the correspondent
>> put_page, and put_page and page_cache_release are synonimops)

linus> put_page() is _not_ synonymous with page_cache_release()!

linus> Imagine a time in the not too distant future when the page cache
linus> granularity is 8kB or 16kB due to better IO performance (possibly
linus> controlled by a config option), and page_cache_release() will do an
linus> "order=1" or "order=2" page free..

Linus, I agree with you here, but we do a get_page 5 lines before, I
think that if I do a get_page I should do a put_page to liberate it. 
But I can be wrong, and then I would like to know if in the future, it
could be posible to do a get_page and liberate it with a
page_cache_release?  That was my point.  Sorry for the bad wording.

Later, Juan.

PD. As always, I apreciate a lot your comments.

-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Linus Torvalds]

On 11 May 2000, Juan J. Quintela wrote:
> 
> Linus, I agree with you here, but we do a get_page 5 lines before, I
> think that if I do a get_page I should do a put_page to liberate it. 

No, "get_page()" really means "increment the usage count by one", and the
problem is that it is obviously completely neutral wrt the actual size of
the page.

What we _could_ do is to just for clarity have

	#define page_cache_get()	get_page()

and then pair up every "page_cache_get()" with "page_cache_release()".
Which makes sense to me. So if you feel strongly about this issue..

		Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Juan J. Quintela]

>>>>> "linus" == Linus Torvalds <torvalds@transmeta.com> writes:

Hi

linus> What we _could_ do is to just for clarity have

linus> 	#define page_cache_get()	get_page()

linus> and then pair up every "page_cache_get()" with "page_cache_release()".
linus> Which makes sense to me. So if you feel strongly about this issue..

You ask for it, here is the patch. Noted that I have changed all the
get_page/put_page/__free_page that I have find to the equivalents in
the page_cache_get/page_cache_release/page_cache_release.

There are two points where I am not sure about the thing to do:

- In shm.c it calls alloc_pages, I have substituted it for page_cache,
  due to the fact that shm use the page_cache, if somebody changes
  the page_cache, it needs to change the shm code acordingly.

- In buffers.c it calls alloc_page, but it calls it with a different
  mask, then I have left the alloc_pages, call. But I have put the
  get/put operations as page_cache_* operations, due to the fact that
  they use the page_cache.

Once that we are here, what are the *semantic* difference between
page_cache_release and page_cache_free?

Any comment?

Later, Juan.

diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/fs/buffer.c testing2/fs/buffer.c
--- pre7/fs/buffer.c	Fri May 12 01:11:40 2000
+++ testing2/fs/buffer.c	Fri May 12 02:44:30 2000
@@ -1264,7 +1264,7 @@
 		set_bit(BH_Mapped, &bh->b_state);
 	}
 	tail->b_this_page = head;
-	get_page(page);
+	page_cache_get(page);
 	page->buffers = head;
 	return 0;
 }
@@ -1351,7 +1351,7 @@
 	} while (bh);
 	tail->b_this_page = head;
 	page->buffers = head;
-	get_page(page);
+	page_cache_get(page);
 }
 
 static void unmap_underlying_metadata(struct buffer_head * bh)
@@ -2106,7 +2106,7 @@
 	return 1;
 
 no_buffer_head:
-	__free_page(page);
+	page_cache_release(page);
 out:
 	return 0;
 }
@@ -2190,7 +2190,7 @@
 
 	/* And free the page */
 	page->buffers = NULL;
-	__free_page(page);
+	page_cache_release(page);
 	spin_unlock(&free_list[index].lock);
 	write_unlock(&hash_table_lock);
 	spin_unlock(&lru_list_lock);
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/fs/nfs/write.c testing2/fs/nfs/write.c
--- pre7/fs/nfs/write.c	Fri May 12 01:11:41 2000
+++ testing2/fs/nfs/write.c	Fri May 12 01:56:29 2000
@@ -528,7 +528,7 @@
 	 * long write-back delay. This will be adjusted in
 	 * update_nfs_request below if the region is not locked. */
 	req->wb_page    = page;
-	get_page(page);
+	page_cache_get(page);
 	req->wb_offset  = offset;
 	req->wb_bytes   = count;
 	req->wb_dentry  = dget(dentry);
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/include/linux/pagemap.h testing2/include/linux/pagemap.h
--- pre7/include/linux/pagemap.h	Fri May 12 01:11:42 2000
+++ testing2/include/linux/pagemap.h	Fri May 12 01:45:46 2000
@@ -28,6 +28,7 @@
 #define PAGE_CACHE_MASK		PAGE_MASK
 #define PAGE_CACHE_ALIGN(addr)	(((addr)+PAGE_CACHE_SIZE-1)&PAGE_CACHE_MASK)
 
+#define page_cache_get(x)	get_page(x);
 #define page_cache_alloc()	alloc_pages(GFP_HIGHUSER, 0)
 #define page_cache_free(x)	__free_page(x)
 #define page_cache_release(x)	__free_page(x)
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/ipc/shm.c testing2/ipc/shm.c
--- pre7/ipc/shm.c	Fri May 12 01:11:43 2000
+++ testing2/ipc/shm.c	Fri May 12 02:35:59 2000
@@ -1348,7 +1348,7 @@
 		   could potentially fault on our pte under us */
 		if (pte_none(pte)) {
 			shm_unlock(shp->id);
-			page = alloc_page(GFP_HIGHUSER);
+			page = page_cache_alloc();
 			if (!page)
 				goto oom;
 			clear_user_highpage(page, address);
@@ -1380,7 +1380,7 @@
 	}
 
 	/* pte_val(pte) == SHM_ENTRY (shp, idx) */
-	get_page(pte_page(pte));
+	page_cache_get(pte_page(pte));
 	return pte_page(pte);
 
 oom:
@@ -1448,7 +1448,7 @@
 	lock_kernel();
 	rw_swap_page(WRITE, page, 0);
 	unlock_kernel();
-	__free_page(page);
+	page_cache_release(page);
 }
 
 static int shm_swap_preop(swp_entry_t *swap_entry)
@@ -1537,7 +1537,7 @@
 
 	pte = pte_mkdirty(mk_pte(page, PAGE_SHARED));
 	SHM_ENTRY(shp, idx) = pte;
-	get_page(page);
+	page_cache_get(page);
 	shm_rss++;
 
 	shm_swp--;
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/mm/filemap.c testing2/mm/filemap.c
--- pre7/mm/filemap.c	Fri May 12 01:11:43 2000
+++ testing2/mm/filemap.c	Fri May 12 02:00:07 2000
@@ -145,7 +145,7 @@
 
 		if (head->next!=head) {
 			page = list_entry(head->next, struct page, list);
-			get_page(page);
+			page_cache_get(page);
 			spin_unlock(&pagemap_lru_lock);
 			spin_unlock(&pagecache_lock);
 			/* We need to block */
@@ -187,13 +187,13 @@
 		/* page wholly truncated - free it */
 		if (offset >= start) {
 			if (TryLockPage(page)) {
-				get_page(page);
+				page_cache_get(page);
 				spin_unlock(&pagecache_lock);
 				wait_on_page(page);
 				page_cache_release(page);
 				goto repeat;
 			}
-			get_page(page);
+			page_cache_get(page);
 			spin_unlock(&pagecache_lock);
 
 			if (!page->buffers || block_flushpage(page, 0))
@@ -237,7 +237,7 @@
 			spin_unlock(&pagecache_lock);
 			goto repeat;
 		}
-		get_page(page);
+		page_cache_get(page);
 		spin_unlock(&pagecache_lock);
 
 		memclear_highpage_flush(page, partial, PAGE_CACHE_SIZE-partial);
@@ -252,9 +252,9 @@
 		 */
 		UnlockPage(page);
 		page_cache_release(page);
-		get_page(page);
+		page_page_get(page);
 		wait_on_page(page);
-		put_page(page);
+		page_cache_release(page);
 		goto repeat;
 	}
 	spin_unlock(&pagecache_lock);
@@ -312,7 +312,7 @@
 		spin_unlock(&pagemap_lru_lock);
 
 		/* avoid freeing the page while it's locked */
-		get_page(page);
+		page_cache_get(page);
 
 		/*
 		 * Is it a buffer page? Try to clean it up regardless
@@ -376,7 +376,7 @@
 unlock_continue:
 		spin_lock(&pagemap_lru_lock);
 		UnlockPage(page);
-		put_page(page);
+		page_cache_release(page);
 dispose_continue:
 		list_add(page_lru, dispose);
 	}
@@ -386,7 +386,7 @@
 	page_cache_release(page);
 made_buffer_progress:
 	UnlockPage(page);
-	put_page(page);
+	page_cache_release(page);
 	ret = 1;
 	spin_lock(&pagemap_lru_lock);
 	/* nr_lru_pages needs the spinlock */
@@ -474,7 +474,7 @@
 		if (page->index < start)
 			continue;
 
-		get_page(page);
+		page_cache_get(page);
 		spin_unlock(&pagecache_lock);
 		lock_page(page);
 
@@ -516,7 +516,7 @@
 	if (!PageLocked(page))
 		BUG();
 
-	get_page(page);
+	page_cache_get(page);
 	spin_lock(&pagecache_lock);
 	page->index = index;
 	add_page_to_inode_queue(mapping, page);
@@ -541,7 +541,7 @@
 
 	flags = page->flags & ~((1 << PG_uptodate) | (1 << PG_error) | (1 << PG_dirty));
 	page->flags = flags | (1 << PG_locked) | (1 << PG_referenced);
-	get_page(page);
+	page_cache_get(page);
 	page->index = offset;
 	add_page_to_inode_queue(mapping, page);
 	__add_page_to_hash_queue(page, hash);
@@ -683,7 +683,7 @@
 	spin_lock(&pagecache_lock);
 	page = __find_page_nolock(mapping, offset, *hash);
 	if (page)
-		get_page(page);
+		page_cache_get(page);
 	spin_unlock(&pagecache_lock);
 
 	/* Found the page, sleep if locked. */
@@ -733,7 +733,7 @@
 	spin_lock(&pagecache_lock);
 	page = __find_page_nolock(mapping, offset, *hash);
 	if (page)
-		get_page(page);
+		page_cache_get(page);
 	spin_unlock(&pagecache_lock);
 
 	/* Found the page, sleep if locked. */
@@ -1091,7 +1091,7 @@
 		if (!page)
 			goto no_cached_page;
 found_page:
-		get_page(page);
+		page_cache_get(page);
 		spin_unlock(&pagecache_lock);
 
 		if (!Page_Uptodate(page))
@@ -1594,7 +1594,7 @@
 		set_pte(ptep, pte_mkclean(pte));
 		flush_tlb_page(vma, address);
 		page = pte_page(pte);
-		get_page(page);
+		page_cache_get(page);
 	} else {
 		if (pte_none(pte))
 			return 0;
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/mm/memory.c testing2/mm/memory.c
--- pre7/mm/memory.c	Fri May 12 01:11:43 2000
+++ testing2/mm/memory.c	Fri May 12 02:30:44 2000
@@ -861,7 +861,7 @@
 	 * Ok, we need to copy. Oh, well..
 	 */
 	spin_unlock(&mm->page_table_lock);
-	new_page = alloc_page(GFP_HIGHUSER);
+	new_page = page_cache_alloc();
 	if (!new_page)
 		return -1;
 	spin_lock(&mm->page_table_lock);
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/mm/swap_state.c testing2/mm/swap_state.c
--- pre7/mm/swap_state.c	Fri May 12 01:11:43 2000
+++ testing2/mm/swap_state.c	Fri May 12 02:23:47 2000
@@ -136,7 +136,7 @@
 		}
 		UnlockPage(page);
 	}
-	__free_page(page);
+	put_page_release(page);
 }
 
 
@@ -172,7 +172,7 @@
 		 */
 		if (!PageSwapCache(found)) {
 			UnlockPage(found);
-			__free_page(found);
+			page_cache_release(found);
 			goto repeat;
 		}
 		if (found->mapping != &swapper_space)
@@ -187,7 +187,7 @@
 out_bad:
 	printk (KERN_ERR "VM: Found a non-swapper swap page!\n");
 	UnlockPage(found);
-	__free_page(found);
+	page_cache_release(found);
 	return 0;
 }
 
@@ -237,7 +237,7 @@
 	return new_page;
 
 out_free_page:
-	__free_page(new_page);
+	page_cache_release(new_page);
 out_free_swap:
 	swap_free(entry);
 out:





-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

PATCH: new page_cache_get() (try 2)

[Juan J. Quintela]

>>>>> "juan" == Juan J Quintela <quintela@fi.udc.es> writes:

Hi again

juan> You ask for it, here is the patch. Noted that I have changed all the
juan> get_page/put_page/__free_page that I have find to the equivalents in
juan> the page_cache_get/page_cache_release/page_cache_release.

juan> There are two points where I am not sure about the thing to do:

juan> - In shm.c it calls alloc_pages, I have substituted it for page_cache,
juan>   due to the fact that shm use the page_cache, if somebody changes
juan>   the page_cache, it needs to change the shm code acordingly.

juan> - In buffers.c it calls alloc_page, but it calls it with a different
juan>   mask, then I have left the alloc_pages, call. But I have put the
juan>   get/put operations as page_cache_* operations, due to the fact that
juan>   they use the page_cache.

juan> Once that we are here, what are the *semantic* difference between
juan> page_cache_release and page_cache_free?

I have done 2 errors in previous patch.  In this patch I had fixed
that 2 errors, and I have included the new invalidate_inode_pages
function that likes trond.  I have also removed the end of the loop in
truncate_inode_pages to show that it doesn't make sense (at least to
me).  We do an:
      UnlockPage();
      page_cache_release();
      page_cache_get();
      wait_on_page();       // here we will return fast almost sure
                            // this functions is inlined and we have 
                            // just dropped the lock
      page_cache_release();
      goto repeat;

and I have changed that to:

    UnlockPage();
    page_cache_release();
    goto repeat;

If somebody knows a reason to have the previous code, I am very
interested in knowing it.

Later, Juan.

PD. Juan will learn not to make changes and don't recheck...
    Juan will learn not to make changes and don't recheck...
    .....

diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/fs/buffer.c testing2/fs/buffer.c
--- pre7/fs/buffer.c	Fri May 12 01:11:40 2000
+++ testing2/fs/buffer.c	Fri May 12 02:49:23 2000
@@ -1264,7 +1264,7 @@
 		set_bit(BH_Mapped, &bh->b_state);
 	}
 	tail->b_this_page = head;
-	get_page(page);
+	page_cache_get(page);
 	page->buffers = head;
 	return 0;
 }
@@ -1351,7 +1351,7 @@
 	} while (bh);
 	tail->b_this_page = head;
 	page->buffers = head;
-	get_page(page);
+	page_cache_get(page);
 }
 
 static void unmap_underlying_metadata(struct buffer_head * bh)
@@ -2106,7 +2106,7 @@
 	return 1;
 
 no_buffer_head:
-	__free_page(page);
+	page_cache_release(page);
 out:
 	return 0;
 }
@@ -2190,7 +2190,7 @@
 
 	/* And free the page */
 	page->buffers = NULL;
-	__free_page(page);
+	page_cache_release(page);
 	spin_unlock(&free_list[index].lock);
 	write_unlock(&hash_table_lock);
 	spin_unlock(&lru_list_lock);
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/fs/nfs/write.c testing2/fs/nfs/write.c
--- pre7/fs/nfs/write.c	Fri May 12 01:11:41 2000
+++ testing2/fs/nfs/write.c	Fri May 12 01:56:29 2000
@@ -528,7 +528,7 @@
 	 * long write-back delay. This will be adjusted in
 	 * update_nfs_request below if the region is not locked. */
 	req->wb_page    = page;
-	get_page(page);
+	page_cache_get(page);
 	req->wb_offset  = offset;
 	req->wb_bytes   = count;
 	req->wb_dentry  = dget(dentry);
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/include/linux/pagemap.h testing2/include/linux/pagemap.h
--- pre7/include/linux/pagemap.h	Fri May 12 01:11:42 2000
+++ testing2/include/linux/pagemap.h	Fri May 12 03:44:11 2000
@@ -28,6 +28,7 @@
 #define PAGE_CACHE_MASK		PAGE_MASK
 #define PAGE_CACHE_ALIGN(addr)	(((addr)+PAGE_CACHE_SIZE-1)&PAGE_CACHE_MASK)
 
+#define page_cache_get(x)	get_page(x);
 #define page_cache_alloc()	alloc_pages(GFP_HIGHUSER, 0)
 #define page_cache_free(x)	__free_page(x)
 #define page_cache_release(x)	__free_page(x)
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/ipc/shm.c testing2/ipc/shm.c
--- pre7/ipc/shm.c	Fri May 12 01:11:43 2000
+++ testing2/ipc/shm.c	Fri May 12 02:35:59 2000
@@ -1348,7 +1348,7 @@
 		   could potentially fault on our pte under us */
 		if (pte_none(pte)) {
 			shm_unlock(shp->id);
-			page = alloc_page(GFP_HIGHUSER);
+			page = page_cache_alloc();
 			if (!page)
 				goto oom;
 			clear_user_highpage(page, address);
@@ -1380,7 +1380,7 @@
 	}
 
 	/* pte_val(pte) == SHM_ENTRY (shp, idx) */
-	get_page(pte_page(pte));
+	page_cache_get(pte_page(pte));
 	return pte_page(pte);
 
 oom:
@@ -1448,7 +1448,7 @@
 	lock_kernel();
 	rw_swap_page(WRITE, page, 0);
 	unlock_kernel();
-	__free_page(page);
+	page_cache_release(page);
 }
 
 static int shm_swap_preop(swp_entry_t *swap_entry)
@@ -1537,7 +1537,7 @@
 
 	pte = pte_mkdirty(mk_pte(page, PAGE_SHARED));
 	SHM_ENTRY(shp, idx) = pte;
-	get_page(page);
+	page_cache_get(page);
 	shm_rss++;
 
 	shm_swp--;
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/mm/filemap.c testing2/mm/filemap.c
--- pre7/mm/filemap.c	Fri May 12 01:11:43 2000
+++ testing2/mm/filemap.c	Fri May 12 03:42:22 2000
@@ -111,15 +111,23 @@
 
 #define ITERATIONS 100
 
+/**
+ * invalidate_inode_pages - Invalidate all the unlocked pages of one inode
+ * @inode: the inode which pages we want to invalidate
+ *
+ * This function only removes the unlocked pages, if you want to
+ * remove all the pages of one inode, you must call truncate_inode_pages.
+ */
+
 void invalidate_inode_pages(struct inode * inode)
 {
 	struct list_head *head, *curr;
 	struct page * page;
-	int count;
+	int count = ITERATIONS;
 
 	head = &inode->i_mapping->pages;
 
-	while (head != head->next) {
+	while (count == ITERATIONS) {
 		spin_lock(&pagecache_lock);
 		spin_lock(&pagemap_lru_lock);
 		head = &inode->i_mapping->pages;
@@ -140,22 +148,8 @@
 			page_cache_release(page);
 		}
 
-		/* At this stage we have passed through the list
-		 * once, and there may still be locked pages. */
-
-		if (head->next!=head) {
-			page = list_entry(head->next, struct page, list);
-			get_page(page);
-			spin_unlock(&pagemap_lru_lock);
-			spin_unlock(&pagecache_lock);
-			/* We need to block */
-			lock_page(page);
-			UnlockPage(page);
-			page_cache_release(page);
-		} else {                                         
-			spin_unlock(&pagemap_lru_lock);
-			spin_unlock(&pagecache_lock);
-		}
+		spin_unlock(&pagemap_lru_lock);
+		spin_unlock(&pagecache_lock);
 	}
 }
 
@@ -187,13 +181,13 @@
 		/* page wholly truncated - free it */
 		if (offset >= start) {
 			if (TryLockPage(page)) {
-				get_page(page);
+				page_cache_get(page);
 				spin_unlock(&pagecache_lock);
 				wait_on_page(page);
 				page_cache_release(page);
 				goto repeat;
 			}
-			get_page(page);
+			page_cache_get(page);
 			spin_unlock(&pagecache_lock);
 
 			if (!page->buffers || block_flushpage(page, 0))
@@ -237,7 +231,7 @@
 			spin_unlock(&pagecache_lock);
 			goto repeat;
 		}
-		get_page(page);
+		page_cache_get(page);
 		spin_unlock(&pagecache_lock);
 
 		memclear_highpage_flush(page, partial, PAGE_CACHE_SIZE-partial);
@@ -252,9 +246,6 @@
 		 */
 		UnlockPage(page);
 		page_cache_release(page);
-		get_page(page);
-		wait_on_page(page);
-		put_page(page);
 		goto repeat;
 	}
 	spin_unlock(&pagecache_lock);
@@ -312,7 +303,7 @@
 		spin_unlock(&pagemap_lru_lock);
 
 		/* avoid freeing the page while it's locked */
-		get_page(page);
+		page_cache_get(page);
 
 		/*
 		 * Is it a buffer page? Try to clean it up regardless
@@ -376,7 +367,7 @@
 unlock_continue:
 		spin_lock(&pagemap_lru_lock);
 		UnlockPage(page);
-		put_page(page);
+		page_cache_release(page);
 dispose_continue:
 		list_add(page_lru, dispose);
 	}
@@ -386,7 +377,7 @@
 	page_cache_release(page);
 made_buffer_progress:
 	UnlockPage(page);
-	put_page(page);
+	page_cache_release(page);
 	ret = 1;
 	spin_lock(&pagemap_lru_lock);
 	/* nr_lru_pages needs the spinlock */
@@ -474,7 +465,7 @@
 		if (page->index < start)
 			continue;
 
-		get_page(page);
+		page_cache_get(page);
 		spin_unlock(&pagecache_lock);
 		lock_page(page);
 
@@ -516,7 +507,7 @@
 	if (!PageLocked(page))
 		BUG();
 
-	get_page(page);
+	page_cache_get(page);
 	spin_lock(&pagecache_lock);
 	page->index = index;
 	add_page_to_inode_queue(mapping, page);
@@ -541,7 +532,7 @@
 
 	flags = page->flags & ~((1 << PG_uptodate) | (1 << PG_error) | (1 << PG_dirty));
 	page->flags = flags | (1 << PG_locked) | (1 << PG_referenced);
-	get_page(page);
+	page_cache_get(page);
 	page->index = offset;
 	add_page_to_inode_queue(mapping, page);
 	__add_page_to_hash_queue(page, hash);
@@ -683,7 +674,7 @@
 	spin_lock(&pagecache_lock);
 	page = __find_page_nolock(mapping, offset, *hash);
 	if (page)
-		get_page(page);
+		page_cache_get(page);
 	spin_unlock(&pagecache_lock);
 
 	/* Found the page, sleep if locked. */
@@ -733,7 +724,7 @@
 	spin_lock(&pagecache_lock);
 	page = __find_page_nolock(mapping, offset, *hash);
 	if (page)
-		get_page(page);
+		page_cache_get(page);
 	spin_unlock(&pagecache_lock);
 
 	/* Found the page, sleep if locked. */
@@ -1091,7 +1082,7 @@
 		if (!page)
 			goto no_cached_page;
 found_page:
-		get_page(page);
+		page_cache_get(page);
 		spin_unlock(&pagecache_lock);
 
 		if (!Page_Uptodate(page))
@@ -1594,7 +1585,7 @@
 		set_pte(ptep, pte_mkclean(pte));
 		flush_tlb_page(vma, address);
 		page = pte_page(pte);
-		get_page(page);
+		page_cache_get(page);
 	} else {
 		if (pte_none(pte))
 			return 0;
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/mm/memory.c testing2/mm/memory.c
--- pre7/mm/memory.c	Fri May 12 01:11:43 2000
+++ testing2/mm/memory.c	Fri May 12 02:30:44 2000
@@ -861,7 +861,7 @@
 	 * Ok, we need to copy. Oh, well..
 	 */
 	spin_unlock(&mm->page_table_lock);
-	new_page = alloc_page(GFP_HIGHUSER);
+	new_page = page_cache_alloc();
 	if (!new_page)
 		return -1;
 	spin_lock(&mm->page_table_lock);
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7/mm/swap_state.c testing2/mm/swap_state.c
--- pre7/mm/swap_state.c	Fri May 12 01:11:43 2000
+++ testing2/mm/swap_state.c	Fri May 12 03:13:09 2000
@@ -136,7 +136,7 @@
 		}
 		UnlockPage(page);
 	}
-	__free_page(page);
+        page_cache_release(page);
 }
 
 
@@ -172,7 +172,7 @@
 		 */
 		if (!PageSwapCache(found)) {
 			UnlockPage(found);
-			__free_page(found);
+			page_cache_release(found);
 			goto repeat;
 		}
 		if (found->mapping != &swapper_space)
@@ -187,7 +187,7 @@
 out_bad:
 	printk (KERN_ERR "VM: Found a non-swapper swap page!\n");
 	UnlockPage(found);
-	__free_page(found);
+	page_cache_release(found);
 	return 0;
 }
 
@@ -237,7 +237,7 @@
 	return new_page;
 
 out_free_page:
-	__free_page(new_page);
+	page_cache_release(new_page);
 out_free_swap:
 	swap_free(entry);
 out:



-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Ingo Molnar]

On 11 May 2000, Juan J. Quintela wrote:

> Linus, I agree with you here, but we do a get_page 5 lines before, I
> think that if I do a get_page I should do a put_page to liberate it. 

get_page() is different - it elevates the page count of a page of
_arbitrary order_. put_page() on the other hand does a __free_page()
[unconditional order 0]. This is a speciality of the Buddy allocator (you
can get a reference to a page through it's pointer without knowing the
order of the page, but you cannot free it without knowing the order), and
it's bad naming (i believe that particular naming is my fault).

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Trond Myklebust]

>>>>> " " == Juan J Quintela <quintela@fi.udc.es> writes:

     > Linus, I agree with you here, but we do a get_page 5 lines
     > before, I think that if I do a get_page I should do a put_page
     > to liberate it.  But I can be wrong, and then I would like to
     > know if in the future, it could be posible to do a get_page and
     > liberate it with a page_cache_release?  That was my point.
     > Sorry for the bad wording.

That part of the code is broken. We do not want to wait on locked
pages in invalidate_inode_pages(): that's the whole reason for its
existence. truncate_inode_pages() is the waiting version.

Cheers,
  Trond
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Juan J. Quintela]

>>>>> "trond" == Trond Myklebust <trond.myklebust@fys.uio.no> writes:

>>>>> " " == Juan J Quintela <quintela@fi.udc.es> writes:
>> Linus, I agree with you here, but we do a get_page 5 lines
>> before, I think that if I do a get_page I should do a put_page
>> to liberate it.  But I can be wrong, and then I would like to
>> know if in the future, it could be posible to do a get_page and
>> liberate it with a page_cache_release?  That was my point.
>> Sorry for the bad wording.

trond> That part of the code is broken. We do not want to wait on locked
trond> pages in invalidate_inode_pages(): that's the whole reason for its
trond> existence. truncate_inode_pages() is the waiting version.

Then you want only invalidate the non_locked pages: do you like:
What do you think of the following patch them?

Comments.

Later.


--- pre7-9/mm/filemap.c	Thu May 11 02:24:03 2000
+++ remove_inode/mm/filemap.c	Fri May 12 00:51:51 2000
@@ -67,7 +67,7 @@
 		PAGE_BUG(page);
 }
 
-static void remove_page_from_hash_queue(struct page * page)
+static inline void remove_page_from_hash_queue(struct page * page)
 {
 	if(page->pprev_hash) {
 		if(page->next_hash)
@@ -92,44 +92,55 @@
  * sure the page is locked and that nobody else uses it - or that usage
  * is safe.
  */
+static inline void __remove_inode_page(struct page *page)
+{
+	remove_page_from_inode_queue(page);
+	remove_page_from_hash_queue(page);
+	page->mapping = NULL;
+}
+
 void remove_inode_page(struct page *page)
 {
 	if (!PageLocked(page))
 		PAGE_BUG(page);
 
 	spin_lock(&pagecache_lock);
-	remove_page_from_inode_queue(page);
-	remove_page_from_hash_queue(page);
-	page->mapping = NULL;
+        __remove_inode_page(page);
 	spin_unlock(&pagecache_lock);
 }
 
+#define ITERATIONS 100
+
 void invalidate_inode_pages(struct inode * inode)
 {
 	struct list_head *head, *curr;
 	struct page * page;
+        int count = ITERATIONS;
 
- repeat:
-	head = &inode->i_mapping->pages;
-	spin_lock(&pagecache_lock);
-	curr = head->next;
-
-	while (curr != head) {
-		page = list_entry(curr, struct page, list);
-		curr = curr->next;
-
-		/* We cannot invalidate a locked page */
-		if (TryLockPage(page))
-			continue;
-		spin_unlock(&pagecache_lock);
-
-		lru_cache_del(page);
-		remove_inode_page(page);
-		UnlockPage(page);
-		page_cache_release(page);
-		goto repeat;
-	}
-	spin_unlock(&pagecache_lock);
+        while (count == ITERATIONS) {
+                spin_lock(&pagecache_lock);
+                spin_lock(&pagemap_lru_lock);
+                head = &inode->i_mapping->pages;
+                curr = head->next;
+                count = 0;
+
+                while ((curr != head) && (count++ < ITERATIONS)) {
+                        page = list_entry(curr, struct page, list);
+                        curr = curr->next;
+
+                        /* We cannot invalidate a locked page */
+                        if (TryLockPage(page))
+                                continue;
+
+                        __lru_cache_del(page);
+                        __remove_inode_page(page);
+                        UnlockPage(page);
+                        page_cache_release(page);
+                }
+
+                spin_unlock(&pagemap_lru_lock);
+                spin_unlock(&pagecache_lock);
+        }
 }
 
 /*
@@ -160,8 +171,8 @@
 		/* page wholly truncated - free it */
 		if (offset >= start) {
 			if (TryLockPage(page)) {
-				spin_unlock(&pagecache_lock);
 				get_page(page);
+				spin_unlock(&pagecache_lock);
 				wait_on_page(page);
 				put_page(page);
 				goto repeat;
@@ -323,9 +334,7 @@
 		/* is it a page-cache page? */
 		if (page->mapping) {
 			if (!PageDirty(page) && !pgcache_under_min()) {
-				remove_page_from_inode_queue(page);
-				remove_page_from_hash_queue(page);
-				page->mapping = NULL;
+                                __remove_inode_page(page);
 				spin_unlock(&pagecache_lock);
 				goto made_inode_progress;
 			}
--- pre7-9/include/linux/swap.h	Thu May 11 02:24:03 2000
+++ remove_inode/include/linux/swap.h	Thu May 11 18:00:27 2000
@@ -171,13 +171,18 @@
 	spin_unlock(&pagemap_lru_lock);		\
 } while (0)
 
+#define	__lru_cache_del(page)			\
+do {						\
+	list_del(&(page)->lru);			\
+	nr_lru_pages--;				\
+} while (0)
+
 #define	lru_cache_del(page)			\
 do {						\
 	if (!PageLocked(page))			\
 		BUG();				\
 	spin_lock(&pagemap_lru_lock);		\
-	list_del(&(page)->lru);			\
-	nr_lru_pages--;				\
+	__lru_cache_del(page);			\
 	spin_unlock(&pagemap_lru_lock);		\
 } while (0)
 



-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Trond Myklebust]

>>>>> " " == Juan J Quintela <quintela@fi.udc.es> writes:

     > Then you want only invalidate the non_locked pages: do you

That's right. This patch looks much more appropriate.

     > + while (count == ITERATIONS) {
     > + spin_lock(&pagecache_lock);
     > + spin_lock(&pagemap_lru_lock);
     > + head = &inode->i_mapping->pages;
     > + curr = head->next;
     > + count = 0;
     > +
     > + while ((curr != head) && (count++ < ITERATIONS)) {

Just one question: Isn't it better to do it all in 1 iteration through
the loop rather than doing it in batches of 100 pages?
You can argue that you're freeing up the spinlocks for the duration of
the loop_and_test, but is that really going to make a huge difference
to SMP performance?

Cheers,
  Trond
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Juan J. Quintela]

>>>>> "trond" == Trond Myklebust <trond.myklebust@fys.uio.no> writes:

>>>>> " " == Juan J Quintela <quintela@fi.udc.es> writes:
>> Then you want only invalidate the non_locked pages: do you

trond> That's right. This patch looks much more appropriate.

>> + while (count == ITERATIONS) {
>> + spin_lock(&pagecache_lock);
>> + spin_lock(&pagemap_lru_lock);
>> + head = &inode->i_mapping->pages;
>> + curr = head->next;
>> + count = 0;
>> +
>> + while ((curr != head) && (count++ < ITERATIONS)) {

trond> Just one question: Isn't it better to do it all in 1 iteration through
trond> the loop rather than doing it in batches of 100 pages?
trond> You can argue that you're freeing up the spinlocks for the duration of
trond> the loop_and_test, but is that really going to make a huge difference
trond> to SMP performance?

Trond, I have not an SMP machine (yet), and I can not tell you numbers
now.  I put the counter there to show that we *may* want to limit the
latency there.  I am thinking in the write of a big file, that can
take a lot to free all the pages, but I don't know, *you* are the NFS
expert, this was one of the reasons that we want feedback from the
users of the call.  (You have been very good giving comments).

My idea to put a limit is to put a limit than normally you do all in
one iteration, but in the exceptional case of a big amount of pages,
the latency is limited.  There is a limit in the number of pages that
can be in that list?

100 is one number that can need tuning, I don't know.  SMP experts
anywhere?

By the way, while we are here, the only difference between
truncate_inode_pages and invalidate_inode_pages is the one that you
told here before?  I am documenting some of the MM stuff, and your
comments in that aspect are really wellcome.  (You will have noted
now that I am quite newbie here).

Later, Juan.


-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Trond Myklebust]

>>>>> " " == Juan J Quintela <quintela@fi.udc.es> writes:

     > Trond, I have not an SMP machine (yet), and I can not tell you
     > numbers now.  I put the counter there to show that we *may*
     > want to limit the latency there.  I am thinking in the write of
     > a big file, that can take a lot to free all the pages, but I

I'm pretty SMP-less myself at the moment (I'm visiting in Strasbourg
again), so I'm afraid I cannot run the test for you.

     > By the way, while we are here, the only difference between
     > truncate_inode_pages and invalidate_inode_pages is the one that
     > you told here before?  I am documenting some of the MM stuff,
     > and your comments in that aspect are really wellcome.  (You
     > will have noted now that I am quite newbie here).

Well. As far as NFS and other non-disk based systems are concerned
that is the functional difference between the two. That and the fact
that truncate_inode_pages() takes an offset as an argument.

For disk-based systems, they are very different beasts, since
truncate_inode_pages() will also attempt to invalidate and/or wait on
any pending buffers on the pages it clears out.

Strictly speaking therefore, one should not confuse the two, however
truncate_inode_pages() is (ab)used as a sleeping substitute for
invalidate_inode_pages() by some of the icache pruning code in
fs/inode.c.

Cheers,
  Trond
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[John Cavan]

I'm by no means an expert in this, I just follow the list to learn, but
would it not be possible to make ITERATIONS count a runtime configurable
parameter in the /proc filesystem that defaults to 100? That would allow
for the best tuning scenario for a given system.

Just a thought.

John Cavan

"Juan J. Quintela" wrote:
> 
> >>>>> "trond" == Trond Myklebust <trond.myklebust@fys.uio.no> writes:
> 
> >>>>> " " == Juan J Quintela <quintela@fi.udc.es> writes:
> >> Then you want only invalidate the non_locked pages: do you
> 
> trond> That's right. This patch looks much more appropriate.
> 
> >> + while (count == ITERATIONS) {
> >> + spin_lock(&pagecache_lock);
> >> + spin_lock(&pagemap_lru_lock);
> >> + head = &inode->i_mapping->pages;
> >> + curr = head->next;
> >> + count = 0;
> >> +
> >> + while ((curr != head) && (count++ < ITERATIONS)) {
> 
> trond> Just one question: Isn't it better to do it all in 1 iteration through
> trond> the loop rather than doing it in batches of 100 pages?
> trond> You can argue that you're freeing up the spinlocks for the duration of
> trond> the loop_and_test, but is that really going to make a huge difference
> trond> to SMP performance?
> 
> Trond, I have not an SMP machine (yet), and I can not tell you numbers
> now.  I put the counter there to show that we *may* want to limit the
> latency there.  I am thinking in the write of a big file, that can
> take a lot to free all the pages, but I don't know, *you* are the NFS
> expert, this was one of the reasons that we want feedback from the
> users of the call.  (You have been very good giving comments).
> 
> My idea to put a limit is to put a limit than normally you do all in
> one iteration, but in the exceptional case of a big amount of pages,
> the latency is limited.  There is a limit in the number of pages that
> can be in that list?
> 
> 100 is one number that can need tuning, I don't know.  SMP experts
> anywhere?
> 
> By the way, while we are here, the only difference between
> truncate_inode_pages and invalidate_inode_pages is the one that you
> told here before?  I am documenting some of the MM stuff, and your
> comments in that aspect are really wellcome.  (You will have noted
> now that I am quite newbie here).
> 
> Later, Juan.
> 
> --
> In theory, practice and theory are the same, but in practice they
> are different -- Larry McVoy
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.rutgers.edu
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Juan J. Quintela]

>>>>> "john" == John Cavan <john.cavan@sympatico.ca> writes:

Hi

john> I'm by no means an expert in this, I just follow the list to learn, but
john> would it not be possible to make ITERATIONS count a runtime configurable
john> parameter in the /proc filesystem that defaults to 100? That would allow
john> for the best tuning scenario for a given system.

This was a first approach patch, if people like the thing configurable,
I can try to do that the weekend.

Notice: that will be my first trip to /proc land....

The rest of the people think that that value would need to be tunable
(I am not strong about that, but I think that I would not need to be
tunable,  perhaps 100 is not the correct value, but I think that one
value that didn't put a lot of latency would be enough).

Later, Juan.

-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Trond Myklebust]

>>>>> " " == Juan J Quintela <quintela@fi.udc.es> writes:

     > This was a first approach patch, if people like the thing
     > configurable, I can try to do that the weekend.

Juan, Linus,

   Could you please look into changing the name of
invalidate_inode_pages() to invalidate_pages_noblock() or something
like that? Since NFS is the only place where this function is used, a
change of name should not break any other code.

The reason I think this is necessary, is that this is the second time
the 2.3.x kernel is broken because somebody has misunderstood, and has
added wait_on_page() functionality to the same function.
Alternatively, please make sure that we add explicit comments to that
effect.

     > Notice: that will be my first trip to /proc land....

Ugh. Sounds like an extremely complex "solution" to something which
has not yet been demonstrated to be a problem.

Cheers,
  Trond
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Arjan van de Ven]

In article <shs7ld0dj8x.fsf@charged.uio.no> you wrote:
>>>>>> " " == Juan J Quintela <quintela@fi.udc.es> writes:

>      > This was a first approach patch, if people like the thing
>      > configurable, I can try to do that the weekend.

> Juan, Linus,

>    Could you please look into changing the name of
> invalidate_inode_pages() to invalidate_pages_noblock() or something
> like that? Since NFS is the only place where this function is used, a
> change of name should not break any other code.

I'd vote for "invalidate_unlocked_inode_pages", as it also suggests
that the locked pages aren't invalidated.

Greetings,
   Arjan van de Ven
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Trond Myklebust]

>>>>> " " == Arjan van de Ven <arjan@fenrus.demon.nl> writes:


    >> Could you please look into changing the name of
    >> invalidate_inode_pages() to invalidate_pages_noblock() or
    >> something like that? Since NFS is the only place where this
    >> function is used, a change of name should not break any other
    >> code.

     > I'd vote for "invalidate_unlocked_inode_pages", as it also
     > suggests that the locked pages aren't invalidated.

That sounds very good to me. Just so long as the name becomes more
self-documenting than it is now.
Intelligent people are making mistakes about what we want to do with
this function, so it definitely needs to be documented more clearly.

Cheers,
  Trond
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Juan J. Quintela]

>>>>> "trond" == Trond Myklebust <trond.myklebust@fys.uio.no> writes:

>>>>> " " == Arjan van de Ven <arjan@fenrus.demon.nl> writes:
..

>> I'd vote for "invalidate_unlocked_inode_pages", as it also
>> suggests that the locked pages aren't invalidated.

trond> That sounds very good to me. Just so long as the name becomes more
trond> self-documenting than it is now.
trond> Intelligent people are making mistakes about what we want to do with
trond> this function, so it definitely needs to be documented more clearly.

Here it is a patch that changes all the refernences in the kernel from
invalidate_inode_pages to invalidate_unlocked_inode_pages.

I have CC: the mail to linux-fsdevel because the SMB people also use
that function.

Comments?

Later, Juan.

PD. This patch aplies in top of my previous patch to this function,
    that you can get from the MM-list of from:

http://carpanta.dc.fi.udc.es/~quintela/kernel/2.3.99-pre7/page_cache_get.diff


diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS works/fs/nfs/inode.c testing/fs/nfs/inode.c
--- works/fs/nfs/inode.c	Wed Apr 26 02:28:56 2000
+++ testing/fs/nfs/inode.c	Fri May 12 19:22:00 2000
@@ -586,7 +586,7 @@
 	NFS_ATTRTIMEO(inode) = NFS_MINATTRTIMEO(inode);
 	NFS_ATTRTIMEO_UPDATE(inode) = jiffies;
 
-	invalidate_inode_pages(inode);
+	invalidate_unlocked_inode_pages(inode);
 
 	memset(NFS_COOKIEVERF(inode), 0, sizeof(NFS_COOKIEVERF(inode)));
 	NFS_CACHEINV(inode);
@@ -982,12 +982,12 @@
  * of the server's inode.
  *
  * This is a bit tricky because we have to make sure all dirty pages
- * have been sent off to the server before calling invalidate_inode_pages.
- * To make sure no other process adds more write requests while we try
- * our best to flush them, we make them sleep during the attribute refresh.
+ * have been sent off to the server before calling
+ * invalidate_unlocked_inode_pages.  To make sure no other process
+ * adds more write requests while we try our best to flush them, we
+ * make them sleep during the attribute refresh.
  *
- * A very similar scenario holds for the dir cache.
- */
+ * A very similar scenario holds for the dir cache.  */
 int
 nfs_refresh_inode(struct inode *inode, struct nfs_fattr *fattr)
 {
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS works/fs/smbfs/cache.c testing/fs/smbfs/cache.c
--- works/fs/smbfs/cache.c	Sat Dec  4 09:30:56 1999
+++ testing/fs/smbfs/cache.c	Fri May 12 19:22:21 2000
@@ -326,7 +326,7 @@
 	 * Get rid of any unlocked pages, and clear the
 	 * 'valid' flag in case a scan is in progress.
 	 */
-	invalidate_inode_pages(dir);
+	invalidate_unlocked_inode_pages(dir);
 	dir->u.smbfs_i.cache_valid &= ~SMB_F_CACHEVALID;
 	dir->u.smbfs_i.oldmtime = 0;
 }
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS works/fs/smbfs/inode.c testing/fs/smbfs/inode.c
--- works/fs/smbfs/inode.c	Thu Mar 16 01:51:14 2000
+++ testing/fs/smbfs/inode.c	Fri May 12 19:22:12 2000
@@ -205,7 +205,7 @@
 			 * But we do want to invalidate the caches ...
 			 */
 			if (!S_ISDIR(inode->i_mode))
-				invalidate_inode_pages(inode);
+				invalidate_unlocked_inode_pages(inode);
 			else
 				smb_invalid_dir_cache(inode);
 			error = -EIO;
@@ -263,7 +263,7 @@
 (long) last_time, (long) inode->i_mtime);
 #endif
 		if (!S_ISDIR(inode->i_mode))
-			invalidate_inode_pages(inode);
+			invalidate_unlocked_inode_pages(inode);
 		else
 			smb_invalid_dir_cache(inode);
 	}
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS works/include/linux/fs.h testing/include/linux/fs.h
--- works/include/linux/fs.h	Fri May 12 01:11:42 2000
+++ testing/include/linux/fs.h	Fri May 12 19:22:52 2000
@@ -954,7 +954,7 @@
 extern void balance_dirty(kdev_t);
 extern int check_disk_change(kdev_t);
 extern int invalidate_inodes(struct super_block *);
-extern void invalidate_inode_pages(struct inode *);
+extern void invalidate_unlocked_inode_pages(struct inode *);
 #define invalidate_buffers(dev)	__invalidate_buffers((dev), 0)
 #define destroy_buffers(dev)	__invalidate_buffers((dev), 1)
 extern void __invalidate_buffers(kdev_t dev, int);
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS works/kernel/ksyms.c testing/kernel/ksyms.c
--- works/kernel/ksyms.c	Fri May 12 01:11:43 2000
+++ testing/kernel/ksyms.c	Fri May 12 19:22:35 2000
@@ -175,7 +175,7 @@
 EXPORT_SYMBOL(check_disk_change);
 EXPORT_SYMBOL(__invalidate_buffers);
 EXPORT_SYMBOL(invalidate_inodes);
-EXPORT_SYMBOL(invalidate_inode_pages);
+EXPORT_SYMBOL(invalidate_unlocked_inode_pages);
 EXPORT_SYMBOL(truncate_inode_pages);
 EXPORT_SYMBOL(fsync_dev);
 EXPORT_SYMBOL(permission);
diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS works/mm/filemap.c testing/mm/filemap.c
--- works/mm/filemap.c	Fri May 12 19:03:11 2000
+++ testing/mm/filemap.c	Fri May 12 19:22:46 2000
@@ -112,14 +112,15 @@
 #define ITERATIONS 100
 
 /**
- * invalidate_inode_pages - Invalidate all the unlocked pages of one inode
+ * invalidate_unlocked_inode_pages - Invalidate all the unlocked
+ * pages that inode
  * @inode: the inode which pages we want to invalidate
  *
  * This function only removes the unlocked pages, if you want to
  * remove all the pages of one inode, you must call truncate_inode_pages.
  */
 
-void invalidate_inode_pages(struct inode * inode)
+void invalidate_unlocked_inode_pages(struct inode * inode)
 {
 	struct list_head *head, *curr;
 	struct page * page;


-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Juan J. Quintela]

>>>>> "trond" == Trond Myklebust <trond.myklebust@fys.uio.no> writes:
Hi

trond>    Could you please look into changing the name of
trond> invalidate_inode_pages() to invalidate_pages_noblock() or something
trond> like that? Since NFS is the only place where this function is used, a
trond> change of name should not break any other code.

We was talking about that in IRC just now....
I will do the patch later today.

trond> The reason I think this is necessary, is that this is the second time
trond> the 2.3.x kernel is broken because somebody has misunderstood, and has
trond> added wait_on_page() functionality to the same function.
trond> Alternatively, please make sure that we add explicit comments to that
trond> effect.

In my last patch there are a comment indicating that, that if you want
to wail for the *locked* pages also, you need to call truncate inode
pages.  I will study truncate_inode_pages and their use later today,
came here with a comment for the semantic of both functions, and
people can told me if they agree/disagree.

>> Notice: that will be my first trip to /proc land....

trond> Ugh. Sounds like an extremely complex "solution" to something which
trond> has not yet been demonstrated to be a problem.

I think the same. I will preffer to tune the number to be *not too
much time* and nothing else.

Later, Juan.



-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Jeff V. Merkey]

It should be expanded to support 64K pages.  Check
/usr/src/linux/include/asm-ia64/page.h.  IA64 supports page sizes up to
64K.  

:-)

Jeff

Linus Torvalds wrote:
> 
> On 11 May 2000, Juan J. Quintela wrote:
> > - we change one page_cache_release to put_page in truncate_inode_pages
> >   (people find lost when they see a get_page without the correspondent
> >   put_page, and put_page and page_cache_release are synonimops)
> 
> put_page() is _not_ synonymous with page_cache_release()!
> 
> Imagine a time in the not too distant future when the page cache
> granularity is 8kB or 16kB due to better IO performance (possibly
> controlled by a config option), and page_cache_release() will do an
> "order=1" or "order=2" page free..
> 
>                 Linus
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.rutgers.edu
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Trond Myklebust]

You seem to assume that invalidate_inode_pages() is supposed to
invalidate *all* pages in the inode. This is NOT the case, and any
rewrite is going to lead to hard lockups if you try to make it so.

Most calls to invalidate_inode_pages() are made while we hold the page
lock for some page that has just been updated (and hence we know is up
to date). The reason is that under NFS, we receive a set of attributes
as part of the result from READ/WRITE/... If this triggers a cache
invalidation, then we do not want to invalidate the page that we know
is safe, hence we call invalidate_inode_pages() before the newly read
in page is unlocked.

Your code of the form

    while (head != head->next) {
... 
   }

without some alternative method of exit will therefore lock up under NFS.

Filesystems which want to make sure they clear out locked pages should
use truncate_inode_pages() instead.

Cheers,
  Trond
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Juan J. Quintela]

>>>>> "trond" == Trond Myklebust <trond.myklebust@fys.uio.no> writes:

Hi Trond

trond> You seem to assume that invalidate_inode_pages() is supposed to
trond> invalidate *all* pages in the inode. This is NOT the case, and any
trond> rewrite is going to lead to hard lockups if you try to make it so.

Or I don't understand some obvious (the big probability) or the old
code also try to invalidate *all* the pages in the inode.  If there
are some locked page we do a goto repeat, we do 

       head = &inode->i_mapping->pages;
and 
       curr = head->next; 

and then the test is:
    while(head != curr)

I can't see any difference with doing:

    head = &inode->i_mapping->pages;
    while (head != head->next)

(I have removed the locking to clarify the example).
It can be that I am not understanding something obvious, but I think
that the old code also invalidates oll the pages.

trond> Most calls to invalidate_inode_pages() are made while we hold the page
trond> lock for some page that has just been updated (and hence we know is up
trond> to date). The reason is that under NFS, we receive a set of attributes
trond> as part of the result from READ/WRITE/... If this triggers a cache
trond> invalidation, then we do not want to invalidate the page that we know
trond> is safe, hence we call invalidate_inode_pages() before the newly read
trond> in page is unlocked.

I think that the old code will not finish until we liberate all the
pages, the only way to go out the code is the continue or the end of
the loop, that does a goto before the loop.  I think that the only
real difference between the two codes (locking issues aside) is that
the old version does busy waiting, and new one, liberates all the
non_locked pages and then sleeps waiting one page to become unlocked.
the other version when find one page locked will continue until it
liberates one non locked page, and then will "repeat" the process from
the begining.  If my reasoning is wrong, please point me where, I can
see where.

Thanks a lot for your comments and your good work.

Later, Juan.

-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

Re: PATCH: rewrite of invalidate_inode_pages

[Trond Myklebust]

>>>>> " " == Juan J Quintela <quintela@fi.udc.es> writes:

     > (I have removed the locking to clarify the example).  It can be
     > that I am not understanding something obvious, but I think that
     > the old code also invalidates oll the pages.

No it doesn't. If there are locked pages it skips them. In the end we 
should find ourselves with a ring of locked pages, so we're doing the
equivalent of the loop

  while (head != curr) {
      curr = curr->next;
      if (PageLocked(page))
	    continue;
      .... This code is no longer called 'cos all pages are locked .....
  }


     > new one, liberates all the non_locked pages and then sleeps
     > waiting one page to become unlocked.  the other version when

This is wrong. The reason is that under NFS, the rpciod can call
invalidate_inode_pages(). If it sleeps on a locked page, then it means
we must have some page IO in progress on that page. Who serves page IO
under NFS? rpciod.
So we deadlock...

As I said. The whole idea behind invalidate_inode_pages() is to serve
the need of NFS (and any future filesystems) for non-blocking
invalidation of the page cache.

Cheers,
  Trond
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/