Review: Don't use kmap() in xfs_iozero().
kmap is inefficient and does scale well. kmap_atomic() is a better choice. Use the generic wrapper function instead of open coding the kmap-memset-dcache flush-kumap stuff. Suggested by Andrew Morton.
Comments?
Cheers,
Dave.
Signature · 47 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
---
fs/xfs/linux-2.6/xfs_lrw.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_lrw.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/linux-2.6/xfs_lrw.c 2007-01-31 13:56:12.000000000 +1100
+++ 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_lrw.c 2007-01-31 14:19:50.379179841 +1100
@@ -138,7 +138,6 @@ xfs_iozero(
unsigned bytes;
struct page *page;
struct address_space *mapping;
- char *kaddr;
int status;
mapping = ip->i_mapping;
@@ -156,15 +155,13 @@ xfs_iozero(
if (!page)
break;
- kaddr = kmap(page);
status = mapping->a_ops->prepare_write(NULL, page, offset,
offset + bytes);
- if (status) {
+ if (status)
goto unlock;
- }
- memset((void *) (kaddr + offset), 0, bytes);
- flush_dcache_page(page);
+ memclear_highpage_flush(page, (unsigned int)offset, bytes);
+
status = mapping->a_ops->commit_write(NULL, page, offset,
offset + bytes);
if (!status) {
@@ -175,7 +172,6 @@ xfs_iozero(
}
unlock:
- kunmap(page);
unlock_page(page);
page_cache_release(page);
if (status)Quoted 5 lines · David Chinner
kmap is inefficient and does scale well.
kmap_atomic()is a better choice. Use the generic wrapper function instead of open coding the kmap-memset-dcache flush-kumap stuff. Suggested by Andrew Morton.Comments?
Looks good.
Review
Quoted 1 line
(quoted diff) +1
-0+ memclear_highpage_flush(page, (unsigned int)offset, bytes);
Do you need the cast here? An unsigned long should be automatically demoted to an unsigned int when passing it as an argument.
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · David Chinner
kmap is inefficient and does scale well.
kmap_atomic()is a better choice. Use the generic wrapper function instead of open coding the kmap-memset-dcache flush-kumap stuff. Suggested by Andrew Morton.
Quoted 8 lines
Quoted 1 line
Comments?
Looks good.
Review
Quoted 1 line
(quoted diff) +1
-0+ memclear_highpage_flush(page, (unsigned int)offset, bytes);Do you need the cast here? An unsigned long should be automatically demoted to an unsigned int when passing it as an argument.
Even on 64 bit platforms? I just added an explicit cast as a matter of avoiding potential gcc warnings on other platforms/compiler versions. Maybe I'm just being paranoid and I can remove it?
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 3 lines · David Chinner
Even on 64 bit platforms? I just added an explicit cast as a matter of avoiding potential gcc warnings on other platforms/compiler versions. Maybe I'm just being paranoid and I can remove it?
Yes, it's fine on 64bit platforms aswell - I just built my equivalent buffer.c changes on 64bit powerpc and we have lots of similar cases all over the tree.