Skip to contentSkip to conversation

xip cleanups

PATCH In review Christoph Hellwig 2 participants 4 messages Jun 28, 2005 – Jun 30, 2005 1 message not in the archive ext2 file system

lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 1/1 xip cleanups

adjust to kernel style and remove some unneeded NULL checks

fs/ext2/xip.c +16 -19
Index: linux-2.6/fs/ext2/xip.c
===================================================================
@@ -15,42 +15,39 @@
 #include "xip.h"
 
 static inline int
-__inode_direct_access(struct inode *inode, sector_t sector, unsigned long *data) {
+__inode_direct_access(struct inode *inode, sector_t sector, unsigned long *data)
+{
 	BUG_ON(!inode->i_sb->s_bdev->bd_disk->fops->direct_access);
 	return inode->i_sb->s_bdev->bd_disk->fops
 		->direct_access(inode->i_sb->s_bdev,sector,data);
 }
 
 int
-ext2_clear_xip_target(struct inode *inode, int block) {
-	sector_t sector = block*(PAGE_SIZE/512);
+ext2_clear_xip_target(struct inode *inode, int block)
+{
+	sector_t sector = block * (PAGE_SIZE/512);
 	unsigned long data;
 	int rc;
 
 	rc = __inode_direct_access(inode, sector, &data);
-	if (rc)
-		return rc;
-	clear_page((void*)data);
-	return 0;
+	if (!rc)
+		clear_page(data);
+	return rc;
 }
 
 void ext2_xip_verify_sb(struct super_block *sb)
 {
 	struct ext2_sb_info *sbi = EXT2_SB(sb);
 
-	if ((sbi->s_mount_opt & EXT2_MOUNT_XIP)) {
-		if ((sb->s_bdev == NULL) ||
-			sb->s_bdev->bd_disk == NULL ||
-			sb->s_bdev->bd_disk->fops == NULL ||
-			sb->s_bdev->bd_disk->fops->direct_access == NULL) {
-			sbi->s_mount_opt &= (~EXT2_MOUNT_XIP);
-			ext2_warning(sb, __FUNCTION__,
-				"ignoring xip option - not supported by bdev");
-		}
+	if ((sbi->s_mount_opt & EXT2_MOUNT_XIP) &&
+	    !sb->s_bdev->bd_disk->fops->direct_access) {
+		sbi->s_mount_opt &= ~EXT2_MOUNT_XIP;
+		ext2_warning(sb, __FUNCTION__,
+			"ignoring xip option - not supported by bdev");
 	}
 }
 
-struct page*
+struct page *
 ext2_get_xip_page(struct address_space *mapping, sector_t blockno,
 		   int create)
 {

@@ -60,7 +57,7 @@
 
 	tmp.b_state = 0;
 	tmp.b_blocknr = 0;
-	rc = ext2_get_block(mapping->host, blockno/(PAGE_SIZE/512) , &tmp,
+	rc = ext2_get_block(mapping->host, blockno/(PAGE_SIZE/512), &tmp,
 				create);
 	if (rc)
 		return ERR_PTR(rc);

@@ -71,7 +68,7 @@
 	}
 
 	rc = __inode_direct_access
-		(mapping->host,tmp.b_blocknr*(PAGE_SIZE/512) ,&data);
+		(mapping->host,tmp.b_blocknr * (PAGE_SIZE/512), &data);
 	if (rc)
 		return ERR_PTR(rc);
#1 lore raw
Quoted 2 lines · Christoph Hellwig

adjust to kernel style and remove some unneeded NULL checks

Cool!

fs/ext2/xip.c line 35

Quoted 8 lines
fs/ext2/xip.c +3 -4
 	rc = __inode_direct_access(inode, sector, &data);
-	if (rc)
-		return rc;
-	clear_page((void*)data);
-	return 0;
+	if (!rc)
+		clear_page(data);
+	return rc;

I personally prefer the original code. As a general rule, error handling code is indented further than regular good-case code. That makes reading a *lot* faster and the compiler should be smart enough to generate identical code.

What are your arguments for the change?

fs/ext2/xip.c line 71

Quoted 2 lines
fs/ext2/xip.c +1 -1
-		(mapping->host,tmp.b_blocknr*(PAGE_SIZE/512) ,&data);
+		(mapping->host,tmp.b_blocknr * (PAGE_SIZE/512), &data);

                               ^

You missed one.

Jörn

Signature · 4 lines
Fancy algorithms are slow when n is small, and n is usually small.
Fancy algorithms have big constants. Until you know that n is
frequently going to be big, don't get fancy.
-- Rob Pike
author
#2 lore raw
Quoted 4 lines · J?rn Engel

I personally prefer the original code. As a general rule, error handling code is indented further than regular good-case code. That makes reading a *lot* faster and the compiler should be smart enough to generate identical code.

ok.. the cast removal is also wrong as Arnd pointed out, btw.

Quoted 4 lines

fs/ext2/xip.c line 71

Quoted 2 lines
fs/ext2/xip.c +1 -1
-		(mapping->host,tmp.b_blocknr*(PAGE_SIZE/512) ,&data);
+		(mapping->host,tmp.b_blocknr * (PAGE_SIZE/512), &data);

                               ^

You missed one.

feel free to add it :)

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#4 lore raw
Quoted 2 lines · Arnd Bergmann

On Dinsdag 28 Juni 2005 14:01, Christoph Hellwig wrote:> ?int> -ext2_clear_xip_target(struct inode *inode, int block) {> -???????sector_t sector = block*(PAGE_SIZE/512);> +ext2_clear_xip_target(struct inode *inode, int block)> +{> +???????sector_t sector = block * (PAGE_SIZE/512);> ????????unsigned long data;> ????????int rc;> ?> ????????rc = __inode_direct_access(inode, sector, &data);> -???????if (rc)> -???????????????return rc;> -???????clear_page((void*)data);> -???????return 0;> +???????if (!rc)> +???????????????clear_page(data);> +???????return rc;> ?} Did you try building it? While most of your changes areobviously correct, passing an unsigned long value to clear_pagewithout a cast isn't.

On what hardware? ;-) Ok, that one is indeed bogus.

Btw, you mailer totally messed up the quoted part.