On 2012-02-08, at 11:08 AM, Ted Ts'o wrote: > So I've started looking at this patch series, and I'm wondering if it > might be better if we collapse these two feature flags: > > EXT4_FEATURE_RO_COMPAT_METADATA_CSUM > and > EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM > > To a single feature flag, EXT4_FEATURE_INCOMPAT_METADATA_CSUM, which > also implies EXT4_FEATURE_RO_COMPAT_GDT_CSUM. (So in the future, when > we enable INCOMPAT_CSUM_METADATA, the presence or absence of > EXT4_RO_COMPAT_GDT_CSUM won't matter, and in fact mke2fs will skip > setting that feature flag altogether. Tune2fs could also drop the > GDT_CSUM flag when adding the CSUM_METADATA flag.) The only reason for INCOMPAT_BG_USE_META_CSUM is to change the checksum algorithm for group descriptors from crc16 to half-crc32 to match the other metadata checksums. I'd rather keep the metadata checksum feature RO_COMPAT rather than push it to INCOMPAT over this tiny-and-mostly-meaningless change. If enabling RO_COMPAT_METADATA_CSUM implies RO_COMPAT_GDT_CSUM, then it could also imply that the GDT checksum is using half-crc32 easily. > The reasoning behind this is that it simplifies the combinatorics we > need to test, and it also simplifies our code base. In addition, it's > really easy to make tune2fs recalculate the checksums when the feature > flag is set, and reculate the block group checksums using the old > algorithm when the metadata flag is unset. So if someone wants to > mount the file system on a downlevel kernel, it's really not that bad > that this feature is an INCOMPAT feature; we can easily downgrade it > using tune2fs. > > In fact, if we wanted to take this to extremes, we could call it > EXT4_FEATURE_INCOMPAT_NEW_METADATA, and then let it imply the > following feature flags as well: > > EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER > EXT4_FEATURE_RO_COMPAT_LARGE_FILE > EXT4_FEATURE_RO_COMPAT_HUGE_FILE > EXT4_FEATURE_RO_COMPAT_DIR_NLINK > EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE > EXT4_FEATURE_INCOMPAT_FILETYPE > EXT4_FEATURE_INCOMPAT_FLEX_BG The above set of features itself looks harmless enough, since they have been around for a long time and are mostly just relaxing checks in the code. However, I don't think that introducing a NEW feature flag will allow the checks for the old features to be removed from the code for a long time, so it will just mean checks for two features everywhere in the kernel and e2fsprogs. A simpler implementation would be to have something like: #define EXT4_FEATURE_RO_COMPAT_DEFAULT_EXT4 (list of above RO_COMPAT) #define EXT4_FEATURE_INCOMPAT_DEFAULT_EXT4 (list of above INCOMPAT) and fix libe2p to use "default_ext4" if all these flags are set. Also consider things like INCOMPAT_COMPRESSION and HAS_JOURNAL. It would be a shame if we had made a decision like this in the past and could not turn these features off, so being able to selectively disable these features in the future also has benefits. > We can add inline functions in fs/ext4/ext4.h and in > lib/ext2fs/ext2fs.h to make the source code look a bit simpler. Couldn't this all be done in the tools level, instead of changing the feature flags? Have debugfs/dumpe2fs/tune2fs treat a set of feature flags as "default_ext4" would be equivalent? > This would help to reduce our testing load, and it would also make > the output of dumpe2fs easier to understand... This could also be done at the tools level, by preventing users from setting strange combinations of feature flags unless they pass the "--yes-this-feature-combination-is-untested" to mke2fs, tune2fs, and debugfs, and deprecating truly ancient features entirely. I would be happy to just deprecate ancient features like SPARSE_SUPER, LARGE_FILE, FILETYPE, v1 filesystems entirely, and prevent them from being turned off, and have e2fsck "fix" any such filesystem that it finds (with a clear warning). If such ancient filesystems exist, they will not be running new e2fsprogs either. Next in line would be EXT_ATTR (with v1 xattrs) and DIR_INDEX, since they have been supported in ext3 for a long time. It is probably too early to deprecate DIR_NLINK, EXTRA_ISIZE, HUGE_FILE and FLEX_BG, since they are new in ext4, but eventually those too. Interestingly, ZFS had sequential versions (i.e. version N has to support the on-disk format of every feature in N-1 forever), and they have now moved over to using feature flags after I suggested this to them 4 years ago, exactly for the reason that it gives them the ability to develop/use features that may not be used everywhere. One improvement of the ZFS implementation is that it distinguishes between "allowed" features and "in-use" features. That allows the maximum set of features to be enabled at format/upgrade time, but doesn't cause interoperability problems until the features are used. Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
