filemap_fdatawait() wait_on_page_writeback_range(mapping, 0, -1)?

filemap_fdatawait() wait_on_page_writeback_range(mapping, 0, -1)?

[Marcelo Tosatti]

Hi Andrew,

I dont understand why we do call wait_on_page_writeback_range() with -1 
as the "end" argument.

Is there a good reason for that? I bet so...

-1 sounds pretty stupid at first, it does unnecessary calls to 
the radix lookup code.

--- a/mm/filemap.c.orig      2004-08-19 14:36:02.000000000 -0300
+++ b/mm/filemap.c.isize     2004-08-19 18:17:14.000000000 -0300
@@ -231,7 +231,7 @@
  */
 int filemap_fdatawait(struct address_space *mapping)
 
-       return wait_on_page_writeback_range(mapping, 0, -1);
+       return wait_on_page_writeback_range(mapping, 0, i_size_read(mapping->host));
 }
  
 EXPORT_SYMBOL(filemap_fdatawait);

Re: filemap_fdatawait() wait_on_page_writeback_range(mapping, 0, -1)?

[Andrew Morton]

Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
>
> Hi Andrew,
> 
> I dont understand why we do call wait_on_page_writeback_range() with -1 
> as the "end" argument.

"every page in the file".

> -1 sounds pretty stupid at first, it does unnecessary calls to 
> the radix lookup code.

I guess it could cause one extra call into the lookup code.  There's an
additional check in -mm's wait_on_page_writeback_range() which would prevent
that.

> --- a/mm/filemap.c.orig      2004-08-19 14:36:02.000000000 -0300
> +++ b/mm/filemap.c.isize     2004-08-19 18:17:14.000000000 -0300
> @@ -231,7 +231,7 @@
>   */
>  int filemap_fdatawait(struct address_space *mapping)
>  
> -       return wait_on_page_writeback_range(mapping, 0, -1);
> +       return wait_on_page_writeback_range(mapping, 0, i_size_read(mapping->host));
>  }

That would need a >> PAGE_CACHE_SHIFT

Re: filemap_fdatawait() wait_on_page_writeback_range(mapping, 0, -1)?

[Marcelo Tosatti]

On Thu, Aug 19, 2004 at 02:49:47PM -0700, Andrew Morton wrote:
> Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
> >
> > Hi Andrew,
> > 
> > I dont understand why we do call wait_on_page_writeback_range() with -1 
> > as the "end" argument.
> 
> "every page in the file".
> 
> > -1 sounds pretty stupid at first, it does unnecessary calls to 
> > the radix lookup code.
> 
> I guess it could cause one extra call into the lookup code.  There's an
> additional check in -mm's wait_on_page_writeback_range() which would prevent
> that.

this? 

+                       /* until radix tree lookup accepts end_index */
+                       if (page->index > end)
+                               continue;

I dont see why that would make a difference. 

What seem to happen is that when we get near the EOF, the min calculation 
which could make sense, doesnt:

min(end - index, (pgoff_t)PAGEVEC_SIZE-1)

and some unnecessary cycles will be spent doing search at __lookup_tag(). And 
using i_size_read() is more meaningful anyway.

What I'm trying to do is make wait_on_page_writeback_range() do reverse
search instead ascending. Since we write pages in ascending order, doing 
the wait on reverse order makes sense and will avoid possibly tons of 
wakeups.

Naive me tried to implement that using pagevec_lookup_tag(), but I'm
convinced we need pagevec_reverse_lookup_tag() do reverse search
on the radix tree. I'll try getting that done on the weekend.

What you think about it?

> > --- a/mm/filemap.c.orig      2004-08-19 14:36:02.000000000 -0300
> > +++ b/mm/filemap.c.isize     2004-08-19 18:17:14.000000000 -0300
> > @@ -231,7 +231,7 @@
> >   */
> >  int filemap_fdatawait(struct address_space *mapping)
> >  
> > -       return wait_on_page_writeback_range(mapping, 0, -1);
> > +       return wait_on_page_writeback_range(mapping, 0, i_size_read(mapping->host));
> >  }
> 
> That would need a >> PAGE_CACHE_SHIFT

Oh yes, right. I was too excited :)

Fixed version with changelog, against 2.6.8.1-mm2:

filemap_fdatawait() calls wait_on_page_writeback_range() with -1 as "end" parameter.
This is not needed since we know the EOF from the inode. Use that instead.

Signed-off-by: Marcelo Tosatti <marcelo.tosatti@cyclades.com>

diff -u linux-2.6.8/mm/filemap.c.orig linux-2.6.8/mm/filemap.c
--- linux-2.6.8/mm/filemap.c.orig       2004-08-19 20:11:52.000000000 -0300
+++ linux-2.6.8/mm/filemap.c    2004-08-19 20:12:40.000000000 -0300
@@ -288,7 +288,8 @@
  */
 int filemap_fdatawait(struct address_space *mapping)
 {
-       return wait_on_page_writeback_range(mapping, 0, -1);
+       return wait_on_page_writeback_range(mapping, 0,
+                       i_size_read(mapping->host) >> PAGE_CACHE_SHIFT);
 }
 EXPORT_SYMBOL(filemap_fdatawait);
  

Re: filemap_fdatawait() wait_on_page_writeback_range(mapping, 0, -1)?

[Andrew Morton]

Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
>
> On Thu, Aug 19, 2004 at 02:49:47PM -0700, Andrew Morton wrote:
> > Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
> > >
> > > Hi Andrew,
> > > 
> > > I dont understand why we do call wait_on_page_writeback_range() with -1 
> > > as the "end" argument.
> > 
> > "every page in the file".
> > 
> > > -1 sounds pretty stupid at first, it does unnecessary calls to 
> > > the radix lookup code.
> > 
> > I guess it could cause one extra call into the lookup code.  There's an
> > additional check in -mm's wait_on_page_writeback_range() which would prevent
> > that.
> 
> this? 
> 
> +                       /* until radix tree lookup accepts end_index */
> +                       if (page->index > end)
> +                               continue;

No, I'm being thick.  Please ignore.

> What I'm trying to do is make wait_on_page_writeback_range() do reverse
> search instead ascending. Since we write pages in ascending order, doing 
> the wait on reverse order makes sense and will avoid possibly tons of 
> wakeups.

Yes, that's probably the low-hanging-fruit wrt CPU consumption in there.

> Naive me tried to implement that using pagevec_lookup_tag(), but I'm
> convinced we need pagevec_reverse_lookup_tag() do reverse search
> on the radix tree. I'll try getting that done on the weekend.

Yes, a descending-order gang lookup would be needed.

It'd be tricky to implement.  Probably you could get just as much benefit
by simply waiting on the highest-index page prior to doing the full-range
walk.  Certainly that'd be interesting as a first step: run some tests, see
what impact it has on context switch totals.

Bear in mind that while waiting on the pages in ascending-offset-order
does incur extra context switches, it also yields lowest latency.  Because it
pipelines the inspection of each page with the ongoing I/O.

Re: filemap_fdatawait() wait_on_page_writeback_range(mapping, 0, -1)?

[Andrew Morton]

Anton Altaparmakov <aia21@cam.ac.uk> wrote:
>
> > >   */
>  > >  int filemap_fdatawait(struct address_space *mapping)
>  > >  
>  > > -       return wait_on_page_writeback_range(mapping, 0, -1);
>  > > +       return wait_on_page_writeback_range(mapping, 0, i_size_read(mapping->host));
>  > >  }
>  > 
>  > That would need a >> PAGE_CACHE_SHIFT
> 
>  Um, what happens to the last partial page?  Perhaps this is necessary:
> 
>  (i_size_read(mapping->host) + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT

/*
 * Wait for writeback to complete against pages indexed by start->end
 * inclusive
 */
static int wait_on_page_writeback_range(struct address_space *mapping,
				pgoff_t start, pgoff_t end)


(start,end) is an inclusive range specifically because of the
do-it-for-the-whole-file requirement.