Review: Don't use kmap() in xfs_iozero().

Review: Don't use kmap() in xfs_iozero().

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

Cheers,

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

Re: Review: Don't use kmap() in xfs_iozero().

[Christoph Hellwig]

On Fri, Feb 02, 2007 at 04:40:44PM +1100, David Chinner wrote:
> 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.

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

Re: Review: Don't use kmap() in xfs_iozero().

[David Chinner]

On Fri, Feb 02, 2007 at 11:48:30AM +0000, Christoph Hellwig wrote:
> On Fri, Feb 02, 2007 at 04:40:44PM +1100, David Chinner wrote:
> > 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.
> 
> > +		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.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: Review: Don't use kmap() in xfs_iozero().

[Christoph Hellwig]

On Sat, Feb 03, 2007 at 12:39:41AM +1100, David Chinner wrote:
> 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.