[PATCH] filemap : fix the wrong offset

[PATCH] filemap : fix the wrong offset

[Huang Shijie]

The offset should be in PAGE_CACHE_SHIFT unit, not in PAGE_SHIFT unit.

Though we do not fully implement the page cache in larger chunks,
but in the vma_address(), all the pages do the (PAGE_CACHE_SHIFT - PAGE_SHIFT) shift,
so do a reverse operation in filemap_fault() is needed.

Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
 mm/filemap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index ef169f3..2d8385e 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1502,7 +1502,7 @@ int filemap_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	struct address_space *mapping = file->f_mapping;
 	struct file_ra_state *ra = &file->f_ra;
 	struct inode *inode = mapping->host;
-	pgoff_t offset = vmf->pgoff;
+	pgoff_t offset = vmf->pgoff >> (PAGE_CACHE_SHIFT - PAGE_SHIFT);
 	struct page *page;
 	pgoff_t size;
 	int ret = 0;
-- 
1.6.0.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] filemap : fix the wrong offset

[Hugh Dickins]

On Tue, 29 Sep 2009, Huang Shijie wrote:

> The offset should be in PAGE_CACHE_SHIFT unit, not in PAGE_SHIFT unit.
> 
> Though we do not fully implement the page cache in larger chunks,
> but in the vma_address(), all the pages do the (PAGE_CACHE_SHIFT - PAGE_SHIFT) shift,
> so do a reverse operation in filemap_fault() is needed.
> 
> Signed-off-by: Huang Shijie <shijie8@gmail.com>

Well, I expect you're right, and I won't object to this patch,
so long as you don't follow it up with a stream of like patches.

I really think this issue is better ignored.  There was a time,
seven years ago, when I cared about it, and made such corrections
in mm/shmem.c.  But we're chipping away at the tip of the iceberg
here, and it's just a waste of everybody's time for so long as
PAGE_CACHE_SIZE == PAGE_SIZE.

There have been patches experimenting with PAGE_CACHE_SIZE multiple
of PAGE_SIZE (and probably not PAGE_SIZE multiple of PAGE_CACHE_SIZE);
and I've come to the conclusion that the only sensible place for these
PAGE_CACHE_SHIFT - PAGE_SHIFT patches is in a patch which really makes
that difference.

I wish PAGE_CACHE_SIZE had never been added in the first place,
long before it was needed; but ripping it out doesn't seem quite
the right thing to do either; and likewise I leave a smattering of
PAGE_CACHE_SHIFT - PAGE_SHIFT lines in, just to remind us from time
to time that there might one day be a difference.

I know this is a very unsatisfying response: but you and I
and everyone else have better things to spend our time on.
Thinking about the difference between two things that are
always the same is rather a waste of mental energy.

Patches to make them different: that's a very different matter.
Andrew happened not to like Christoph Lameter's patches for that;
but certainly those patches were making a real difference, and
deserved all their (PAGE_CACHE_SHIFT - PAGE_SHIFT)s (if they
had any: perhaps tucked away in a conversion macro, I forget).

Hugh

> ---
>  mm/filemap.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index ef169f3..2d8385e 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1502,7 +1502,7 @@ int filemap_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>  	struct address_space *mapping = file->f_mapping;
>  	struct file_ra_state *ra = &file->f_ra;
>  	struct inode *inode = mapping->host;
> -	pgoff_t offset = vmf->pgoff;
> +	pgoff_t offset = vmf->pgoff >> (PAGE_CACHE_SHIFT - PAGE_SHIFT);
>  	struct page *page;
>  	pgoff_t size;
>  	int ret = 0;
> -- 
> 1.6.0.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] filemap : fix the wrong offset

[Huang Shijie]

Hugh Dickins wrote:
> I really think this issue is better ignored.  There was a time,
> seven years ago, when I cared about it, and made such corrections
> in mm/shmem.c.  But we're chipping away at the tip of the iceberg
> here, and it's just a waste of everybody's time for so long as
> PAGE_CACHE_SIZE == PAGE_SIZE.
>
> There have been patches experimenting with PAGE_CACHE_SIZE multiple
> of PAGE_SIZE (and probably not PAGE_SIZE multiple of PAGE_CACHE_SIZE);
> and I've come to the conclusion that the only sensible place for these
> PAGE_CACHE_SHIFT - PAGE_SHIFT patches is in a patch which really makes
> that difference.
>
> I wish PAGE_CACHE_SIZE had never been added in the first place,
> long before it was needed; but ripping it out doesn't seem quite
> the right thing to do either; and likewise I leave a smattering of
> PAGE_CACHE_SHIFT - PAGE_SHIFT lines in, just to remind us from time
> to time that there might one day be a difference.
>
>   
Ok. I get it.

But the filemap_fault()  looks  strange. Some functions such as 
do_sync_mmap_readahead() treat offset
in the PAGE_CAHE_SHIFT unit,though offset is actually in the PAGE_SHIFT 
unit.
> I know this is a very unsatisfying response: but you and I
> and everyone else have better things to spend our time on.
> Thinking about the difference between two things that are
> always the same is rather a waste of mental energy.
>   
Thanks a lot for your explanation.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] filemap : fix the wrong offset

[Hugh Dickins]

On Wed, 30 Sep 2009, Huang Shijie wrote:
> 
> But the filemap_fault()  looks  strange. Some functions such as
> do_sync_mmap_readahead() treat offset
> in the PAGE_CAHE_SHIFT unit,though offset is actually in the PAGE_SHIFT unit.

If this one stands out to you as a puzzling place where it's important
to acknowledge the possibility of the difference, please don't take what
I said too seriously: go ahead and send Andrew the patch again, perhaps
adding a comment that this wouldn't be the only place that's wrong.

Or do you think this is the only place?  I'd be surprised, but
haven't checked: perhaps we have cleaned others up down the years.

I just don't want a long trickle of such patches here and there.
And there's not a lot of point in getting everywhere fixed up,
because people are sure to add patches later which get it wrong again.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>