Re: [PATCH 1/8] e2fsprogs: Next3 on-disk format changes

["Amir G." <amir73il@users.sourceforge.net>]

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