Re: [PATCH] Check for compound pages in set_page_dirty()
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. - 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/
Re: [PATCH] Check for compound pages in set_page_dirty()
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. - 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/
Re: [PATCH] Check for compound pages in set_page_dirty()
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 - 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/
Re: [PATCH] Check for compound pages in set_page_dirty()
On Mon, 23 Jul 2007, Christoph Lameter wrote: > 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? Hugh - 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/
Re: [PATCH] Check for compound pages in set_page_dirty()
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? - 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/
Re: [PATCH] Check for compound pages in set_page_dirty()
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 - 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/
Re: [PATCH] Check for compound pages in set_page_dirty()
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 <[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. > > > > 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
Re: [PATCH] Check for compound pages in set_page_dirty()
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 <[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. > > > > 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 - 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/
Re: [PATCH] Check for compound pages in set_page_dirty()
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 <[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.
>
> 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)) {
Re: [PATCH] Check for compound pages in set_page_dirty()
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 <[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.
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 [EMAIL PROTECTED]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
-
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/
Re: [PATCH] Check for compound pages in set_page_dirty()
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 - 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/
Re: [PATCH] Check for compound pages in set_page_dirty()
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 - 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/
Re: [PATCH] Check for compound pages in set_page_dirty()
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 <[EMAIL PROTECTED]>
> > >
> > > 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 <[EMAIL PROTECTED]>
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_n
Re: [PATCH] Check for compound pages in set_page_dirty()
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 <[EMAIL PROTECTED]>
> >
> > 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
-
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/
Re: [PATCH] Check for compound pages in set_page_dirty()
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 <[EMAIL PROTECTED]>
>
> 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
-
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/
[PATCH] Check for compound pages in set_page_dirty()
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 <[EMAIL PROTECTED]>
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
-
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/
