Re: [PATCH 1/13] fs: convert core functions to zero_user_page

2007-04-11 Thread Jörn Engel
On Tue, 10 April 2007 22:56:38 -0700, Andrew Morton wrote:
> 
> And I'm surprised that this:
> 
> +static inline void memclear_highpage_flush(struct page *page, unsigned int 
> offset, unsigned int size)
> +{
> + return zero_user_page(page, offset, size);
> +}
> 
> compiled.  zero_user_page() returns void...

As does memclear_highpage_flush().  Some of my code looks like:
void some_func(...)
{
if (foo)
return do_foo(...);
if (bar)
return do_bar(...);
...
}

do_foo() and do_bar() also return void.  Saves an extra line for the
return statment and the brackets.

Doesn't help in the code you quoted, of course.

Jörn

-- 
Measure. Don't tune for speed until you've measured, and even then
don't unless one part of the code overwhelms the rest.
-- Rob Pike
-
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 1/13] fs: convert core functions to zero_user_page

2007-04-11 Thread Nate Diller

On 4/10/07, Andrew Morton <[EMAIL PROTECTED]> wrote:

On Tue, 10 Apr 2007 20:36:00 -0700 Nate Diller <[EMAIL PROTECTED]> wrote:

> It's very common for file systems to need to zero part or all of a page, the
> simplist way is just to use kmap_atomic() and memset().  There's actually a
> library function in include/linux/highmem.h that does exactly that, but it's
> confusingly named memclear_highpage_flush(), which is descriptive of *how*
> it does the work rather than what the *purpose* is.  So this patchset
> renames the function to zero_user_page(), and calls it from the various
> places that currently open code it.
>
> This first patch introduces the new function call, and converts all the core
> kernel callsites, both the open-coded ones and the old
> memclear_highpage_flush() ones.  Following this patch is a series of
> conversions for each file system individually, per AKPM, and finally a patch
> deprecating the old call.

For the reasons Anton identified, I think it is better design while we're here
to force callers to pass in the kmap-type which they wish to use for the atomic
kmap.  It makes the programmer think about what he wants to happen.  The price
of getting this wrong tends to be revoltingly rare file corruption.


yeah, I actually agree with you, on thinking about it.  Thanks for
doing the conversion :)


But we cannot make this change in the obvious fashion, because the KM_FOO
identifiers are undefined if CONFIG_HIGHMEM=n.  So

zero_user_page(page, 1, 2, KM_USER0);

won't compile on non-highmem.

So we are forced to use a macro, like below.

Also, you forgot to mark memclear_highpage_flush() __deprecated.


that follows in a later patch ... for some reason I had trouble
compiling using your notation, and i had to add a function prototype
with the __deprecated flag. shrug.



And I'm surprised that this:

+static inline void memclear_highpage_flush(struct page *page, unsigned int 
offset, unsigned int size)
+{
+   return zero_user_page(page, offset, size);
+}

compiled.  zero_user_page() returns void...


it's funny, it didn't even warn about it.  also it seems your version
below is incomplete ... shouldn't it read:

+static inline void memclear_highpage_flush(struct page *page,
+   unsigned int offset, unsigned int size) __deprecated
{
-   return zero_user_page(page, offset, size);
+   zero_user_page(page, offset, size, KM_USER0);
}

NATE



 drivers/block/loop.c|2 +-
 fs/buffer.c |   21 -
 fs/direct-io.c  |2 +-
 fs/mpage.c  |6 --
 include/linux/highmem.h |   29 +
 mm/filemap_xip.c|2 +-
 6 files changed, 36 insertions(+), 26 deletions(-)

diff -puN 
drivers/block/loop.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type 
drivers/block/loop.c
--- 
a/drivers/block/loop.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type
+++ a/drivers/block/loop.c
@@ -250,7 +250,7 @@ static int do_lo_send_aops(struct loop_d
 */
printk(KERN_ERR "loop: transfer error block %llu\n",
   (unsigned long long)index);
-   zero_user_page(page, offset, size);
+   zero_user_page(page, offset, size, KM_USER0);
}
flush_dcache_page(page);
ret = aops->commit_write(file, page, offset,
diff -puN 
fs/buffer.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type 
fs/buffer.c
--- a/fs/buffer.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type
+++ a/fs/buffer.c
@@ -1855,7 +1855,7 @@ static int __block_prepare_write(struct
break;
if (buffer_new(bh)) {
clear_buffer_new(bh);
-   zero_user_page(page, block_start, bh->b_size);
+   zero_user_page(page, block_start, bh->b_size, KM_USER0);
set_buffer_uptodate(bh);
mark_buffer_dirty(bh);
}
@@ -1943,7 +1943,8 @@ int block_read_full_page(struct page *pa
SetPageError(page);
}
if (!buffer_mapped(bh)) {
-   zero_user_page(page, i * blocksize, blocksize);
+   zero_user_page(page, i * blocksize, blocksize,
+   KM_USER0);
if (!err)
set_buffer_uptodate(bh);
continue;
@@ -2107,7 +2108,8 @@ int cont_prepare_write(struct page *page
PAGE_CACHE_SIZE, get_block);
if (status)
goto out_unmap;
-   zero_user_page(page, zerofrom, PAGE_CACHE_SIZE-zerofrom);
+   zero_user_page(page, zerofrom, 

Re: [PATCH 1/13] fs: convert core functions to zero_user_page

2007-04-11 Thread Nate Diller

On 4/10/07, Andrew Morton [EMAIL PROTECTED] wrote:

On Tue, 10 Apr 2007 20:36:00 -0700 Nate Diller [EMAIL PROTECTED] wrote:

 It's very common for file systems to need to zero part or all of a page, the
 simplist way is just to use kmap_atomic() and memset().  There's actually a
 library function in include/linux/highmem.h that does exactly that, but it's
 confusingly named memclear_highpage_flush(), which is descriptive of *how*
 it does the work rather than what the *purpose* is.  So this patchset
 renames the function to zero_user_page(), and calls it from the various
 places that currently open code it.

 This first patch introduces the new function call, and converts all the core
 kernel callsites, both the open-coded ones and the old
 memclear_highpage_flush() ones.  Following this patch is a series of
 conversions for each file system individually, per AKPM, and finally a patch
 deprecating the old call.

For the reasons Anton identified, I think it is better design while we're here
to force callers to pass in the kmap-type which they wish to use for the atomic
kmap.  It makes the programmer think about what he wants to happen.  The price
of getting this wrong tends to be revoltingly rare file corruption.


yeah, I actually agree with you, on thinking about it.  Thanks for
doing the conversion :)


But we cannot make this change in the obvious fashion, because the KM_FOO
identifiers are undefined if CONFIG_HIGHMEM=n.  So

zero_user_page(page, 1, 2, KM_USER0);

won't compile on non-highmem.

So we are forced to use a macro, like below.

Also, you forgot to mark memclear_highpage_flush() __deprecated.


that follows in a later patch ... for some reason I had trouble
compiling using your notation, and i had to add a function prototype
with the __deprecated flag. shrug.



And I'm surprised that this:

+static inline void memclear_highpage_flush(struct page *page, unsigned int 
offset, unsigned int size)
+{
+   return zero_user_page(page, offset, size);
+}

compiled.  zero_user_page() returns void...


it's funny, it didn't even warn about it.  also it seems your version
below is incomplete ... shouldn't it read:

+static inline void memclear_highpage_flush(struct page *page,
+   unsigned int offset, unsigned int size) __deprecated
{
-   return zero_user_page(page, offset, size);
+   zero_user_page(page, offset, size, KM_USER0);
}

NATE



 drivers/block/loop.c|2 +-
 fs/buffer.c |   21 -
 fs/direct-io.c  |2 +-
 fs/mpage.c  |6 --
 include/linux/highmem.h |   29 +
 mm/filemap_xip.c|2 +-
 6 files changed, 36 insertions(+), 26 deletions(-)

diff -puN 
drivers/block/loop.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type 
drivers/block/loop.c
--- 
a/drivers/block/loop.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type
+++ a/drivers/block/loop.c
@@ -250,7 +250,7 @@ static int do_lo_send_aops(struct loop_d
 */
printk(KERN_ERR loop: transfer error block %llu\n,
   (unsigned long long)index);
-   zero_user_page(page, offset, size);
+   zero_user_page(page, offset, size, KM_USER0);
}
flush_dcache_page(page);
ret = aops-commit_write(file, page, offset,
diff -puN 
fs/buffer.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type 
fs/buffer.c
--- a/fs/buffer.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type
+++ a/fs/buffer.c
@@ -1855,7 +1855,7 @@ static int __block_prepare_write(struct
break;
if (buffer_new(bh)) {
clear_buffer_new(bh);
-   zero_user_page(page, block_start, bh-b_size);
+   zero_user_page(page, block_start, bh-b_size, KM_USER0);
set_buffer_uptodate(bh);
mark_buffer_dirty(bh);
}
@@ -1943,7 +1943,8 @@ int block_read_full_page(struct page *pa
SetPageError(page);
}
if (!buffer_mapped(bh)) {
-   zero_user_page(page, i * blocksize, blocksize);
+   zero_user_page(page, i * blocksize, blocksize,
+   KM_USER0);
if (!err)
set_buffer_uptodate(bh);
continue;
@@ -2107,7 +2108,8 @@ int cont_prepare_write(struct page *page
PAGE_CACHE_SIZE, get_block);
if (status)
goto out_unmap;
-   zero_user_page(page, zerofrom, PAGE_CACHE_SIZE-zerofrom);
+   zero_user_page(page, zerofrom, PAGE_CACHE_SIZE - zerofrom,
+  

Re: [PATCH 1/13] fs: convert core functions to zero_user_page

2007-04-11 Thread Jörn Engel
On Tue, 10 April 2007 22:56:38 -0700, Andrew Morton wrote:
 
 And I'm surprised that this:
 
 +static inline void memclear_highpage_flush(struct page *page, unsigned int 
 offset, unsigned int size)
 +{
 + return zero_user_page(page, offset, size);
 +}
 
 compiled.  zero_user_page() returns void...

As does memclear_highpage_flush().  Some of my code looks like:
void some_func(...)
{
if (foo)
return do_foo(...);
if (bar)
return do_bar(...);
...
}

do_foo() and do_bar() also return void.  Saves an extra line for the
return statment and the brackets.

Doesn't help in the code you quoted, of course.

Jörn

-- 
Measure. Don't tune for speed until you've measured, and even then
don't unless one part of the code overwhelms the rest.
-- Rob Pike
-
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 1/13] fs: convert core functions to zero_user_page

2007-04-10 Thread Andrew Morton
On Tue, 10 Apr 2007 20:36:00 -0700 Nate Diller <[EMAIL PROTECTED]> wrote:

> It's very common for file systems to need to zero part or all of a page, the
> simplist way is just to use kmap_atomic() and memset().  There's actually a
> library function in include/linux/highmem.h that does exactly that, but it's
> confusingly named memclear_highpage_flush(), which is descriptive of *how*
> it does the work rather than what the *purpose* is.  So this patchset
> renames the function to zero_user_page(), and calls it from the various
> places that currently open code it.
> 
> This first patch introduces the new function call, and converts all the core
> kernel callsites, both the open-coded ones and the old
> memclear_highpage_flush() ones.  Following this patch is a series of
> conversions for each file system individually, per AKPM, and finally a patch
> deprecating the old call.

For the reasons Anton identified, I think it is better design while we're here
to force callers to pass in the kmap-type which they wish to use for the atomic
kmap.  It makes the programmer think about what he wants to happen.  The price
of getting this wrong tends to be revoltingly rare file corruption.

But we cannot make this change in the obvious fashion, because the KM_FOO
identifiers are undefined if CONFIG_HIGHMEM=n.  So

zero_user_page(page, 1, 2, KM_USER0);

won't compile on non-highmem.

So we are forced to use a macro, like below.

Also, you forgot to mark memclear_highpage_flush() __deprecated.

And I'm surprised that this:

+static inline void memclear_highpage_flush(struct page *page, unsigned int 
offset, unsigned int size)
+{
+   return zero_user_page(page, offset, size);
+}

compiled.  zero_user_page() returns void...


 drivers/block/loop.c|2 +-
 fs/buffer.c |   21 -
 fs/direct-io.c  |2 +-
 fs/mpage.c  |6 --
 include/linux/highmem.h |   29 +
 mm/filemap_xip.c|2 +-
 6 files changed, 36 insertions(+), 26 deletions(-)

diff -puN 
drivers/block/loop.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type 
drivers/block/loop.c
--- 
a/drivers/block/loop.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type
+++ a/drivers/block/loop.c
@@ -250,7 +250,7 @@ static int do_lo_send_aops(struct loop_d
 */
printk(KERN_ERR "loop: transfer error block %llu\n",
   (unsigned long long)index);
-   zero_user_page(page, offset, size);
+   zero_user_page(page, offset, size, KM_USER0);
}
flush_dcache_page(page);
ret = aops->commit_write(file, page, offset,
diff -puN 
fs/buffer.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type 
fs/buffer.c
--- a/fs/buffer.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type
+++ a/fs/buffer.c
@@ -1855,7 +1855,7 @@ static int __block_prepare_write(struct 
break;
if (buffer_new(bh)) {
clear_buffer_new(bh);
-   zero_user_page(page, block_start, bh->b_size);
+   zero_user_page(page, block_start, bh->b_size, KM_USER0);
set_buffer_uptodate(bh);
mark_buffer_dirty(bh);
}
@@ -1943,7 +1943,8 @@ int block_read_full_page(struct page *pa
SetPageError(page);
}
if (!buffer_mapped(bh)) {
-   zero_user_page(page, i * blocksize, blocksize);
+   zero_user_page(page, i * blocksize, blocksize,
+   KM_USER0);
if (!err)
set_buffer_uptodate(bh);
continue;
@@ -2107,7 +2108,8 @@ int cont_prepare_write(struct page *page
PAGE_CACHE_SIZE, get_block);
if (status)
goto out_unmap;
-   zero_user_page(page, zerofrom, PAGE_CACHE_SIZE-zerofrom);
+   zero_user_page(page, zerofrom, PAGE_CACHE_SIZE - zerofrom,
+   KM_USER0);
generic_commit_write(NULL, new_page, zerofrom, PAGE_CACHE_SIZE);
unlock_page(new_page);
page_cache_release(new_page);
@@ -2134,7 +2136,7 @@ int cont_prepare_write(struct page *page
if (status)
goto out1;
if (zerofrom < offset) {
-   zero_user_page(page, zerofrom, offset-zerofrom);
+   zero_user_page(page, zerofrom, offset - zerofrom, KM_USER0);
__block_commit_write(inode, page, zerofrom, offset);
}
return 0;
@@ -2333,7 +2335,7 @@ failed:
 * Error recovery is pretty slack.  Clear the 

[PATCH 1/13] fs: convert core functions to zero_user_page

2007-04-10 Thread Nate Diller
It's very common for file systems to need to zero part or all of a page, the
simplist way is just to use kmap_atomic() and memset().  There's actually a
library function in include/linux/highmem.h that does exactly that, but it's
confusingly named memclear_highpage_flush(), which is descriptive of *how*
it does the work rather than what the *purpose* is.  So this patchset
renames the function to zero_user_page(), and calls it from the various
places that currently open code it.

This first patch introduces the new function call, and converts all the core
kernel callsites, both the open-coded ones and the old
memclear_highpage_flush() ones.  Following this patch is a series of
conversions for each file system individually, per AKPM, and finally a patch
deprecating the old call.  The diffstat below shows the entire patchset.

Compile tested in x86_64.

signed-off-by: Nate Diller <[EMAIL PROTECTED]>

---

 drivers/block/loop.c |6 ---
 fs/affs/file.c   |6 ---
 fs/buffer.c  |   53 +--
 fs/direct-io.c   |8 +---
 fs/ecryptfs/mmap.c   |   14 +---
 fs/ext3/inode.c  |   12 +--
 fs/ext4/inode.c  |   12 +--
 fs/ext4/writeback.c  |   12 +--
 fs/gfs2/bmap.c   |6 ---
 fs/mpage.c   |   11 +-
 fs/nfs/read.c|   10 ++---
 fs/nfs/write.c   |2 -
 fs/ntfs/aops.c   |   26 ++-
 fs/ntfs/file.c   |   47 +--
 fs/ocfs2/aops.c  |5 --
 fs/reiser4/plugin/file/cryptcompress.c   |   19 +--
 fs/reiser4/plugin/file/file.c|6 ---
 fs/reiser4/plugin/item/ctail.c   |6 ---
 fs/reiser4/plugin/item/extent_file_ops.c |   19 +++
 fs/reiser4/plugin/item/tail.c|8 +---
 fs/reiserfs/file.c   |   39 ++
 fs/reiserfs/inode.c  |   13 +--
 fs/xfs/linux-2.6/xfs_lrw.c   |2 -
 include/linux/highmem.h  |7 +++-
 mm/filemap_xip.c |7 
 mm/truncate.c|2 -
 26 files changed, 82 insertions(+), 276 deletions(-)

---

diff -urpN -X dontdiff linux-2.6.21-rc6-mm1/drivers/block/loop.c 
linux-2.6.21-rc6-mm1-test/drivers/block/loop.c
--- linux-2.6.21-rc6-mm1/drivers/block/loop.c   2007-04-10 18:27:04.0 
-0700
+++ linux-2.6.21-rc6-mm1-test/drivers/block/loop.c  2007-04-10 
18:18:16.0 -0700
@@ -244,17 +244,13 @@ static int do_lo_send_aops(struct loop_d
transfer_result = lo_do_transfer(lo, WRITE, page, offset,
bvec->bv_page, bv_offs, size, IV);
if (unlikely(transfer_result)) {
-   char *kaddr;
-
/*
 * The transfer failed, but we still write the data to
 * keep prepare/commit calls balanced.
 */
printk(KERN_ERR "loop: transfer error block %llu\n",
   (unsigned long long)index);
-   kaddr = kmap_atomic(page, KM_USER0);
-   memset(kaddr + offset, 0, size);
-   kunmap_atomic(kaddr, KM_USER0);
+   zero_user_page(page, offset, size);
}
flush_dcache_page(page);
ret = aops->commit_write(file, page, offset,
diff -urpN -X dontdiff linux-2.6.21-rc6-mm1/fs/buffer.c 
linux-2.6.21-rc6-mm1-test/fs/buffer.c
--- linux-2.6.21-rc6-mm1/fs/buffer.c2007-04-10 18:27:04.0 -0700
+++ linux-2.6.21-rc6-mm1-test/fs/buffer.c   2007-04-10 18:18:16.0 
-0700
@@ -1862,13 +1862,8 @@ static int __block_prepare_write(struct 
if (block_start >= to)
break;
if (buffer_new(bh)) {
-   void *kaddr;
-
clear_buffer_new(bh);
-   kaddr = kmap_atomic(page, KM_USER0);
-   memset(kaddr+block_start, 0, bh->b_size);
-   flush_dcache_page(page);
-   kunmap_atomic(kaddr, KM_USER0);
+   zero_user_page(page, block_start, bh->b_size);
set_buffer_uptodate(bh);
mark_buffer_dirty(bh);
}
@@ -1956,10 +1951,7 @@ int block_read_full_page(struct page *pa
SetPageError(page);
}
if (!buffer_mapped(bh)) {
-   void *kaddr = kmap_atomic(page, KM_USER0);
-   memset(kaddr + 

[PATCH 1/13] fs: convert core functions to zero_user_page

2007-04-10 Thread Nate Diller
It's very common for file systems to need to zero part or all of a page, the
simplist way is just to use kmap_atomic() and memset().  There's actually a
library function in include/linux/highmem.h that does exactly that, but it's
confusingly named memclear_highpage_flush(), which is descriptive of *how*
it does the work rather than what the *purpose* is.  So this patchset
renames the function to zero_user_page(), and calls it from the various
places that currently open code it.

This first patch introduces the new function call, and converts all the core
kernel callsites, both the open-coded ones and the old
memclear_highpage_flush() ones.  Following this patch is a series of
conversions for each file system individually, per AKPM, and finally a patch
deprecating the old call.  The diffstat below shows the entire patchset.

Compile tested in x86_64.

signed-off-by: Nate Diller [EMAIL PROTECTED]

---

 drivers/block/loop.c |6 ---
 fs/affs/file.c   |6 ---
 fs/buffer.c  |   53 +--
 fs/direct-io.c   |8 +---
 fs/ecryptfs/mmap.c   |   14 +---
 fs/ext3/inode.c  |   12 +--
 fs/ext4/inode.c  |   12 +--
 fs/ext4/writeback.c  |   12 +--
 fs/gfs2/bmap.c   |6 ---
 fs/mpage.c   |   11 +-
 fs/nfs/read.c|   10 ++---
 fs/nfs/write.c   |2 -
 fs/ntfs/aops.c   |   26 ++-
 fs/ntfs/file.c   |   47 +--
 fs/ocfs2/aops.c  |5 --
 fs/reiser4/plugin/file/cryptcompress.c   |   19 +--
 fs/reiser4/plugin/file/file.c|6 ---
 fs/reiser4/plugin/item/ctail.c   |6 ---
 fs/reiser4/plugin/item/extent_file_ops.c |   19 +++
 fs/reiser4/plugin/item/tail.c|8 +---
 fs/reiserfs/file.c   |   39 ++
 fs/reiserfs/inode.c  |   13 +--
 fs/xfs/linux-2.6/xfs_lrw.c   |2 -
 include/linux/highmem.h  |7 +++-
 mm/filemap_xip.c |7 
 mm/truncate.c|2 -
 26 files changed, 82 insertions(+), 276 deletions(-)

---

diff -urpN -X dontdiff linux-2.6.21-rc6-mm1/drivers/block/loop.c 
linux-2.6.21-rc6-mm1-test/drivers/block/loop.c
--- linux-2.6.21-rc6-mm1/drivers/block/loop.c   2007-04-10 18:27:04.0 
-0700
+++ linux-2.6.21-rc6-mm1-test/drivers/block/loop.c  2007-04-10 
18:18:16.0 -0700
@@ -244,17 +244,13 @@ static int do_lo_send_aops(struct loop_d
transfer_result = lo_do_transfer(lo, WRITE, page, offset,
bvec-bv_page, bv_offs, size, IV);
if (unlikely(transfer_result)) {
-   char *kaddr;
-
/*
 * The transfer failed, but we still write the data to
 * keep prepare/commit calls balanced.
 */
printk(KERN_ERR loop: transfer error block %llu\n,
   (unsigned long long)index);
-   kaddr = kmap_atomic(page, KM_USER0);
-   memset(kaddr + offset, 0, size);
-   kunmap_atomic(kaddr, KM_USER0);
+   zero_user_page(page, offset, size);
}
flush_dcache_page(page);
ret = aops-commit_write(file, page, offset,
diff -urpN -X dontdiff linux-2.6.21-rc6-mm1/fs/buffer.c 
linux-2.6.21-rc6-mm1-test/fs/buffer.c
--- linux-2.6.21-rc6-mm1/fs/buffer.c2007-04-10 18:27:04.0 -0700
+++ linux-2.6.21-rc6-mm1-test/fs/buffer.c   2007-04-10 18:18:16.0 
-0700
@@ -1862,13 +1862,8 @@ static int __block_prepare_write(struct 
if (block_start = to)
break;
if (buffer_new(bh)) {
-   void *kaddr;
-
clear_buffer_new(bh);
-   kaddr = kmap_atomic(page, KM_USER0);
-   memset(kaddr+block_start, 0, bh-b_size);
-   flush_dcache_page(page);
-   kunmap_atomic(kaddr, KM_USER0);
+   zero_user_page(page, block_start, bh-b_size);
set_buffer_uptodate(bh);
mark_buffer_dirty(bh);
}
@@ -1956,10 +1951,7 @@ int block_read_full_page(struct page *pa
SetPageError(page);
}
if (!buffer_mapped(bh)) {
-   void *kaddr = kmap_atomic(page, KM_USER0);
-   memset(kaddr + i * 

Re: [PATCH 1/13] fs: convert core functions to zero_user_page

2007-04-10 Thread Andrew Morton
On Tue, 10 Apr 2007 20:36:00 -0700 Nate Diller [EMAIL PROTECTED] wrote:

 It's very common for file systems to need to zero part or all of a page, the
 simplist way is just to use kmap_atomic() and memset().  There's actually a
 library function in include/linux/highmem.h that does exactly that, but it's
 confusingly named memclear_highpage_flush(), which is descriptive of *how*
 it does the work rather than what the *purpose* is.  So this patchset
 renames the function to zero_user_page(), and calls it from the various
 places that currently open code it.
 
 This first patch introduces the new function call, and converts all the core
 kernel callsites, both the open-coded ones and the old
 memclear_highpage_flush() ones.  Following this patch is a series of
 conversions for each file system individually, per AKPM, and finally a patch
 deprecating the old call.

For the reasons Anton identified, I think it is better design while we're here
to force callers to pass in the kmap-type which they wish to use for the atomic
kmap.  It makes the programmer think about what he wants to happen.  The price
of getting this wrong tends to be revoltingly rare file corruption.

But we cannot make this change in the obvious fashion, because the KM_FOO
identifiers are undefined if CONFIG_HIGHMEM=n.  So

zero_user_page(page, 1, 2, KM_USER0);

won't compile on non-highmem.

So we are forced to use a macro, like below.

Also, you forgot to mark memclear_highpage_flush() __deprecated.

And I'm surprised that this:

+static inline void memclear_highpage_flush(struct page *page, unsigned int 
offset, unsigned int size)
+{
+   return zero_user_page(page, offset, size);
+}

compiled.  zero_user_page() returns void...


 drivers/block/loop.c|2 +-
 fs/buffer.c |   21 -
 fs/direct-io.c  |2 +-
 fs/mpage.c  |6 --
 include/linux/highmem.h |   29 +
 mm/filemap_xip.c|2 +-
 6 files changed, 36 insertions(+), 26 deletions(-)

diff -puN 
drivers/block/loop.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type 
drivers/block/loop.c
--- 
a/drivers/block/loop.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type
+++ a/drivers/block/loop.c
@@ -250,7 +250,7 @@ static int do_lo_send_aops(struct loop_d
 */
printk(KERN_ERR loop: transfer error block %llu\n,
   (unsigned long long)index);
-   zero_user_page(page, offset, size);
+   zero_user_page(page, offset, size, KM_USER0);
}
flush_dcache_page(page);
ret = aops-commit_write(file, page, offset,
diff -puN 
fs/buffer.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type 
fs/buffer.c
--- a/fs/buffer.c~fs-convert-core-functions-to-zero_user_page-pass-kmap-type
+++ a/fs/buffer.c
@@ -1855,7 +1855,7 @@ static int __block_prepare_write(struct 
break;
if (buffer_new(bh)) {
clear_buffer_new(bh);
-   zero_user_page(page, block_start, bh-b_size);
+   zero_user_page(page, block_start, bh-b_size, KM_USER0);
set_buffer_uptodate(bh);
mark_buffer_dirty(bh);
}
@@ -1943,7 +1943,8 @@ int block_read_full_page(struct page *pa
SetPageError(page);
}
if (!buffer_mapped(bh)) {
-   zero_user_page(page, i * blocksize, blocksize);
+   zero_user_page(page, i * blocksize, blocksize,
+   KM_USER0);
if (!err)
set_buffer_uptodate(bh);
continue;
@@ -2107,7 +2108,8 @@ int cont_prepare_write(struct page *page
PAGE_CACHE_SIZE, get_block);
if (status)
goto out_unmap;
-   zero_user_page(page, zerofrom, PAGE_CACHE_SIZE-zerofrom);
+   zero_user_page(page, zerofrom, PAGE_CACHE_SIZE - zerofrom,
+   KM_USER0);
generic_commit_write(NULL, new_page, zerofrom, PAGE_CACHE_SIZE);
unlock_page(new_page);
page_cache_release(new_page);
@@ -2134,7 +2136,7 @@ int cont_prepare_write(struct page *page
if (status)
goto out1;
if (zerofrom  offset) {
-   zero_user_page(page, zerofrom, offset-zerofrom);
+   zero_user_page(page, zerofrom, offset - zerofrom, KM_USER0);
__block_commit_write(inode, page, zerofrom, offset);
}
return 0;
@@ -2333,7 +2335,7 @@ failed:
 * Error recovery is pretty slack.  Clear the page and mark it