Re: [PATCH 0/2] cramfs: Add mount option swapendian
Am Donnerstag, 15. November 2007 23:48 schrieb Phillip Lougher: So a v2 cramfs would be a great idea. That is what I always considered Squashfs to be. But I also made the mistake of making Squashfs both little and big endian. That's going to be fixed and then I'll make a second attempt at submitting it for inclusion in the mainline kernel. I didn't have a closer look on squashfs, but I am going to have it this weekend. So far, it seems as if no cramfs v2 is needed. My proposal is the following: I'll write a new patch for inclusion in the mainline kernel that makes cramfs little endian only. For people who really want to be able to mount filesystems with both kinds of endianness there will be a seperate patch (not intended to be merged into mainline) available somewhere on the net (my website or so). It will definitely be marked as non-official in order to prevent people from creating images in big endian. Officially, cramfs should only support little endian images. If squashfs will be merged, cramfs should be marked as obsolete. Andi - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] cramfs: Add mount option swapendian
On Fri, 16 Nov 2007, Andi Drebes wrote: I'll write a new patch for inclusion in the mainline kernel that makes cramfs little endian only. For people who really want to be able to mount filesystems with both kinds of endianness there will be a seperate patch (not intended to be merged into mainline) available somewhere on the net (my website or so). It will definitely be marked as non-official in order to prevent people from creating images in big endian. Officially, cramfs should only support little endian images. Good. We do actually have precedence for exactly this kind of situation before: it's what happened to ext2 as well (people were trying to push a switch-endian version, and I said no) and the m68k people had their own patches for a while but we're *so* much better off from being fixed-endian that it's not even funny. If squashfs will be merged, cramfs should be marked as obsolete. Sounds like that, yes. Linus - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] cramfs: Add mount option swapendian
On Thu, 15 Nov 2007, Andi Drebes wrote: This patch introduces the mount option swapendian for cramfs. When this option is set, cramfs is able to mount an image that was created on a machine whose endianness differs from the mounting machine's one. Please don't do it this way. It would be *much* better to just standardize on one endianness, and be done with it. That way there are no config options, no confusion, and the code is smaller, simpler, and faster. Because nn unconditional byte swap is generally faster than a conditional non-byte-swap! So can you please just make it little-endian? There can't be that many big-endian machines that really care about old cramfs images.. Linus - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] cramfs: Add mount option swapendian
On Thu, 15 Nov 2007, Linus Torvalds wrote: It would be *much* better to just standardize on one endianness, and be done with it. That way there are no config options, no confusion, and the code is smaller, simpler, and faster. .. it's also statically checkable with tools like sparse, so it avoids bugs not only by being simpler, but by simply being fundamentally more robust to start with. Linus - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] cramfs: Add mount option swapendian
On Thu, Nov 15, 2007 at 12:45:20PM -0800, Linus Torvalds wrote: Please don't do it this way. It would be *much* better to just standardize on one endianness, and be done with it. That way there are no config options, no confusion, and the code is smaller, simpler, and faster. Because nn unconditional byte swap is generally faster than a conditional non-byte-swap! So can you please just make it little-endian? There can't be that many big-endian machines that really care about old cramfs images.. Actually there are as lots of initrd are cramfs. This means you'd need to update mkcramfs all big endian machines out there. - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] cramfs: Add mount option swapendian
On Thu, 15 Nov 2007, Christoph Hellwig wrote: Actually there are as lots of initrd are cramfs. This means you'd need to update mkcramfs all big endian machines out there. So? Normally the initrd goes with the kernel anyway. Linus - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] cramfs: Add mount option swapendian
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 000..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 [EMAIL PROTECTED]
+ *
+ * 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)
+
+/*
Re: [PATCH 0/2] cramfs: Add mount option swapendian
This patch introduces the mount option swapendian for cramfs. When this option is set, cramfs is able to mount an image that was created on a machine whose endianness differs from the mounting machine's one. Please don't do it this way. It would be *much* better to just standardize on one endianness, and be done with it. That way there are no config options, no confusion, and the code is smaller, simpler, and faster. Because nn unconditional byte swap is generally faster than a conditional non-byte-swap! This is a valid objection. The problem is that the endianness for cramfs has never been standardized. Now there are filesystem images in both little and big endian out there. I encountered this problem first when I tried to mount my router's initrd. Yes, I know, I could have converted the image into little endian. I just find that it is more consistent to support both kinds of endianness. So can you please just make it little-endian? Actually, in my eyes, it would be better to create a new version of cramfs that standardizes the endianness and the block size. But that doesn't solve the problems one might have with old images. There can't be that many big-endian machines that really care about old cramfs images.. s.a. (There's at least one...) Andi - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] cramfs: Add mount option swapendian
Linus Torvalds wrote: But it should be *trivial* to compress the metadata too if the code just were to put the metadata at the beginning of the image, and the real data at the end, and then you can build up the image from both ends and you *can* have a fixed starting point for the data (up at the top of the image) even though you are changing the size of the metadata by compression. I decided to compress the metadata when I designed Squashfs, a read-only filesystem which was inspired by Cramfs. Squashfs stores the data at the front of the filesystem and puts the metadata at the end, so the data is always at a fixed point. Doing that and a couple of other things allows the metadata to be built up and compressed in one-pass while the filesystem is being created. The metadata is split into an inode table and a directory table and compressed separately because it compresses better than way. But I literally designed and wrote the thing in a couple of days, and I really didn't think it through right. As a result, the metadata may be dense, but it's totally uncompressed. It would have been better to allow a less dense basic format (allowing bigger uid/gid values, and offsets and file sizes), but compress it. Squashfs stores much more metadata information, but as it is compressed it is much smaller than Cramfs. Typically the inode table compresses to less than 40% and the directory table to less than 50%. So a v2 cramfs would be a great idea. That is what I always considered Squashfs to be. But I also made the mistake of making Squashfs both little and big endian. That's going to be fixed and then I'll make a second attempt at submitting it for inclusion in the mainline kernel. Phillip - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
