Date:	Fri, 23 Sep 2011 08:57:43 +0300
From:	Amir Goldstein <amir73il@...il.com>
To:	djwong@...ibm.com
Cc:	"Theodore Ts'o" <tytso@....edu>,
	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH 1/2] libext2fs: add metadata checksum and snapshot feature flags

On Thu, Sep 22, 2011 at 10:18 PM, Darrick J. Wong <djwong@...ibm.com> wrote:
> On Thu, Sep 22, 2011 at 05:12:32AM +0300, Amir Goldstein wrote:
>> On Wed, Sep 21, 2011 at 11:40 PM, Darrick J. Wong <djwong@...ibm.com> wrote:
>> > On Wed, Sep 21, 2011 at 11:03:57AM +0300, Amir Goldstein wrote:
>> >> On Tue, Sep 20, 2011 at 9:56 PM, Darrick J. Wong <djwong@...ibm.com> wrote:
>> >> > On Thu, Sep 15, 2011 at 06:50:51PM -0400, Theodore Ts'o wrote:
>> > <snip>
>> >> >> diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
>> >> >> index 4fec5db..1c86cb2 100644
>> >> >> --- a/lib/ext2fs/ext2_fs.h
>> >> >> +++ b/lib/ext2fs/ext2_fs.h
>> >> >> @@ -142,7 +142,9 @@ 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_lo;   /* Exclude bitmap for snapshots */
>> >> >> +     __u16   bg_block_bitmap_csum_lo;/* crc32c(s_uuid+grp_num+bitmap) LSB */
>> >> >> +     __u16   bg_inode_bitmap_csum_lo;/* crc32c(s_uuid+grp_num+bitmap) LSB */
>> >> >>       __u16   bg_itable_unused;       /* Unused inodes count */
>> >> >>       __u16   bg_checksum;            /* crc16(s_uuid+grouo_num+group_desc)*/
>> >> >>  };
>> >> >> @@ -159,7 +161,9 @@ 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_lo;   /* Exclude bitmap for snapshots */
>> >> >> +     __u16   bg_block_bitmap_csum_lo;/* crc32c(s_uuid+grp_num+bitmap) LSB */
>> >> >> +     __u16   bg_inode_bitmap_csum_lo;/* crc32c(s_uuid+grp_num+bitmap) LSB */
>> >> >>       __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,7 +173,10 @@ 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 */
>> >> >> +     __u16   bg_block_bitmap_csum_hi;/* crc32c(s_uuid+grp_num+bitmap) MSB */
>> >> >> +     __u16   bg_inode_bitmap_csum_hi;/* crc32c(s_uuid+grp_num+bitmap) MSB */
>> >> >> +     __u32   bg_reserved;
>> >> >
>> >> > Should we attach a checksum to the exclude bitmap?  That would, unfortunately,
>> >> > put the 64-byte block group pretty close to full, unless we're ok with making
>> >> > the bg even bigger...
>> >>
>> >> Good point.
>> >> My initial approach (which I probably expressed somewhere) was that when
>> >> exclude_bitmap feature is set, the block bitmap checksum should cover both
>> >> block bitmap and exclude bitmap.
>> >> The reason being that exclude bitmap adds little entropy over block bitmap:
>> >> - it should always be a sub set of block bitmap bits
>> >> - clearing exclude bit is done together with clearing the block used bit
>> >> - the only case of modifying exclude bitmap only is when a used block becomes
>> >>   excluded (a.k.a moved to snapshot)
>> >>
>> >> so how do you feel about compressing 2 blocks into 16bits of checksum :-/?
>> >
>> > Hrm.... is it generally the case that both block and exclude bitmaps are loaded
>> > at the same time?  I don't think it'd be difficult to checksum both blocks
>> > unless it's common that one of the two are not in memory.
>> >
>>
>> Hrm indeed. no. exclude bitmap is currently not loaded always together with
>> block bitmap, so unless this is changed, we'll have to think of something else.
>
> Or... does it make sense to preload the exclude bitmap at the same time as the
> block bitmap?

it wouldn't be terrible to do that, since exclude bitmap will normally
be located
right next to block bitmap, but if memory pressure is high, that would
lead to half
the number of block bitmaps in memory at any given time.

> If I'm reading you correctly, (block_bitmap | exclude_bitmap)
> yields a bitmap of all blocks that are not available, correct?

not exactly. if all is sane, then
(block_bitmap | exclude_bitmap) == block_bitmap
(block_bitmap & exclude_bitmap) == exclude_bitmap

> So in the case
> that exclude bitmaps are enabled, you'd need both to be in memory anyway.
>

exclude_bitmap is loaded into memory in the following cases:
1. allocating snapshot inode blocks (during COW)
2. deleting snapshot inode blocks (snapshot delete)
3. moving "deleted" blocks to snapshot (MOW)

specifically, exclude_bitmap is NOT loaded into memory in the following cases:
1. allocating regular inode blocks
2. deleting regular inode blocks, which were allocated after last snapshot

For example, in kernel compilation benchmark, all temp files
are created and deleted without modifying exclude bitmap (or snapshots).

>> However that brings me to wonder:
>> block/inode/exclude bitmap seldom change more than a handful of bits/words
>> at a time.
>> Does it make sense to diff update the CRC of the bitmaps instead of
>> recomputing it?
>
> Yes, that would be a good (later) optimization at least to consider.
>
> --D
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists