Request for export of truncate_complete_page

Request for export of truncate_complete_page

[Oleg Drokin]

Hello!

   Can I ask for truncate_complete_page() to be exported?
   For Lustre filesystem it is necessary to poke out pages in the middle of
   a file, but truncate_inode_pages() is not very suitable, because we
   poke those pages one at a time when locks on those pages are cancelled, but
   we cannot kill entire set of pages as a group, because there might be some
   other lock that covers a subset of those pages, so we still need to iterate
   through all of them, and while we are at it, it is easier to kill pages
   as we check them one by one.

--- linux-2.6.16-rc4/include/linux/mm.h.orig	2006-02-21 00:22:58.000000000 +0200
+++ linux-2.6.16-rc4/include/linux/mm.h	2006-02-21 00:23:37.000000000 +0200
@@ -941,6 +941,8 @@ extern unsigned long do_brk(unsigned lon
 
 /* filemap.c */
 extern unsigned long page_unuse(struct page *);
+extern void truncate_complete_page(struct address_space *mapping,
+                                   struct page *page);
 extern void truncate_inode_pages(struct address_space *, loff_t);
 extern void truncate_inode_pages_range(struct address_space *,
 				       loff_t lstart, loff_t lend);
--- linux-2.6.16-rc4/mm/truncate.c.orig	2006-02-21 00:20:27.000000000 +0200
+++ linux-2.6.16-rc4/mm/truncate.c	2006-02-21 00:21:42.000000000 +0200
@@ -33,7 +33,7 @@ static inline void truncate_partial_page
  * its lock, b) when a concurrent invalidate_inode_pages got there first and
  * c) when tmpfs swizzles a page between a tmpfs inode and swapper_space.
  */
-static void
+void
 truncate_complete_page(struct address_space *mapping, struct page *page)
 {
 	if (page->mapping != mapping)
@@ -48,6 +48,7 @@ truncate_complete_page(struct address_sp
 	remove_from_page_cache(page);
 	page_cache_release(page);	/* pagecache ref */
 }
+EXPORT_SYMBOL(truncate_complete_page);
 
 /*
  * This is for invalidate_inode_pages().  That function can be called at


Bye,
    Oleg

Re: Request for export of truncate_complete_page

[Andrew Morton]

Oleg Drokin <green@linuxhacker.ru> wrote:
>
> Hello!
> 
>    Can I ask for truncate_complete_page() to be exported?
>    For Lustre filesystem it is necessary to poke out pages in the middle of
>    a file, but truncate_inode_pages() is not very suitable, because we
>    poke those pages one at a time when locks on those pages are cancelled, but
>    we cannot kill entire set of pages as a group, because there might be some
>    other lock that covers a subset of those pages, so we still need to iterate
>    through all of them, and while we are at it, it is easier to kill pages
>    as we check them one by one.
> 

Isn't truncate_inode_pages_range() suitable?

> +EXPORT_SYMBOL(truncate_complete_page);

_GPL would be nicer.   Plus a comment to keep people from "cleaning it up".

Re: Request for export of truncate_complete_page

[Christoph Hellwig]

On Tue, Feb 21, 2006 at 12:38:10AM +0200, Oleg Drokin wrote:
> Hello!
> 
>    Can I ask for truncate_complete_page() to be exported?
>    For Lustre filesystem it is necessary to poke out pages in the middle of
>    a file, but truncate_inode_pages() is not very suitable, because we
>    poke those pages one at a time when locks on those pages are cancelled, but
>    we cannot kill entire set of pages as a group, because there might be some
>    other lock that covers a subset of those pages, so we still need to iterate
>    through all of them, and while we are at it, it is easier to kill pages
>    as we check them one by one.

Truncating around inside a file is very fishy, so this would need a review
of cour code.  Once you submit the new lustre client we can discuss exporting
this as _GPL, before we won't export it anyway.

Re: Request for export of truncate_complete_page

[Oleg Drokin]

Hello!

On Mon, Feb 20, 2006 at 09:54:10PM -0800, Andrew Morton wrote:
> >    Can I ask for truncate_complete_page() to be exported?
> >    For Lustre filesystem it is necessary to poke out pages in the middle of
> >    a file, but truncate_inode_pages() is not very suitable, because we
> >    poke those pages one at a time when locks on those pages are cancelled, but
> >    we cannot kill entire set of pages as a group, because there might be some
> >    other lock that covers a subset of those pages, so we still need to iterate
> >    through all of them, and while we are at it, it is easier to kill pages
> >    as we check them one by one.
> Isn't truncate_inode_pages_range() suitable?

No, for the reason I specified above. We still walk entire region on page by
page basis, checking that page is not covered by other locks that might warrant
its exclusion from truncating, pushing writing of the page if it is still dirty
and needs to be written (or clearing its dirty flag if it needs to be
discarded).
So we have sort of reimplemented truncate_inode_pages_range() with extra logic
specific to Lustre and calling truncate_inode_pages_range() for every page
we want to get rid of is overkill.

> > +EXPORT_SYMBOL(truncate_complete_page);
> _GPL would be nicer.   Plus a comment to keep people from "cleaning it up".

Not that I mind if it is GPL or not, but currently truncate_complete_page()
can be reimplemented with EXPORT_SYMBOL stuff only. Here's how:

static inline void ll_remove_from_page_cache(struct page *page)
{
        struct address_space *mapping = page->mapping;

        BUG_ON(!PageLocked(page));

        write_lock_irq(&mapping->tree_lock);
        radix_tree_delete(&mapping->page_tree, page->index);
        page->mapping = NULL;
        mapping->nrpages--;
        atomic_add(-1, &nr_pagecache); // XXX pagecache_acct(-1);
        write_unlock_irq(&mapping->tree_lock);
}

static inline void
truncate_complete_page(struct address_space *mapping, struct page *page)
{
        if (page->mapping != mapping)
                return;

        if (PagePrivate(page))
                page->mapping->a_ops->invalidatepage(page, 0);

        clear_page_dirty(page);
        ClearPageUptodate(page);
        ClearPageMappedToDisk(page);
        ll_remove_from_page_cache(page);
        page_cache_release(page);       /* pagecache ref */
}

Bye,
    Oleg