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@xxxxxxxxxxxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
