On Thu, Sep 15, 2011 at 1:14 AM, Ted Ts'o <[email protected]> wrote:
> On Wed, Sep 14, 2011 at 01:59:06PM -0600, Andreas Dilger wrote:
>>
>> There is the field that you told Amir he could use for the exception
>> bitmap for snapshots, which is using one of the two reserved fields in
>> ext2_group_desc, and also one of the 3 reserved fields in ext4_group_desc
>> for 64-bit block numbers. ?That leaves one __u32 in ext2_group_desc, and
>> two __u32 in ext4_group_desc for checksums.
>
> Right, that's what I was forgetting. ?Thanks for reminding me!
>
Thanks Andreas for reminding!
Here is a patch to reserve the exclude bitmap field, so we won't
forget them again...
The full patch series for adding exclude bitmap can be found here:
https://github.com/amir73il/e2fsprogs-snapshots-patch-queue
---
From: Amir Goldstein <[email protected]>
Date: Thu, 15 Sep 2011 09:42:38 +0300
Ext4 snapshots require the compatible feature 'exclude_bitmap', meaning that
the exclude bitmap was allocated.
The allocated exclude bitmap blocks are stored in the block group descriptors.
To allow easy migration of existing Next3 installations, I took the liberty
of 'burning' the old 'exclude_inode' compatible flag and using a new
'exclude_bitmap' comaptible flag, to state the exclude bitmap blocks are stored
in the group descriptors.
Signed-off-by: Amir Goldstein <[email protected]>
---
lib/ext2fs/ext2_fs.h | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
index 54cb3d4..f1f0e19 100644
--- a/lib/ext2fs/ext2_fs.h
+++ b/lib/ext2fs/ext2_fs.h
@@ -142,7 +142,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_reserved[1];
__u16 bg_itable_unused; /* Unused inodes count */
__u16 bg_checksum; /* crc16(s_uuid+grouo_num+group_desc)*/
};
@@ -159,7 +160,8 @@ struct ext4_group_desc
__u16 bg_free_inodes_count; /* Free inodes count */
__u16 bg_used_dirs_count; /* Directories count */
__u16 bg_flags; /* EXT4_BG_flags (INODE_UNINIT, etc) */
- __u32 bg_reserved[2]; /* Likely block/inode bitmap checksum */
+ __u32 bg_exclude_bitmap; /* Exclude bitmap block */
+ __u32 bg_reserved[1]; /* Likely block/inode bitmap checksum */
__u16 bg_itable_unused; /* Unused inodes count */
__u16 bg_checksum; /* crc16(sb_uuid+group+desc) */
__u32 bg_block_bitmap_hi; /* Blocks bitmap block MSB */
@@ -169,12 +171,14 @@ struct ext4_group_desc
__u16 bg_free_inodes_count_hi;/* Free inodes count MSB */
__u16 bg_used_dirs_count_hi; /* Directories count MSB */
__u16 bg_itable_unused_hi; /* Unused inodes count MSB */
- __u32 bg_reserved2[3];
+ __u32 bg_exclude_bitmap_hi; /* Exclude bitmap block MSB */
+ __u32 bg_reserved2[2];
};
#define EXT2_BG_INODE_UNINIT 0x0001 /* Inode table/bitmap not initialized */
#define EXT2_BG_BLOCK_UNINIT 0x0002 /* Block bitmap not initialized */
#define EXT2_BG_INODE_ZEROED 0x0004 /* On-disk itable initialized to zero */
+#define EXT2_BG_EXCLUDE_UNINIT 0x0008 /* Exclude bitmap not initialized */
/*
* Data structures used by the directory indexing feature
@@ -670,7 +674,8 @@ struct ext2_super_block {
#define EXT2_FEATURE_COMPAT_RESIZE_INODE 0x0010
#define EXT2_FEATURE_COMPAT_DIR_INDEX 0x0020
#define EXT2_FEATURE_COMPAT_LAZY_BG 0x0040
-#define EXT2_FEATURE_COMPAT_EXCLUDE_INODE 0x0080
+/* #define EXT2_FEATURE_COMPAT_EXCLUDE_INODE 0x0080 not used */
+#define EXT2_FEATURE_COMPAT_EXCLUDE_BITMAP 0x0100
#define EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER 0x0001
#define EXT2_FEATURE_RO_COMPAT_LARGE_FILE 0x0002
--
1.7.0.4
On 2011-09-15, at 12:50 AM, Amir Goldstein wrote:
> Here is a patch to reserve the exclude bitmap field, so we won't
> forget them again...
> The full patch series for adding exclude bitmap can be found here:
> https://github.com/amir73il/e2fsprogs-snapshots-patch-queue
>
> ---
> From: Amir Goldstein <[email protected]>
> Date: Thu, 15 Sep 2011 09:42:38 +0300
>
> Ext4 snapshots require the compatible feature 'exclude_bitmap', meaning that
> the exclude bitmap was allocated.
> The allocated exclude bitmap blocks are stored in the block group descriptors.
>
> To allow easy migration of existing Next3 installations, I took the liberty
> of 'burning' the old 'exclude_inode' compatible flag and using a new
> 'exclude_bitmap' comaptible flag, to state the exclude bitmap blocks are stored
> in the group descriptors.
>
> Signed-off-by: Amir Goldstein <[email protected]>
> ---
> lib/ext2fs/ext2_fs.h | 13 +++++++++----
> 1 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
> index 54cb3d4..f1f0e19 100644
> --- a/lib/ext2fs/ext2_fs.h
> +++ b/lib/ext2fs/ext2_fs.h
> @@ -142,7 +142,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 */
I'd prefer if these comments were "Snapshot exclude bitmap block" so
that it is more clear what an exclude bitmap is used for.
For consistency, it would also be good to name these "_lo" to match the
kernel fields.
> + __u32 bg_reserved[1];
> __u16 bg_itable_unused; /* Unused inodes count */
> __u16 bg_checksum; /* crc16(s_uuid+grouo_num+group_desc)*/
> };
> @@ -159,7 +160,8 @@ struct ext4_group_desc
> __u16 bg_free_inodes_count; /* Free inodes count */
> __u16 bg_used_dirs_count; /* Directories count */
> __u16 bg_flags; /* EXT4_BG_flags (INODE_UNINIT, etc) */
> - __u32 bg_reserved[2]; /* Likely block/inode bitmap checksum */
> + __u32 bg_exclude_bitmap; /* Exclude bitmap block */
> + __u32 bg_reserved[1]; /* Likely block/inode bitmap checksum */
> __u16 bg_itable_unused; /* Unused inodes count */
> __u16 bg_checksum; /* crc16(sb_uuid+group+desc) */
> __u32 bg_block_bitmap_hi; /* Blocks bitmap block MSB */
>
> @@ -169,12 +171,14 @@ struct ext4_group_desc
> __u16 bg_free_inodes_count_hi;/* Free inodes count MSB */
> __u16 bg_used_dirs_count_hi; /* Directories count MSB */
> __u16 bg_itable_unused_hi; /* Unused inodes count MSB */
> - __u32 bg_reserved2[3];
> + __u32 bg_exclude_bitmap_hi; /* Exclude bitmap block MSB */
> + __u32 bg_reserved2[2];
> };
>
> #define EXT2_BG_INODE_UNINIT 0x0001 /* Inode table/bitmap not initialized */
> #define EXT2_BG_BLOCK_UNINIT 0x0002 /* Block bitmap not initialized */
> #define EXT2_BG_INODE_ZEROED 0x0004 /* On-disk itable initialized to zero */
> +#define EXT2_BG_EXCLUDE_UNINIT 0x0008 /* Exclude bitmap not initialized */
>
> /*
> * Data structures used by the directory indexing feature
> @@ -670,7 +674,8 @@ struct ext2_super_block {
> #define EXT2_FEATURE_COMPAT_RESIZE_INODE 0x0010
> #define EXT2_FEATURE_COMPAT_DIR_INDEX 0x0020
> #define EXT2_FEATURE_COMPAT_LAZY_BG 0x0040
> -#define EXT2_FEATURE_COMPAT_EXCLUDE_INODE 0x0080
> +/* #define EXT2_FEATURE_COMPAT_EXCLUDE_INODE 0x0080 not used */
> +#define EXT2_FEATURE_COMPAT_EXCLUDE_BITMAP 0x0100
>
> #define EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER 0x0001
> #define EXT2_FEATURE_RO_COMPAT_LARGE_FILE 0x0002
> --
> 1.7.0.4
Cheers, Andreas
On Thu, Sep 15, 2011 at 09:50:20AM +0300, Amir Goldstein wrote:
> -#define EXT2_FEATURE_COMPAT_EXCLUDE_INODE 0x0080
> +/* #define EXT2_FEATURE_COMPAT_EXCLUDE_INODE 0x0080 not used */
> +#define EXT2_FEATURE_COMPAT_EXCLUDE_BITMAP 0x0100
Why this change? Is it because you're already using 0x0100 in
shipping systems?
- Ted
On Thu, Sep 15, 2011 at 4:16 PM, Ted Ts'o <[email protected]> wrote:
> On Thu, Sep 15, 2011 at 09:50:20AM +0300, Amir Goldstein wrote:
>> -#define EXT2_FEATURE_COMPAT_EXCLUDE_INODE ? ? ?0x0080
>> +/* #define EXT2_FEATURE_COMPAT_EXCLUDE_INODE ? 0x0080 not used */
>> +#define EXT2_FEATURE_COMPAT_EXCLUDE_BITMAP ? ? 0x0100
>
> Why this change? ?Is it because you're already using 0x0100 in
> shipping systems?
>
I am using 0x80 in shipping systems and it signifies something a bit
different then
the proposed 0x100.
EXCLUDE_INODE means that special inode 9 is used to reference exclude
bitmap blocks.
EXCLUDE_BITMAP means that exclude bitmap blocks are referenced from
group descriptors.
With this distinction it will be easier for me to make the migration.
Thanks,
Amir.
On Thu, Sep 15, 2011 at 1:06 PM, Andreas Dilger
<[email protected]> wrote:
> On 2011-09-15, at 12:50 AM, Amir Goldstein wrote:
>> Here is a patch to reserve the exclude bitmap field, so we won't
>> forget them again...
>> The full patch series for adding exclude bitmap can be found here:
>> https://github.com/amir73il/e2fsprogs-snapshots-patch-queue
>>
>> ---
>> From: Amir Goldstein <[email protected]>
>> Date: Thu, 15 Sep 2011 09:42:38 +0300
>>
>> Ext4 snapshots require the compatible feature 'exclude_bitmap', meaning that
>> the exclude bitmap was allocated.
>> The allocated exclude bitmap blocks are stored in the block group descriptors.
>>
>> To allow easy migration of existing Next3 installations, I took the liberty
>> of 'burning' the old 'exclude_inode' compatible flag and using a new
>> 'exclude_bitmap' comaptible flag, to state the exclude bitmap blocks are stored
>> in the group descriptors.
>>
>> Signed-off-by: Amir Goldstein <[email protected]>
>> ---
>> lib/ext2fs/ext2_fs.h | ? 13 +++++++++----
>> 1 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
>> index 54cb3d4..f1f0e19 100644
>> --- a/lib/ext2fs/ext2_fs.h
>> +++ b/lib/ext2fs/ext2_fs.h
>> @@ -142,7 +142,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 */
>
> I'd prefer if these comments were "Snapshot exclude bitmap block" so
> that it is more clear what an exclude bitmap is used for.
I'll change that.
>
> For consistency, it would also be good to name these "_lo" to match the
> kernel fields.
>
That's a major cleanup.
Forgive me for not picking up that glove...
>> + ? ? ? __u32 ? bg_reserved[1];
>> ? ? ? ?__u16 ? bg_itable_unused; ? ? ? /* Unused inodes count */
>> ? ? ? ?__u16 ? bg_checksum; ? ? ? ? ? ?/* crc16(s_uuid+grouo_num+group_desc)*/
>> };
>> @@ -159,7 +160,8 @@ struct ext4_group_desc
>> ? ? ? ?__u16 ? bg_free_inodes_count; ? /* Free inodes count */
>> ? ? ? ?__u16 ? bg_used_dirs_count; ? ? /* Directories count */
>> ? ? ? ?__u16 ? bg_flags; ? ? ? ? ? ? ? /* EXT4_BG_flags (INODE_UNINIT, etc) */
>> - ? ? ? __u32 ? bg_reserved[2]; ? ? ? ? /* Likely block/inode bitmap checksum */
>> + ? ? ? __u32 ? bg_exclude_bitmap; ? ? ?/* Exclude bitmap block */
>> + ? ? ? __u32 ? bg_reserved[1]; ? ? ? ? /* Likely block/inode bitmap checksum */
>> ? ? ? ?__u16 ? bg_itable_unused; ? ? ? /* Unused inodes count */
>> ? ? ? ?__u16 ? bg_checksum; ? ? ? ? ? ?/* crc16(sb_uuid+group+desc) */
>> ? ? ? ?__u32 ? bg_block_bitmap_hi; ? ? /* Blocks bitmap block MSB */
>>
>> @@ -169,12 +171,14 @@ struct ext4_group_desc
>> ? ? ? ?__u16 ? bg_free_inodes_count_hi;/* Free inodes count MSB */
>> ? ? ? ?__u16 ? bg_used_dirs_count_hi; ?/* Directories count MSB */
>> ? ? ? ?__u16 ? bg_itable_unused_hi; ? ?/* Unused inodes count MSB */
>> - ? ? ? __u32 ? bg_reserved2[3];
>> + ? ? ? __u32 ? bg_exclude_bitmap_hi; ? /* Exclude bitmap block MSB */
>> + ? ? ? __u32 ? bg_reserved2[2];
>> };
>>
>> #define EXT2_BG_INODE_UNINIT ? 0x0001 /* Inode table/bitmap not initialized */
>> #define EXT2_BG_BLOCK_UNINIT ? 0x0002 /* Block bitmap not initialized */
>> #define EXT2_BG_INODE_ZEROED ? 0x0004 /* On-disk itable initialized to zero */
>> +#define EXT2_BG_EXCLUDE_UNINIT 0x0008 /* Exclude bitmap not initialized */
>>
>> /*
>> ?* Data structures used by the directory indexing feature
>> @@ -670,7 +674,8 @@ struct ext2_super_block {
>> #define EXT2_FEATURE_COMPAT_RESIZE_INODE ? ? ? 0x0010
>> #define EXT2_FEATURE_COMPAT_DIR_INDEX ? ? ? ? ?0x0020
>> #define EXT2_FEATURE_COMPAT_LAZY_BG ? ? ? ? ? ?0x0040
>> -#define EXT2_FEATURE_COMPAT_EXCLUDE_INODE ? ? ?0x0080
>> +/* #define EXT2_FEATURE_COMPAT_EXCLUDE_INODE ? 0x0080 not used */
>> +#define EXT2_FEATURE_COMPAT_EXCLUDE_BITMAP ? ? 0x0100
>>
>> #define EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER ? ?0x0001
>> #define EXT2_FEATURE_RO_COMPAT_LARGE_FILE ? ? ?0x0002
>> --
>> 1.7.0.4
>
>
> Cheers, Andreas
>
>
>
>
>
>
On Thu, Sep 15, 2011 at 05:03:37PM +0300, Amir Goldstein wrote:
> > I'd prefer if these comments were "Snapshot exclude bitmap block" so
> > that it is more clear what an exclude bitmap is used for.
>
> I'll change that.
I'll be sending out a combined patch very shortly which adds the
necessary fields for the snapshot and metadata checksum fields. I
just want to make sure we're all on the same page about reserved
fields as far as the on-disk format is concerned.
> >
> > For consistency, it would also be good to name these "_lo" to match the
> > kernel fields.
> >
>
> That's a major cleanup.
> Forgive me for not picking up that glove...
One way of handling these sorts of cleanups is to make the change in
the header file, and then do something like this:
#define bg_exclude_bitmap bg_exclude_bitmap_lo
Then once you add the accessor functions to set/get the
exclude_bitmap, you can delete the #define to make sure you fixed up
all of your code to actually use the accessor functions.
- Ted
On 2011-09-15, at 7:47 AM, Amir Goldstein wrote:
> On Thu, Sep 15, 2011 at 4:16 PM, Ted Ts'o <[email protected]> wrote:
>> On Thu, Sep 15, 2011 at 09:50:20AM +0300, Amir Goldstein wrote:
>>> -#define EXT2_FEATURE_COMPAT_EXCLUDE_INODE 0x0080
>>> +/* #define EXT2_FEATURE_COMPAT_EXCLUDE_INODE 0x0080 not used */
>>> +#define EXT2_FEATURE_COMPAT_EXCLUDE_BITMAP 0x0100
>>
>> Why this change? Is it because you're already using 0x0100 in
>> shipping systems?
>
> I am using 0x80 in shipping systems and it signifies something a bit
> different then the proposed 0x100.
>
> EXCLUDE_INODE means that special inode 9 is used to reference exclude
> bitmap blocks. EXCLUDE_BITMAP means that exclude bitmap blocks are
> referenced from group descriptors.
> With this distinction it will be easier for me to make the migration.
In that light, why not continue to use an inode to map the exclude bitmap
blocks, where the bitmap offset is (group * blocksize), instead of
explicitly listing all of the blocks in the group descriptor? This is
how the buddy bitmap works in memory only, but it could be done for the
exclude bitmap on disk.
The advantage of this is that it would allow the 32-bit bitmap checksums
to both fit into the group descriptor. The disadvantage is that there
is a chance this inode would become corrupted and the location of the
exclude bitmaps is lost. I don't know how serious that is (e.g. if e2fsck
could fix it by regenerating the bitmaps, or just deleting the snapshot).
Cheers, Andreas
On Thu, Sep 15, 2011 at 01:08:34PM -0600, Andreas Dilger wrote:
> In that light, why not continue to use an inode to map the exclude bitmap
> blocks, where the bitmap offset is (group * blocksize), instead of
> explicitly listing all of the blocks in the group descriptor? This is
> how the buddy bitmap works in memory only, but it could be done for the
> exclude bitmap on disk.
I seem to recall the use of an inode to map the exclude bitmap added a
huge amount of complexity to the snapshot patches. Amir, am I
remembering this correctly?
- Ted
On Fri, Sep 16, 2011 at 12:57 AM, Ted Ts'o <[email protected]> wrote:
> On Thu, Sep 15, 2011 at 01:08:34PM -0600, Andreas Dilger wrote:
>> In that light, why not continue to use an inode to map the exclude bitmap
>> blocks, where the bitmap offset is (group * blocksize), instead of
>> explicitly listing all of the blocks in the group descriptor? ?This is
>> how the buddy bitmap works in memory only, but it could be done for the
>> exclude bitmap on disk.
>
And this exactly is how the exclude inode works, except only the DIND
block is used
for mapping, just like the resize inode.
> I seem to recall the use of an inode to map the exclude bitmap added a
> huge amount of complexity to the snapshot patches. ?Amir, am I
> remembering this correctly?
>
No, I am not sure this is accurate.
I think after we over viewed the e2fsprogs snapshots patch set, you
has 2 observations:
1. the largest part (in lines of code) of the e2fsprogs snapshot patch set
is related to the exclude inode/bitmap code.
2. it reminded you of resize inode too much and you didn't like that
3. There was also the issue of whether or not to allow the removal of
the exclude inode/bitmap
and then re-allocation would not be in optimal layout
In retrospect, after Yongqiang has implemented the alternative exclude
bitmap patch set
for e2fsprogs, I can say:
1. The alternative patch set is not smaller
2. It is a lot more elegant and deals with correct layout of exclude
bitmap (next to block bitmap)
3. It will be able to deal with 64bit fs (unlike exclude/resize inode)
and 64bit resize
Yongqiang, do you have anything else to add to the exclude inode vs.
group desc issue?
Amir.
On Thu, Sep 15, 2011 at 10:08 PM, Andreas Dilger
<[email protected]> wrote:
> On 2011-09-15, at 7:47 AM, Amir Goldstein wrote:
>> On Thu, Sep 15, 2011 at 4:16 PM, Ted Ts'o <[email protected]> wrote:
>>> On Thu, Sep 15, 2011 at 09:50:20AM +0300, Amir Goldstein wrote:
>>>> -#define EXT2_FEATURE_COMPAT_EXCLUDE_INODE ? ? ?0x0080
>>>> +/* #define EXT2_FEATURE_COMPAT_EXCLUDE_INODE ? 0x0080 not used */
>>>> +#define EXT2_FEATURE_COMPAT_EXCLUDE_BITMAP ? ? 0x0100
>>>
>>> Why this change? ?Is it because you're already using 0x0100 in
>>> shipping systems?
>>
>> I am using 0x80 in shipping systems and it signifies something a bit
>> different then the proposed 0x100.
>>
>> EXCLUDE_INODE means that special inode 9 is used to reference exclude
>> bitmap blocks. ?EXCLUDE_BITMAP means that exclude bitmap blocks are
>> referenced from group descriptors.
>> With this distinction it will be easier for me to make the migration.
>
> In that light, why not continue to use an inode to map the exclude bitmap
> blocks, where the bitmap offset is (group * blocksize), instead of
> explicitly listing all of the blocks in the group descriptor? ?This is
> how the buddy bitmap works in memory only, but it could be done for the
> exclude bitmap on disk.
>
> The advantage of this is that it would allow the 32-bit bitmap checksums
> to both fit into the group descriptor. ?The disadvantage is that there
> is a chance this inode would become corrupted and the location of the
> exclude bitmaps is lost. ?I don't know how serious that is (e.g. if e2fsck
> could fix it by regenerating the bitmaps, or just deleting the snapshot).
>
> Cheers, Andreas
>
fsck can fix it. it just marks all blocks used by snapshot inodes.
Amir.
On Fri, Sep 16, 2011 at 2:45 PM, Amir Goldstein <[email protected]> wrote:
> On Fri, Sep 16, 2011 at 12:57 AM, Ted Ts'o <[email protected]> wrote:
>> On Thu, Sep 15, 2011 at 01:08:34PM -0600, Andreas Dilger wrote:
>>> In that light, why not continue to use an inode to map the exclude bitmap
>>> blocks, where the bitmap offset is (group * blocksize), instead of
>>> explicitly listing all of the blocks in the group descriptor? ?This is
>>> how the buddy bitmap works in memory only, but it could be done for the
>>> exclude bitmap on disk.
>>
>
> And this exactly is how the exclude inode works, except only the DIND
> block is used
> for mapping, just like the resize inode.
>
>> I seem to recall the use of an inode to map the exclude bitmap added a
>> huge amount of complexity to the snapshot patches. ?Amir, am I
>> remembering this correctly?
>>
>
> No, I am not sure this is accurate.
> I think after we over viewed the e2fsprogs snapshots patch set, you
> has 2 observations:
> 1. the largest part (in lines of code) of the e2fsprogs snapshot patch set
> ? ?is related to the exclude inode/bitmap code.
> 2. it reminded you of resize inode too much and you didn't like that
> 3. There was also the issue of whether or not to allow the removal of
> the exclude inode/bitmap
> ? ?and then re-allocation would not be in optimal layout
>
> In retrospect, after Yongqiang has implemented the alternative exclude
> bitmap patch set
> for e2fsprogs, I can say:
> 1. The alternative patch set is not smaller
> 2. It is a lot more elegant and deals with correct layout of exclude
> bitmap (next to block bitmap)
> 3. It will be able to deal with 64bit fs (unlike exclude/resize inode)
> and 64bit resize
>
> Yongqiang, do you have anything else to add to the exclude inode vs.
> group desc issue?
Nope, regarding resize group desc is better than exclude inode. For
meta_bg, group desc is much more welcome.
Yongqiang.
>
> Amir.
>
--
Best Wishes
Yongqiang Yang
On 2011-09-16, at 5:43 AM, Yongqiang Yang wrote:
> On Fri, Sep 16, 2011 at 2:45 PM, Amir Goldstein <[email protected]> wrote:
>> No, I am not sure this is accurate.
>> I think after we over viewed the e2fsprogs snapshots patch set, you
>> has 2 observations:
>> 1. the largest part (in lines of code) of the e2fsprogs snapshot patch set
>> is related to the exclude inode/bitmap code.
>> 2. it reminded you of resize inode too much and you didn't like that
>> 3. There was also the issue of whether or not to allow the removal of
>> the exclude inode/bitmap
>> and then re-allocation would not be in optimal layout
>>
>> In retrospect, after Yongqiang has implemented the alternative exclude
>> bitmap patch set
>> for e2fsprogs, I can say:
>> 1. The alternative patch set is not smaller
>> 2. It is a lot more elegant and deals with correct layout of exclude
>> bitmap (next to block bitmap)
>> 3. It will be able to deal with 64bit fs (unlike exclude/resize inode)
>> and 64bit resize
>>
>> Yongqiang, do you have anything else to add to the exclude inode vs.
>> group desc issue?
>
> Nope, regarding resize group desc is better than exclude inode. For
> meta_bg, group desc is much more welcome.
I'm not dead-set on using the exclude inode. I was just wondering if there
was a clear benefit to doing so. I'm OK with having 16-bit checksums for
the inode bitmaps for now, and possibly changing mke2fs to always creating
ext4 filesystems with the 64bit feature. Always specifying 64bit for ext4
filesystems will allow larger checksums, and has the added benefit of
facilitating resize from below 2^32 blocks to over 2^32 blocks as well.
One minor drawback is the size of the s_group_desc array doubles in size, so
the allocation needs to be larger (handled in newer kernels by ext4_kvmalloc())
and it needs more space on disk. At 16TB the group descriptor table would
use 8MB instead of 4MB, and at 256TB the group descriptor table would fill
the whole 128MB of the first group, where we HAVE to use META_BG because the
second group is also filled with the backup GDT.
Cheers, Andreas