Skip to contentSkip to conversation

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

PATCH Marcelo Tosatti 2 participants 5 messages Aug 19, 2004 – Aug 20, 2004 1 message not in the archive

Subsystems (2)
Memory management Page cache
lore
Share & tools
lore

Marcelo Tosatti

author
#0 lore raw

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.

mm/filemap.c.isize 2004-08-19 18:17:14.000000000 -0300 renamed from mm/filemap.c.orig 2004-08-19 14:36:02.000000000 -0300 +1 -1
@@ -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);
#1 lore raw
Quoted 5 lines · Marcelo Tosatti

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".

Quoted 2 lines

-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.

mm/filemap.c.isize 2004-08-19 18:17:14.000000000 -0300 line 235

Quoted 9 lines
mm/filemap.c.isize 2004-08-19 18:17:14.000000000 -0300 renamed from mm/filemap.c.orig 2004-08-19 14:36:02.000000000 -0300 +1 -1
@@ -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

author
#2 lore raw
Quoted 7 lines · Andrew Morton
Quoted 5 lines · Marcelo Tosatti

Hi Andrew,

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

Quoted 8 lines

"every page in the file".

Quoted 2 lines

-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?

Quoted 11 lines

mm/filemap.c.isize 2004-08-19 18:17:14.000000000 -0300 line 235

Quoted 9 lines
mm/filemap.c.isize 2004-08-19 18:17:14.000000000 -0300 renamed from mm/filemap.c.orig 2004-08-19 14:36:02.000000000 -0300 +1 -1
@@ -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 <[email protected]>
mm/filemap.c 2004-08-19 20:12:40.000000000 -0300 renamed from mm/filemap.c.orig 2004-08-19 20:11:52.000000000 -0300 +2 -1
diff -u linux-2.6.8/mm/filemap.c.orig linux-2.6.8/mm/filemap.c
@@ -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);
#3 lore raw
Quoted 20 lines · Marcelo Tosatti
Quoted 7 lines · Andrew Morton
Quoted 5 lines · Marcelo Tosatti

Hi Andrew,

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

Quoted 8 lines

"every page in the file".

Quoted 2 lines

-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?

Review

Quoted 3 lines
(quoted diff) +3 -0
+                       /* until radix tree lookup accepts end_index */
+                       if (page->index > end)
+                               continue;

No, I'm being thick. Please ignore.

Quoted 4 lines

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.

Quoted 3 lines

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.

#5 lore raw
In reply to a message not in the archive
Quoted 8 lines · Anton Altaparmakov
Quoted 8 lines
Quoted 7 lines

*/
int filemap_fdatawait(struct address_space *mapping)

mm/filemap.c.isize 2004-08-19 18:17:14.000000000 -0300 renamed from mm/filemap.c.orig 2004-08-19 14:36:02.000000000 -0300 +1 -1
-       return wait_on_page_writeback_range(mapping, 0, -1);
+       return wait_on_page_writeback_range(mapping, 0, i_size_read(mapping->host));
 }
Quoted 5 lines
Quoted 1 line

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.