Re: [PATCH 0/2] cramfs: Add mount option "swapendian"

["Jörn Engel" <joern@logfs.org>]

On Thu, 15 November 2007 21:35:16 +0100, Andi Drebes wrote:
> +	/* Caching is disabled if the filesystem's
> +	   and the machine's endianness differ. */
> +	if(likely(CRAMFS_SB(sb)->endian))
> +	{

Codingstyle: extra space after "if", keep brace on the same line.
Same goes for most of this patch.

Unlikely not likely to be a good idea.  It clutters up the code for a
minimal gain on host-endian filesystems (and I doubt you can measure
that even in micro-benchmarks) and forcibly mispredicts every branch for
cross-endian filesystems.

The name "endian" could be replaces with something more descriptive.
"host_endian" or "swap_endian" with reversed logic or something.

> +	/* check for wrong endianess */
> +	else if (super.magic == CRAMFS_MAGIC_WEND)
> +	{
> +other_endian:
> +		if (sbi->endian) {
> +			if (!silent) {
> +				printk(KERN_ERR "cramfs: it seems as if you were trying to mount a filesystem "
> +					"whose endianness does not match the host's one. You might want to try "
> +					"the option \"swapendian\" when mounting the filesystem.\n");
> +				printk(KERN_ERR "cramfs: the filesystem will not be mounted.\n");
> +			}
> +			goto out;
> +		}
> +		else {
> +			if (!sbi->endian) {
> +				if (!silent)
> +					printk(KERN_INFO "cramfs: mounting cramfs with another endianness\n");
> +				CRAMFS_CONVERT_SUPER(super);
> +			}
> +		}
> +	}
> +	else if (super.magic == CRAMFS_MAGIC && !sbi->endian)
> +	{
> +		printk(KERN_ERR "cramfs: you are trying to mount a filesystem whose endianness matches the "
> +			"host's one. Do not use the option \"swapendian\".\n");
> +		goto out;
> +	}

You could remove most of this by removing the mount option and simply
deciding endianness based on super.magic.  So why the extra work?

> @@ -483,9 +532,18 @@ static int cramfs_readpage(struct file *file, struct page * page)
>  
>  		start_offset = OFFSET(inode) + maxblock*4;
>  		mutex_lock(&read_mutex);
> -		if (page->index)
> +		if (page->index) {
>  			start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4, 4);
> -		compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset);
> +
> +			if(unlikely(!endian))
> +				start_offset = CPU_ENDIAN_32(start_offset);
> +		}
> +
> +		if(unlikely(!endian))
> +			compr_len = CPU_ENDIAN_32(*(u32 *) cramfs_read(sb, blkptr_offset, 4)) - start_offset;
> +		else
> +			compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset);

The two conditional can be combined into one.

> diff --git a/include/linux/cramfs_endian.h b/include/linux/cramfs_endian.h
> new file mode 100644
> index 0000000..9b90b26
> --- /dev/null
> +++ b/include/linux/cramfs_endian.h
> @@ -0,0 +1,58 @@
> +/*
> + * cramfs_endian.h provides definitions used when mounting
> + * a cram filesystem whose endianness doesn't match the host's
> + * one.
> + *
> + * Copyright 2007 (C) Andi Drebes <andi@programmierforen.de>
> + *
> + * This file is released under the GPLv2.
> + */
> +
> +#ifndef __CRAMFS_ENDIAN_H
> +#define __CRAMFS_ENDIAN_H
> +
> +#ifdef __LITTLE_ENDIAN
> +	#define CPU_ENDIAN_32(x) (be32_to_cpu(x))
> +	#define CPU_ENDIAN_16(x) (be16_to_cpu(x))
> +#elif defined __BIG_ENDIAN
> +	#define CPU_ENDIAN_32(x) (le32_to_cpu(x))
> +	#define CPU_ENDIAN_16(x) (le16_to_cpu(x))
> +#else
> +	#error "Neither __LITTLE_ENDIAN nor __BIG_ENDIAN is defined!"
> +#endif

You're using Xe32_to_cpu on host-endian numbers?  Sparse won't be happy.
Better just use swab32 directly.

> +
> +/* Converts a cramfs_info from the wrong endianess
> +   to host endianess. */
> +#define CRAMFS_CONVERT_INFO(info) \
> +	do { \
> +		(info).crc = CPU_ENDIAN_32((info).crc); \
> +		(info).edition = CPU_ENDIAN_32((info).edition); \
> +		(info).blocks = CPU_ENDIAN_32((info).blocks); \
> +		(info).files = CPU_ENDIAN_32((info).files); \
> +	} while(0)

Better make it a static inline function for type safety.  Possibly even
an out-of-line function.

> +/* Converts a cramfs_info from the wrong endianess
> +   to host endianess. */
> +#define CRAMFS_CONVERT_INODE(inode) \
> +	do { \
> +		__u8* ptr = (__u8*)(inode);\
> +		(inode)->mode = CPU_ENDIAN_16((inode)->mode); \
> +		(inode)->uid = CPU_ENDIAN_16((inode)->uid); \
> +		(inode)->size = (ptr[4] << 16) | (ptr[5] << 8) | (ptr[6]) ; \
> +		(inode)->offset = ((ptr[8] & 0x03) << 24) | (ptr[9] << 16) | (ptr[10] << 8) | ptr[11]; \
> +		(inode)->namelen = (ptr[8] & 0x3f) >> 2; \
> +	} while(0)
> +
> +/* Converts a cramfs superblock from the wrong endianess
> +   to host endianess. */
> +#define CRAMFS_CONVERT_SUPER(super) \
> +	do { \
> +		(super).magic = CPU_ENDIAN_32((super).magic); \
> +		(super).size = CPU_ENDIAN_32((super).size); \
> +		(super).flags = CPU_ENDIAN_32((super).flags); \
> +		(super).future = CPU_ENDIAN_32((super).future); \
> +		CRAMFS_CONVERT_INFO((super).fsid); \
> +		CRAMFS_CONVERT_INODE(&(super).root); \
> +	} while(0)

dito.

Jörn

-- 
The rabbit runs faster than the fox, because the rabbit is rinning for
his life while the fox is only running for his dinner.
-- Aesop
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html