Hi Andreas, Thank you for taking the time to review my patches. FYI, those are not the most recent patches. the latest patch series is available at: http://sourceforge.net/projects/next3/files/Latest%20patch%20series If you like, I can post the latest patches to the list. Also, I suggested Next3/Ext4 merge issues as a topic for LSF: http://marc.info/?l=linux-fsdevel&m=127675996522835&w=2 If you are interested, please help me promote this topic. >> + { "snapshot_inum", &set_sb.s_snapshot_inum, 4, parse_uint }, >> + { "snapshot_id", &set_sb.s_snapshot_id, 4, parse_uint }, >> + { "snapshot_r_blocks_count", &set_sb.s_snapshot_r_blocks_count, >> + 4, parse_uint }, >> + { "snapshot_list", &set_sb.s_snapshot_list, 4, parse_uint }, >> { "hash_seed", &set_sb.s_hash_seed, 16, parse_uuid }, >> { "def_hash_version", &set_sb.s_def_hash_version, 1, parse_hashalg }, >> { "jnl_backup_type", &set_sb.s_jnl_backup_type, 1, parse_uint }, > > > These should be added into the array in superblock offset order, rather than into the middle of the array. OK. I will fix all the arrays you mentioned. > > >> @@ -144,7 +144,8 @@ struct ext2_group_desc >> __u16 bg_free_inodes_count; /* Free inodes count */ >> __u16 bg_used_dirs_count; /* Directories count */ >> __u16 bg_flags; >> - __u32 bg_reserved >> [2] >> ; >> + __u32 bg_exclude_bitmap; /* Exclude bitmap block */ >> + __u32 bg_cow_bitmap; /* COW bitmap block of last snapshot */ > > These two fields were intended to hold the 32-bit checksum of the inode and block bitmaps for this group. Also, a 32-bit block number is not sufficient for a filesystem larger than 2^32 blocks, and there is no way to extend this in the future to hold the full block number. Ted has already told me to evacuate those fields, which I did. They were moved into a per-group memory-only struct. > >> @@ -426,14 +427,14 @@ struct ext2_inode_large { >> -#define i_reserved1 osd1.linux1.l_i_reserved1 >> +#define i_next_snapshot osd1.linux1.l_i_version > > This field is already in use by NFS and Lustre, I don't think it is correct to re-use it. > I discussed this issue with Ted and he agreed that i_version can be overloaded with next_snapshot. This is because snapshot files are not writable to users, so as far as NFS and Lustre are concerned, a snapshot file never changes, which is true. The content of a snapshot file is frozen from snapshot take to snapshot delete. only the metadata of snapshot files changes in time. >> @@ -638,6 +648,10 @@ struct ext2_super_block { >> #define EXT4_FEATURE_RO_COMPAT_GDT_CSUM 0x0010 >> #define EXT4_FEATURE_RO_COMPAT_DIR_NLINK 0x0020 >> #define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE 0x0040 >> +#define NEXT3_FEATURE_RO_COMPAT_HAS_SNAPSHOT 0x1000 /* Next3 has snapshots */ >> +#define NEXT3_FEATURE_RO_COMPAT_IS_SNAPSHOT 0x2000 /* Is a snapshot image */ >> +#define NEXT3_FEATURE_RO_COMPAT_FIX_SNAPSHOT 0x4000 /* Corrupted snapshot */ >> +#define NEXT3_FEATURE_RO_COMPAT_FIX_EXCLUDE 0x8000 /* Bad exclude bitmap */ > > Are all of these states mutually exclusive? Can they legally all be set at the same time? > The last 3 features were moved to s_flags. The 2 'fix' flags are independent and 'tune2fs -O ^has_snapshot' and e2fsck clears them both. 'is_snapshot' is only set inside a read-only snapshot image, where 'has_snapshot' is clear. I was going to ask Ted to re-assign me the 'is_snapshot' feature, but didn't get around to it yet. Thanks, Amir. -- 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
