[PATCH] Check for compound pages in set_page_dirty()

[PATCH] Check for compound pages in set_page_dirty()

[Jens Axboe]

Hi,

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 <jens.axboe@oracle.com>

diff --git a/fs/bio.c b/fs/bio.c
index cd888f9..ff96cd9 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -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);
 	}
 }
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 52bb263..72195bc 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -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);
 		}
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 886ea0d..3c590b9 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -861,8 +861,12 @@ EXPORT_SYMBOL(redirty_page_for_writepage);
  */
 int fastcall set_page_dirty(struct page *page)
 {
-	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

-- 
Jens Axboe

Re: [PATCH] Check for compound pages in set_page_dirty()

[Hugh Dickins]

On Wed, 18 Jul 2007, Jens Axboe wrote:
> 
> 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 <jens.axboe@oracle.com>
> 
> diff --git a/fs/bio.c b/fs/bio.c
> index cd888f9..ff96cd9 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -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);
>  	}
>  }
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 52bb263..72195bc 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -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);
>  		}
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 886ea0d..3c590b9 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -861,8 +861,12 @@ EXPORT_SYMBOL(redirty_page_for_writepage);
>   */
>  int fastcall set_page_dirty(struct page *page)
>  {
> -	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.

Hugh

Re: [PATCH] Check for compound pages in set_page_dirty()

[Jens Axboe]

On Wed, Jul 18 2007, Hugh Dickins wrote:
> On Wed, 18 Jul 2007, Jens Axboe wrote:
> > 
> > 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 <jens.axboe@oracle.com>
> > 
> > diff --git a/fs/bio.c b/fs/bio.c
> > index cd888f9..ff96cd9 100644
> > --- a/fs/bio.c
> > +++ b/fs/bio.c
> > @@ -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);
> >  	}
> >  }
> > diff --git a/fs/direct-io.c b/fs/direct-io.c
> > index 52bb263..72195bc 100644
> > --- a/fs/direct-io.c
> > +++ b/fs/direct-io.c
> > @@ -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);
> >  		}
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index 886ea0d..3c590b9 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -861,8 +861,12 @@ EXPORT_SYMBOL(redirty_page_for_writepage);
> >   */
> >  int fastcall set_page_dirty(struct page *page)
> >  {
> > -	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.

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?

-- 
Jens Axboe

Re: [PATCH] Check for compound pages in set_page_dirty()

[Jens Axboe]

On Wed, Jul 18 2007, Jens Axboe wrote:
> On Wed, Jul 18 2007, Hugh Dickins wrote:
> > On Wed, 18 Jul 2007, Jens Axboe wrote:
> > > 
> > > 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 <jens.axboe@oracle.com>
> > > 
> > > diff --git a/fs/bio.c b/fs/bio.c
> > > index cd888f9..ff96cd9 100644
> > > --- a/fs/bio.c
> > > +++ b/fs/bio.c
> > > @@ -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);
> > >  	}
> > >  }
> > > diff --git a/fs/direct-io.c b/fs/direct-io.c
> > > index 52bb263..72195bc 100644
> > > --- a/fs/direct-io.c
> > > +++ b/fs/direct-io.c
> > > @@ -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);
> > >  		}
> > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > > index 886ea0d..3c590b9 100644
> > > --- a/mm/page-writeback.c
> > > +++ b/mm/page-writeback.c
> > > @@ -861,8 +861,12 @@ EXPORT_SYMBOL(redirty_page_for_writepage);
> > >   */
> > >  int fastcall set_page_dirty(struct page *page)
> > >  {
> > > -	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.
> 
> 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 <jens.axboe@oracle.com>

diff --git a/fs/bio.c b/fs/bio.c
index 33e4634..1e12416 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -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 {
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 52bb263..72195bc 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -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);
 		}

-- 
Jens Axboe

Re: [PATCH] Check for compound pages in set_page_dirty()

[Hugh Dickins]

On Wed, 18 Jul 2007, Jens Axboe wrote:
> 
> 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.)

> 
> ---
> 
> [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 <jens.axboe@oracle.com>

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 <akpm@osdl.org>
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.

diff --git a/fs/bio.c b/fs/bio.c
index d016523..2463163 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -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.

Hugh

> 
> diff --git a/fs/bio.c b/fs/bio.c
> index 33e4634..1e12416 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -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 {
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 52bb263..72195bc 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -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);
>  		}
> 
> -- 
> Jens Axboe
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] Check for compound pages in set_page_dirty()

[Jens Axboe]

On Wed, Jul 18 2007, Hugh Dickins wrote:
> On Wed, 18 Jul 2007, Jens Axboe wrote:
> > 
> > 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.)
> 
> > 
> > ---
> > 
> > [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 <jens.axboe@oracle.com>
> 
> 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 <akpm@osdl.org>
> 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.
> 
> diff --git a/fs/bio.c b/fs/bio.c
> index d016523..2463163 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -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!

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!

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

diff --git a/fs/bio.c b/fs/bio.c
index 33e4634..dcbb160 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -884,12 +884,6 @@ struct bio *bio_map_kern(request_queue_t *q, void *data, unsigned int len,
  * 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.
- *
  * Note that this code is very hard to test under normal circumstances because
  * direct-io pins the pages with get_user_pages().  This makes
  * is_page_cache_freeable return false, and the VM will not clean the pages.
@@ -911,7 +905,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 +972,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 {
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 52bb263..72195bc 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -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);
 		}

-- 
Jens Axboe

Re: [PATCH] Check for compound pages in set_page_dirty()

[Jens Axboe]

On Thu, Jul 19 2007, Jens Axboe wrote:
> On Wed, Jul 18 2007, Hugh Dickins wrote:
> > On Wed, 18 Jul 2007, Jens Axboe wrote:
> > > 
> > > 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.)
> > 
> > > 
> > > ---
> > > 
> > > [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 <jens.axboe@oracle.com>
> > 
> > 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 <akpm@osdl.org>
> > 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.
> > 
> > diff --git a/fs/bio.c b/fs/bio.c
> > index d016523..2463163 100644
> > --- a/fs/bio.c
> > +++ b/fs/bio.c
> > @@ -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!
> 
> 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!
> 
> > 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).

FWIW, I ran a hugepage+O_DIRECT read test, that will cause the direct-io
code to hit set_page_dirty() for a compound page - and it works fine for
me. The fio job file used was:

[global]
directory=/data1
bs=4k
direct=1
hugepage-size=4m
iomem=shmhuge

[iothread]
filename=testfile
size=128m
rw=read

Test box is a lowly x86.

-- 
Jens Axboe

Re: [PATCH] Check for compound pages in set_page_dirty()

[Hugh Dickins]

On Thu, 19 Jul 2007, Jens Axboe wrote:
> On Wed, Jul 18 2007, Hugh Dickins wrote:
> > On Wed, 18 Jul 2007, Jens Axboe wrote:
> > > 
> > > 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.)

I've added several of the hugetlb guys to the CC list, plus CLameter;
but again not yet Trond since the details won't be of much interest
to him; though I have included fs/nfs/direct.c in the patch below,
since it's now looking like a bugfix rather than a cleanup.
I've kept in most of the background text for newcomers.

> > > [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 <jens.axboe@oracle.com>
> > 
> > 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 <akpm@osdl.org>
> > 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.
> > 
> > diff --git a/fs/bio.c b/fs/bio.c
> > index d016523..2463163 100644
> > --- a/fs/bio.c
> > +++ b/fs/bio.c
> > @@ -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!
> 
> Hehe, that didn't really get us much further, did it? :-)

And I've changed my mind several times since then too: from thinking
that the DIO case gives hugetlbfs a lost dirty bug, to thinking that
it's okay after all, to thinking again that yes we do have a bug.

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

That's very unfair to Andrew, who took the trouble to write long and
enlightening comments above the functions.  Let's blame ourselves for
not looking there and reading them.

You won't much like the compound_head(page) I'm putting into
bio_check_pages_dirty, but I think it is being reasonable to
try to avoid scheduling work when that's hardly ever needed
(dirty already set and not cleaned in the meanwhile).

There are enough twists and turns in this area, that I also think
it's as well to keep something here to think about and comment on.

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

Oh, it doesn't take much to confuse me!

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

I believe I've reached my conclusion in the patch below, but definitely
need others to check and test.  I'll be out of contact for a few days
from tomorrow, so better someone else take this up.

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

For a while I thought it wasn't important, because get_user_pages
normally marks the pages dirty at the beginning, and there's no
pdflushing of hugetlb pages to clean them in the meanwhile.

But get_user_pages goes a different route on hugetlb pages, it goes
off to follow_hugetlb_page.  Which doesn't even take an argument to
tell it whether the page is being written to or not.  So far as I
can see, there's no preliminary set_page_dirty in that case (but
haven't checked in practice).  Therefore I now think Ken's case
would rely on bio.c or direct-io.c to set the page dirty after all.

(Side issue not addressed in this patch at all: the hugetlb folks
introduced private hugetlb mappings and hugetlb_cow in 2.6.16: yet
get_user_pages appears not to honour that, which seems a serious
omission: breaking COW is an important duty of get_user_pages.)

I think set_page_dirty just has to act on compound_head(page):
it's only in the head page that page->mapping is set, so set_page_dirty
can't even discover that it ought to go to hugetlbfs_set_page_dirty
without doing the compound_head(page) first.  Which then leaves
the distinct hugetlbfs_set_page_dirty rather pointless: we can
just use the standard __set_page_dirty_no_writeback.

Without that change, even after removing the PageCompound exclusions
from bio.c as you did, I suspect that DIO to a tail page of a
hugetlb page would never get to dirty the head page -
something the libhugetlb guys might like to test for.

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.

Hugh

--- 2.6.22-git12/fs/bio.c	2007-07-19 13:58:42.000000000 +0100
+++ linux/fs/bio.c	2007-07-19 17:05:46.000000000 +0100
@@ -884,12 +884,6 @@ struct bio *bio_map_kern(request_queue_t
  * 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.
- *
  * Note that this code is very hard to test under normal circumstances because
  * direct-io pins the pages with get_user_pages().  This makes
  * is_page_cache_freeable return false, and the VM will not clean the pages.
@@ -911,7 +905,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 +972,12 @@ void bio_check_pages_dirty(struct bio *b
 	for (i = 0; i < bio->bi_vcnt; i++) {
 		struct page *page = bvec[i].bv_page;
 
-		if (PageDirty(page) || PageCompound(page)) {
+		/*
+		 * set_page_dirty(page) sets PageDirty on the head when
+		 * PageCompound: therefore that's where we should look when
+		 * trying to avoid scheduling our dirty work unnecessarily.
+		 */
+		if (PageDirty(compound_head(page))) {
 			page_cache_release(page);
 			bvec[i].bv_page = NULL;
 		} else {
--- 2.6.22-git12/fs/direct-io.c	2007-07-09 00:32:17.000000000 +0100
+++ linux/fs/direct-io.c	2007-07-19 17:05:46.000000000 +0100
@@ -426,7 +426,7 @@ static int dio_bio_complete(struct dio *
 		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);
 		}
--- 2.6.22-git12/fs/hugetlbfs/inode.c	2007-07-19 13:58:43.000000000 +0100
+++ linux/fs/hugetlbfs/inode.c	2007-07-19 17:05:46.000000000 +0100
@@ -470,17 +470,6 @@ static int hugetlbfs_symlink(struct inod
 	return error;
 }
 
-/*
- * mark the head page dirty
- */
-static int hugetlbfs_set_page_dirty(struct page *page)
-{
-	struct page *head = compound_head(page);
-
-	SetPageDirty(head);
-	return 0;
-}
-
 static int hugetlbfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 {
 	struct hugetlbfs_sb_info *sbinfo = HUGETLBFS_SB(dentry->d_sb);
@@ -566,7 +555,7 @@ static const struct address_space_operat
 	.readpage	= hugetlbfs_readpage,
 	.prepare_write	= hugetlbfs_prepare_write,
 	.commit_write	= hugetlbfs_commit_write,
-	.set_page_dirty	= hugetlbfs_set_page_dirty,
+	.set_page_dirty	= __set_page_dirty_no_writeback,
 };
 
 
--- 2.6.22-git12/fs/nfs/direct.c	2007-07-19 13:58:43.000000000 +0100
+++ linux/fs/nfs/direct.c	2007-07-19 17:05:46.000000000 +0100
@@ -133,8 +133,7 @@ static void nfs_direct_dirty_pages(struc
 	npages = (count + (pgbase & ~PAGE_MASK) + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	for (i = 0; i < npages; i++) {
 		struct page *page = pages[i];
-		if (!PageCompound(page))
-			set_page_dirty(page);
+		set_page_dirty(page);
 	}
 }
 
--- 2.6.22-git12/mm/page-writeback.c	2007-07-19 13:58:49.000000000 +0100
+++ linux/mm/page-writeback.c	2007-07-19 17:05:46.000000000 +0100
@@ -861,7 +861,10 @@ EXPORT_SYMBOL(redirty_page_for_writepage
  */
 int fastcall set_page_dirty(struct page *page)
 {
-	struct address_space *mapping = page_mapping(page);
+	struct address_space *mapping;
+
+	page = compound_head(page);
+	mapping = page_mapping(page);
 
 	if (likely(mapping)) {
 		int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;

Re: [PATCH] Check for compound pages in set_page_dirty()

[William Lee Irwin III]

On Thu, Jul 19, 2007 at 06:35:17PM +0100, Hugh Dickins wrote:
> 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?)

This needs to be done some other way. The dirty bit should not be
significant for pseudofs's with no backing store. The consequences
of making it so are becoming apparent in the IO path, and it
caused performance regressions elsewhere as well. ramfs, for instance,
doesn't require anything of this sort to cope with drop_caches.


-- wli

Re: [PATCH] Check for compound pages in set_page_dirty()

[Christoph Lameter]

On Thu, 19 Jul 2007 18:35:17 +0100 (BST)
Hugh Dickins <hugh@veritas.com> 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?

Re: [PATCH] Check for compound pages in set_page_dirty()

[Hugh Dickins]

On Mon, 23 Jul 2007, Christoph Lameter wrote:
> On Thu, 19 Jul 2007 18:35:17 +0100 (BST)
> Hugh Dickins <hugh@veritas.com> 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?

Hugh

Re: [PATCH] Check for compound pages in set_page_dirty()

[Christoph Lameter]

On Thu, 26 Jul 2007, Hugh Dickins wrote:

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

Re: [PATCH] Check for compound pages in set_page_dirty()

[Hugh Dickins]

On Thu, 26 Jul 2007, Christoph Lameter wrote:
> On Thu, 26 Jul 2007, Hugh Dickins wrote:
> 
> > 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.

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

Hugh

Re: [PATCH] Check for compound pages in set_page_dirty()

[Christoph Lameter]

On Thu, 26 Jul 2007, Hugh Dickins wrote:

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

The large blocksize patch currently does not support mmap. I just have 
some patches here that implement some of that using the approach that I 
described.

And without mmap support we never have to use references to tail pages 
anyways. 

We could avoid references to tail pages if we would not allow the mapping 
of 4k subsections of larger pages but instead require that a compound page 
always be mapped in its entirety. That would keep the necessary changes to 
memory.c minimal but would cause trouble for applications that expect to 
be able to map 4k chunks.

If we want to support transparent use of 2M pages then we need to do this 
anyways but at that point we can still have a single large "pte" (well 
really a pmd that we treat as a pte).

If we f.e. require an order 3 page to be mapped in one go then we would 
have to install 8 ptes at once. If we allow mapping of 4k sections that we 
can have mm/memory.c deal with one pte at a time.

Re: [PATCH] Check for compound pages in set_page_dirty()

[Hugh Dickins]

On Wed, 18 Jul 2007, Jens Axboe wrote:
> 
> 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?

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

Hugh

Re: [PATCH] Check for compound pages in set_page_dirty()

[Jens Axboe]

On Wed, Jul 18 2007, Hugh Dickins wrote:
> On Wed, 18 Jul 2007, Jens Axboe wrote:
> > 
> > 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?
> 
> Okay, I'll do that - once I've reminded myself how to test it and done so!

I'll do a test on this tomorrow and let you know what the results are! I
should be able to do a quick fio job file for this, no need to write
test cases :-)

-- 
Jens Axboe