Check for compound pages in set_page_dirty()

We have these checks scattered, makes sense to put them in set_page_dirty() instead. This also fixes a bug where __bio_unmap_user() does set_page_dirty_lock() without checking for a compound page, instead of adding one more check we move it to set_page_dirty().

Signed-off-by Jens Axboe <[email protected]>

fs/bio.c +1 -1

index cd888f9..ff96cd9 100644
@@ -902,7 +902,7 @@ void bio_set_pages_dirty(struct bio *bio)
 	for (i = 0; i < bio->bi_vcnt; i++) {
 		struct page *page = bvec[i].bv_page;
 
-		if (page && !PageCompound(page))
+		if (page)
 			set_page_dirty_lock(page);
 	}
 }

fs/direct-io.c +1 -1

index 52bb263..72195bc 100644
@@ -426,7 +426,7 @@ static int dio_bio_complete(struct dio *dio, struct bio *bio)
 		for (page_no = 0; page_no < bio->bi_vcnt; page_no++) {
 			struct page *page = bvec[page_no].bv_page;
 
-			if (dio->rw == READ && !PageCompound(page))
+			if (dio->rw == READ)
 				set_page_dirty_lock(page);
 			page_cache_release(page);
 		}

mm/page-writeback.c +0 -0

index 886ea0d..3c590b9 100644
@@ -861,8 +861,12 @@ EXPORT_SYMBOL(redirty_page_for_writepage);
  */
 int fastcall set_page_dirty(struct page *page)
 {

Quoted 42 lines · Jens Axboe

We have these checks scattered, makes sense to put them in set_page_dirty() instead. This also fixes a bug where __bio_unmap_user() does set_page_dirty_lock() without checking for a compound page, instead of adding one more check we move it to set_page_dirty().

Signed-off-by Jens Axboe <[email protected]>

fs/bio.c +1 -1

index cd888f9..ff96cd9 100644
@@ -902,7 +902,7 @@ void bio_set_pages_dirty(struct bio *bio)
 	for (i = 0; i < bio->bi_vcnt; i++) {
 		struct page *page = bvec[i].bv_page;
 
-		if (page && !PageCompound(page))
+		if (page)
 			set_page_dirty_lock(page);
 	}
 }

fs/direct-io.c +1 -1

index 52bb263..72195bc 100644
@@ -426,7 +426,7 @@ static int dio_bio_complete(struct dio *dio, struct bio *bio)
 		for (page_no = 0; page_no < bio->bi_vcnt; page_no++) {
 			struct page *page = bvec[page_no].bv_page;
 
-			if (dio->rw == READ && !PageCompound(page))
+			if (dio->rw == READ)
 				set_page_dirty_lock(page);
 			page_cache_release(page);
 		}

mm/page-writeback.c +0 -0

index 886ea0d..3c590b9 100644
@@ -861,8 +861,12 @@ EXPORT_SYMBOL(redirty_page_for_writepage);
  */
 int fastcall set_page_dirty(struct page *page)
 {

mm/page-writeback.c EXPORT_SYMBOL(redirty_page_for_writepage); · line 872

Quoted 10 lines

mm/page-writeback.c +5 -1

-	struct address_space *mapping = page_mapping(page);
+	struct address_space *mapping;
+	
+	if (unlikely(PageCompound(page)))
+		return 0;
 
+	mapping = page_mapping(page);
 	if (likely(mapping)) {
 		int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;
 #ifdef CONFIG_BLOCK

I'd prefer it if we just remove those two tests from fs without adding one into set_page_dirty at all; though I've not tested that recently (replying before remembering the easiest way to do so), and others may disagree.

The real reason for those tests was that pre-2.6.16 a compound page stored its destructor in page[1].mapping: which went badly wrong if that constituent page ever ended up being passed to set_page_dirty.

I moved it somewhere safer in 41d78ba55037468e6c86c53e3076d1a74841de39 but didn't have the courage to remove those tests you're now removing:
the earlier we skip out from that case, the more efficiently it's handled, I didn't want to slow those paths down in case they were important to someone.

So I'd be glad to see those tests now gone without replacement.

Quoted 63 lines · Hugh Dickins

Quoted 42 lines · Jens Axboe

We have these checks scattered, makes sense to put them in set_page_dirty() instead. This also fixes a bug where __bio_unmap_user() does set_page_dirty_lock() without checking for a compound page, instead of adding one more check we move it to set_page_dirty().

Signed-off-by Jens Axboe <[email protected]>

fs/bio.c +1 -1

index cd888f9..ff96cd9 100644
@@ -902,7 +902,7 @@ void bio_set_pages_dirty(struct bio *bio)
 	for (i = 0; i < bio->bi_vcnt; i++) {
 		struct page *page = bvec[i].bv_page;
 
-		if (page && !PageCompound(page))
+		if (page)
 			set_page_dirty_lock(page);
 	}
 }

fs/direct-io.c +1 -1

index 52bb263..72195bc 100644
@@ -426,7 +426,7 @@ static int dio_bio_complete(struct dio *dio, struct bio *bio)
 		for (page_no = 0; page_no < bio->bi_vcnt; page_no++) {
 			struct page *page = bvec[page_no].bv_page;
 
-			if (dio->rw == READ && !PageCompound(page))
+			if (dio->rw == READ)
 				set_page_dirty_lock(page);
 			page_cache_release(page);
 		}

mm/page-writeback.c +0 -0

index 886ea0d..3c590b9 100644
@@ -861,8 +861,12 @@ EXPORT_SYMBOL(redirty_page_for_writepage);
  */
 int fastcall set_page_dirty(struct page *page)
 {

mm/page-writeback.c EXPORT_SYMBOL(redirty_page_for_writepage); · line 872

Quoted 10 lines

mm/page-writeback.c +5 -1

-	struct address_space *mapping = page_mapping(page);
+	struct address_space *mapping;
+	
+	if (unlikely(PageCompound(page)))
+		return 0;
 
+	mapping = page_mapping(page);
 	if (likely(mapping)) {
 		int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;
 #ifdef CONFIG_BLOCK

I'd prefer it if we just remove those two tests from fs without adding one into set_page_dirty at all; though I've not tested that recently (replying before remembering the easiest way to do so), and others may disagree.

The real reason for those tests was that pre-2.6.16 a compound page stored its destructor in page[1].mapping: which went badly wrong if that constituent page ever ended up being passed to set_page_dirty.

Quoted 7 lines

I moved it somewhere safer in 41d78ba55037468e6c86c53e3076d1a74841de39 but didn't have the courage to remove those tests you're now removing:
the earlier we skip out from that case, the more efficiently it's handled, I didn't want to slow those paths down in case they were important to someone.

So I'd be glad to see those tests now gone without replacement.

OK, you clearly have more knowledge in that area than I, but I do wish that you would have made a note in the code at least to remove things like this. It's pretty ugly to have superflous tests like that, especially since there was not even a comment saying _why_ you could not call set_page_dirty() on a compound page. I see it in the commit text, but nobody looking at fs/bio.c or fs/direct-io.c would directly find any reference of that.

Could you submit a patch removing the tests?

Since I had my hands dirty already...

---

[PATCH] Remove PageCompound() checks before calling set_page_dirty()

Pre commit 41d78ba55037468e6c86c53e3076d1a74841de39 it was illegal to call set_page_dirty() on a compound page, since it stored the destructor in the mapping field. But now it's ok, so remove the ugly PageCompound() checks from bio and direct-io.

Signed-off-by Jens Axboe <[email protected]>

fs/bio.c +2 -2

index 33e4634..1e12416 100644
@@ -911,7 +911,7 @@ void bio_set_pages_dirty(struct bio *bio)
 	for (i = 0; i < bio->bi_vcnt; i++) {
 		struct page *page = bvec[i].bv_page;
 
-		if (page && !PageCompound(page))
+		if (page)
 			set_page_dirty_lock(page);
 	}
 }

@@ -978,7 +978,7 @@ void bio_check_pages_dirty(struct bio *bio)
 	for (i = 0; i < bio->bi_vcnt; i++) {
 		struct page *page = bvec[i].bv_page;
 
-		if (PageDirty(page) || PageCompound(page)) {
+		if (PageDirty(page)) {
 			page_cache_release(page);
 			bvec[i].bv_page = NULL;
 		} else {

fs/direct-io.c +1 -1

index 52bb263..72195bc 100644
@@ -426,7 +426,7 @@ static int dio_bio_complete(struct dio *dio, struct bio *bio)
 		for (page_no = 0; page_no < bio->bi_vcnt; page_no++) {
 			struct page *page = bvec[page_no].bv_page;
 
-			if (dio->rw == READ && !PageCompound(page))
+			if (dio->rw == READ)
 				set_page_dirty_lock(page);
 			page_cache_release(page);
 		}

Signature · 8 lines

-- 
Jens Axboe

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 2 lines · Jens Axboe

Since I had my hands dirty already...

Great, thanks. (There's also such a test in fs/nfs/direct.c, but let's not trouble Trond until we've settled what to do here.)

Quoted 11 lines

---

[PATCH] Remove PageCompound() checks before calling set_page_dirty()

Pre commit 41d78ba55037468e6c86c53e3076d1a74841de39 it was illegal to call set_page_dirty() on a compound page, since it stored the destructor in the mapping field. But now it's ok, so remove the ugly PageCompound() checks from bio and direct-io.

Signed-off-by Jens Axboe <[email protected]>

I was about to Ack that, now that I've found something or other in the libhugetlb testsuite comes this way, even on page[1], without showing any problem.

However, I have noticed a particular inefficiency arising: that bio_check_pages_dirty test specifically avoids pages already PageDirty; but hugetlbfs_set_page_dirty carefully redirects to set the head page dirty: so tail pages of a hugetlb compound page will tend never to be PageDirty, and keep on coming back this way.

Which led me to look up the origin of those PageCompound tests:

Author: Andrew Morton <[email protected]>
Date:   Sun Sep 21 01:42:22 2003 -0700

[PATCH] Speed up direct-io hugetlbpage handling

This patch short-circuits all the direct-io page dirtying logic for higher-order pages. Without this, we pointlessly bounce BIOs up to keventd all the time.

fs/bio.c +5 -0

index d016523..2463163 100644
@@ -532,6 +532,12 @@ void bio_unmap_user(struct bio *bio, int write_to_vm)
  * check that the pages are still dirty.   If so, fine.  If not, redirty them
  * in process context.
  *
+ * We special-case compound pages here: normally this means reads into hugetlb
+ * pages.  The logic in here doesn't really work right for compound pages
+ * because the VM does not uniformly chase down the head page in all cases.
+ * But dirtiness of compound pages is pretty meaningless anyway: the VM doesn't
+ * handle them at all.  So we skip compound pages here at an early stage.
...

It looks like I was wrong in thinking it was just trying to avoid the crash on page[1].mapping. At the least, your patch needs also to remove that paragraph of comment from Andrew. But really, it looks like those PageCompound tests should stay, unless you can persuade Andrew to Ack their removal.

Except (now, how many times can I change my mind in the course of one email?), hugetlbfs_set_page_dirty was specifically added by Ken Chen to avoid losing data via /proc/sys/vm/drop_caches. Yet fs/bio.c is carefully avoiding going there when dirtying a hugepage. How does this work? Looks like those PageCompound tests need to go!

I'm lost: I hope Andrew and Ken can sort it out for us.

Quoted 60 lines · Hugh Dickins

Quoted 2 lines · Jens Axboe

Since I had my hands dirty already...

Great, thanks. (There's also such a test in fs/nfs/direct.c, but let's not trouble Trond until we've settled what to do here.)

Quoted 11 lines

---

[PATCH] Remove PageCompound() checks before calling set_page_dirty()

Pre commit 41d78ba55037468e6c86c53e3076d1a74841de39 it was illegal to call set_page_dirty() on a compound page, since it stored the destructor in the mapping field. But now it's ok, so remove the ugly PageCompound() checks from bio and direct-io.

Signed-off-by Jens Axboe <[email protected]>

I was about to Ack that, now that I've found something or other in the libhugetlb testsuite comes this way, even on page[1], without showing any problem.

However, I have noticed a particular inefficiency arising: that bio_check_pages_dirty test specifically avoids pages already PageDirty; but hugetlbfs_set_page_dirty carefully redirects to set the head page dirty: so tail pages of a hugetlb compound page will tend never to be PageDirty, and keep on coming back this way.

Which led me to look up the origin of those PageCompound tests:

Author: Andrew Morton <[email protected]>
Date:   Sun Sep 21 01:42:22 2003 -0700

[PATCH] Speed up direct-io hugetlbpage handling

This patch short-circuits all the direct-io page dirtying logic for higher-order pages. Without this, we pointlessly bounce BIOs up to keventd all the time.

fs/bio.c +5 -0

index d016523..2463163 100644
@@ -532,6 +532,12 @@ void bio_unmap_user(struct bio *bio, int write_to_vm)
  * check that the pages are still dirty.   If so, fine.  If not, redirty them
  * in process context.
  *
+ * We special-case compound pages here: normally this means reads into hugetlb
+ * pages.  The logic in here doesn't really work right for compound pages
+ * because the VM does not uniformly chase down the head page in all cases.
+ * But dirtiness of compound pages is pretty meaningless anyway: the VM doesn't
+ * handle them at all.  So we skip compound pages here at an early stage.
...

It looks like I was wrong in thinking it was just trying to avoid the crash on page[1].mapping. At the least, your patch needs also to remove that paragraph of comment from Andrew. But really, it looks like those PageCompound tests should stay, unless you can persuade Andrew to Ack their removal.

Quoted 5 lines

Except (now, how many times can I change my mind in the course of one email?), hugetlbfs_set_page_dirty was specifically added by Ken Chen to avoid losing data via /proc/sys/vm/drop_caches. Yet fs/bio.c is carefully avoiding going there when dirtying a hugepage. How does this work? Looks like those PageCompound tests need to go!

Hehe, that didn't really get us much further, did it? :-)

My opinion is that since the win is marginal at best, we want to remove such tests as it just clutters up the code. And it's definitely not obvious why the tests are there, since they are not commented at all. Since it's even confusing you, then we can't expect the more vm ignorant of us (which definitely includes me) to grasp it!

Quoted 1 line

I'm lost: I hope Andrew and Ken can sort it out for us.

Posting a revised version, still leaving nfs out of it (I'll ping Trond to do the same, if this goes in).

Quoted 6 lines · Hugh Dickins

Quoted 2 lines · Jens Axboe

Since I had my hands dirty already...

Great, thanks. (There's also such a test in fs/nfs/direct.c, but let's not trouble Trond until we've settled what to do here.)

Quoted 50 lines

Quoted 8 lines

[PATCH] Remove PageCompound() checks before calling set_page_dirty()

Pre commit 41d78ba55037468e6c86c53e3076d1a74841de39 it was illegal to call set_page_dirty() on a compound page, since it stored the destructor in the mapping field. But now it's ok, so remove the ugly PageCompound() checks from bio and direct-io.

Signed-off-by Jens Axboe <[email protected]>

I was about to Ack that, now that I've found something or other in the libhugetlb testsuite comes this way, even on page[1], without showing any problem.

However, I have noticed a particular inefficiency arising: that bio_check_pages_dirty test specifically avoids pages already PageDirty; but hugetlbfs_set_page_dirty carefully redirects to set the head page dirty: so tail pages of a hugetlb compound page will tend never to be PageDirty, and keep on coming back this way.

Which led me to look up the origin of those PageCompound tests:

Author: Andrew Morton <[email protected]>
Date:   Sun Sep 21 01:42:22 2003 -0700

[PATCH] Speed up direct-io hugetlbpage handling

This patch short-circuits all the direct-io page dirtying logic for higher-order pages. Without this, we pointlessly bounce BIOs up to keventd all the time.

fs/bio.c +5 -0

index d016523..2463163 100644
@@ -532,6 +532,12 @@ void bio_unmap_user(struct bio *bio, int write_to_vm)
  * check that the pages are still dirty.   If so, fine.  If not, redirty them
  * in process context.
  *
+ * We special-case compound pages here: normally this means reads into hugetlb
+ * pages.  The logic in here doesn't really work right for compound pages
+ * because the VM does not uniformly chase down the head page in all cases.
+ * But dirtiness of compound pages is pretty meaningless anyway: the VM doesn't
+ * handle them at all.  So we skip compound pages here at an early stage.
...

It looks like I was wrong in thinking it was just trying to avoid the crash on page[1].mapping. At the least, your patch needs also to remove that paragraph of comment from Andrew. But really, it looks like those PageCompound tests should stay, unless you can persuade Andrew to Ack their removal.

Quoted 5 lines

Except (now, how many times can I change my mind in the course of one email?), hugetlbfs_set_page_dirty was specifically added by Ken Chen to avoid losing data via /proc/sys/vm/drop_caches. Yet fs/bio.c is carefully avoiding going there when dirtying a hugepage. How does this work? Looks like those PageCompound tests need to go!

Hehe, that didn't really get us much further, did it? :-)

My opinion is that since the win is marginal at best, we want to remove such tests as it just clutters up the code. And it's definitely not obvious why the tests are there, since they are not commented at all.

Since it's even confusing you, then we can't expect the more vm ignorant of us (which definitely includes me) to grasp it!

Quoted 1 line

I'm lost: I hope Andrew and Ken can sort it out for us.

Posting a revised version, still leaving nfs out of it (I'll ping Trond to do the same, if this goes in).

I started from your patch. But it now seems to me a bugfix to remove those PageCompound tests, because they're preventing a hugetlb page from being marked dirty, when Ken needs it to be marked dirty so /proc/sys/vm/drop_caches doesn't drop the data read in by DIO. (His original patch went into -stable: would the patch fixing this all up need to go into -stable?)

I didn't see any attention to set_page_dirty in Christoph's Large Blocksize (variable page_cachesize) patches, but I expect he'd also be wanting set_page_dirty to act on compound_head.

Quoted 3 lines · On Thu, 19 Jul 2007 18:35:17 +0100 (BST) Hugh Dickins <[email protected]> wrote:

I didn't see any attention to set_page_dirty in Christoph's Large Blocksize (variable page_cachesize) patches, but I expect he'd also be wanting set_page_dirty to act on compound_head.

There is no need for special casing if set_page_dirty() is only ever called for page heads. Make sure that direct I/O does not call set_page_dirty for tail pages.

A VM_BUG_ON(PageTail(page)) could be used to make sure that this does not happen in the future?

I expect we could take that approach in the current kernel, yes (though it would put those compound tests into the bio code that Jens was preferring to remove). But I think not if your variable page_cache_size went in: imagine an mmap of the tail component page of an order-1 page_cache_size page, and that pte only being dirtied:
wouldn't set_page_dirty on that page need to redirect to the head?

Quoted 6 lines · Hugh Dickins

I expect we could take that approach in the current kernel, yes (though it would put those compound tests into the bio code that Jens was preferring to remove). But I think not if your variable page_cache_size went in: imagine an mmap of the tail component page of an order-1 page_cache_size page, and that pte only being dirtied:
wouldn't set_page_dirty on that page need to redirect to the head?

We would need to redirect all of the page state determinations and changes to the head page anyways. So the memory.c code would have to deal with two struct page pointers: One to the head where the state is kept and one to the tail page that contains the actual chunk of data we are interested in. The tail page pointer is only used for address determinations.

VM functions that manipulate the state of a page (like set_page_dirty) could rely on only getting page heads.

Quoted 8 lines

We would need to redirect all of the page state determinations and changes to the head page anyways. So the memory.c code would have to deal with two struct page pointers: One to the head where the state is kept and one to the tail page that contains the actual chunk of data we are interested in. The tail page pointer is only used for address determinations.

VM functions that manipulate the state of a page (like set_page_dirty) could rely on only getting page heads.

Maybe. Sounds ugly. "would": so your patches remain just an RFC?

OK, you clearly have more knowledge in that area than I, but I do wish that you would have made a note in the code at least to remove things like this. It's pretty ugly to have superflous tests like that, especially since there was not even a comment saying _why_ you could not call set_page_dirty() on a compound page. I see it in the commit text, but nobody looking at fs/bio.c or fs/direct-io.c would directly find any reference of that.

Could you submit a patch removing the tests?

Quoted 9 lines · Jens Axboe

OK, you clearly have more knowledge in that area than I, but I do wish that you would have made a note in the code at least to remove things like this. It's pretty ugly to have superflous tests like that, especially since there was not even a comment saying _why_ you could not call set_page_dirty() on a compound page. I see it in the commit text, but nobody looking at fs/bio.c or fs/direct-io.c would directly find any reference of that.

Quoted 1 line

Could you submit a patch removing the tests?

Okay, I'll do that - once I've reminded myself how to test it and done so!