PATCH: Bug in invalidate_inode_pages()?

PATCH: Bug in invalidate_inode_pages()?

[Juan J. Quintela]

Hi
        I think that I have found a bug in invalidate_inode_pages.
It results that we don't remove the pages from the
&inode->i_mapping->pages list, then when we return te do the next loop
through all the pages, we can try to free a page that we have freed in
the previous pass.  Once here I have also removed the goto

Comments, have I lost something obvious?

Later, Juan.

diff -u -urN --exclude=CVS --exclude=*~ --exclude=.#* --exclude=TAGS pre7-6/mm/filemap.c testing2/mm/filemap.c
--- pre7-6/mm/filemap.c	Fri May  5 23:58:56 2000
+++ testing2/mm/filemap.c	Tue May  9 01:37:57 2000
@@ -121,6 +121,7 @@
 		/* We cannot invalidate a locked page */
 		if (TryLockPage(page))
 			continue;
+                list_del(curr);
 		spin_unlock(&pagecache_lock);
 
 		lru_cache_del(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: Bug in invalidate_inode_pages()?

[Linus Torvalds]

On 9 May 2000, Juan J. Quintela wrote:
>         I think that I have found a bug in invalidate_inode_pages.
> It results that we don't remove the pages from the
> &inode->i_mapping->pages list, then when we return te do the next loop
> through all the pages, we can try to free a page that we have freed in
> the previous pass.

This is what "remove_inode_page()" does. Maybe that's not quite clear
enough, so this function may certainly need some comments or something
like that, but your patch is wrong (it will now delete the thing twice,
which can and will result in list corruption).

>  Once here I have also removed the goto

Because we dropped the page cache lock, we really have to repeat, because
the lists are now no longer protected..

		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: Bug in invalidate_inode_pages()?

[Juan J. Quintela]

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


Hi

linus> On 9 May 2000, Juan J. Quintela wrote:
>> I think that I have found a bug in invalidate_inode_pages.
>> It results that we don't remove the pages from the
>> &inode->i_mapping->pages list, then when we return te do the next loop
>> through all the pages, we can try to free a page that we have freed in
>> the previous pass.

linus> This is what "remove_inode_page()" does. Maybe that's not quite clear
linus> enough, so this function may certainly need some comments or something
linus> like that, but your patch is wrong (it will now delete the thing twice,
linus> which can and will result in list corruption).

Then there is the same inode->i_mapping_>pages list and page->list?
If that is the case I think that I would make one comment there
indicating that.

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: Bug in invalidate_inode_pages()?

[Trond Myklebust]

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

     > Hi
     >         I think that I have found a bug in
     >         invalidate_inode_pages.
     > It results that we don't remove the pages from the
     > &inode->i_mapping->pages list, then when we return te do the
     > next loop through all the pages, we can try to free a page that
     > we have freed in the previous pass.  Once here I have also
     > removed the goto

     > Comments, have I lost something obvious?

Unfortunately, yes...

  Firstly, you're removing the wrong page (viz. curr = curr->next).

  Secondly, we're already removing the page from the mapping using the
  inlined function remove_page_from_inode_queue() which is again
  called by remove_inode_page(). This also updates mapping->nrpages.

So invalidate_inode_pages() is correct as it stands.

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/