On Fri, Sep 02, 2011 at 04:02:17PM -0600, Andreas Dilger wrote:
> On 2011-09-02, at 1:32 PM, Darrick J. Wong wrote:
> > On Wed, Aug 31, 2011 at 08:30:25PM -0600, Andreas Dilger wrote:
> >> On 2011-08-31, at 6:31 PM, Darrick J. Wong wrote:
> >>> This patch introduces to ext4 the ability to calculate and verify inode
> >>> checksums. This requires the use of a new ro compatibility flag and some
> >>> accompanying e2fsprogs patches to provide the relevant features in tune2fs and e2fsck.
> >>>
> >>>
> >>> +static __le32 ext4_inode_csum(struct inode *inode, struct ext4_inode *raw)
> >>> +{
> >>> + struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> >>> + struct ext4_inode_info *ei = EXT4_I(inode);
> >>> + int offset = offsetof(struct ext4_inode, i_checksum);
> >>
> >> This could be declared "const int" so that it is not consuming space on
> >> the stack, or just put it inline in the code instead of a stack variable
> >> since it is a compile time constant.
> >>
> >>> + __le32 inum = cpu_to_le32(inode->i_ino);
> >>> + __u32 crc = 0;
> >>> +
> >>> + if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
> >>> + cpu_to_le32(EXT4_OS_LINUX))
> >>
> >> This can be marked unlikely() I think.
> >
> > Ok.
> >
> >>> + return 0;
> >>> + if (!EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> >>> + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> >>> + return 0;
> >>> +
> >>> + crc = crc32c_le(~0, sbi->s_es->s_uuid, sizeof(sbi->s_es->s_uuid));
> >>> + crc = crc32c_le(crc, (__u8 *)&inum, sizeof(inum));
> >>
> >> I wonder if it makes sense to pre-compute the crc32c of s_uuid (stored
> >> in sbi) and/or s_uuid+inum (stored in struct ext4_inode_info). I suspect
> >> precomputing the s_uuid checksum is worthwhile, but I'm not sure whether
> >> precomputing the inode checksum is worthwhile unless it doesn't reduce
> >> the number of ext4_inode_info structs per page in the slab.
> >
> > Sounds like a good idea, I'll look into it.
>
> Looking more closely at the cryptoapi code, I'm fairly confident that
> storing the partial crc32c for the uuid+inum+generation into the inode
> is going to be worthwhile, compared to calling crc32c_le() 3 extra times.
Hmm, can the FS UUID change while the FS is mounted? Or, to look at this from
the other side, does anyone mind if tune2fs -U can tell you to umount before
changing UUID?
I think we need that anyway, to prevent races between tune2fs checksum rewrite
and kernel writing stuff.
I found a bug where if you mount a fs and write to it, then dumpe2fs -h will
complain about superblock checksum errors. Will have to look into that...
> >>> + crc = crc32c_le(crc, (__u8 *)raw, offset);
> >>> + offset += sizeof(raw->i_checksum); /* skip checksum */
> >>> + crc = crc32c_le(crc, (__u8 *)raw + offset,
> >>> + EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize -
> >>> + offset);
> >>
> >> I suspect it would be more efficient to set raw->i_checksum = 0, then
> >> compute the checksum on the whole raw inode buffer, and fill in
> >> raw->i_checksum = cpu_to_le32(crc) at the end. That would mean the
> >> caller ext4_inode_csum_verify() should save the original checksum for
> >> comparison with the returned value.
> >
> > You mean to avoid the overhead of the add/store and the second function call?
>
> Mostly the overhead of the extra calls into crc32c_le() and the cryptoapi.
> There are a lot of extra pointer indirections in that code, and calling
> into cryptoapi for 4-byte values adds (vaguely) 60-100 operations per word
> on top of the actual checksum operations, unless it all disappears at
> compile time (hard to see at first glance).
>
> >> The one problem with this is that it is racy w.r.t other users
> >
> > Yeah, I was thinking that if I move the *_csum_set() calls to a jbd2 callback
> > (for journal mode, obviously) then this might clash with that. Maybe a better
> > approach would be to calculate/verify an entire block's worth of inodes at a
> > time. Then again, if you only want to touch /one/ inode out of a whole block,
> > that's a lot of unnecessary work.
>
> However, if you are doing that from the jbd2 callback, the code also has
> exclusive control over the buffer at that time, so computing the checksum
> on the zeroed bytes in a single pass is not racy, and would definitely be
> less overhead for such a small number of bytes.
>
> >>> + return cpu_to_le32(crc);
> >>> +}
> >>> +
> >>> +static int ext4_inode_csum_verify(struct inode *inode, struct ext4_inode *raw)
> >>> +{
> >>> + if (EXT4_SB(inode->i_sb)->s_es->s_creator_os ==
> >>> + cpu_to_le32(EXT4_OS_LINUX) &&
> >>> + EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> >>> + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
> >>> + (raw->i_checksum != ext4_inode_csum(inode, raw)))
> >>
> >> This check can be marked unlikely(), since the rare case of a checksum
> >> failure can cause a stall in the execution pipeline. It might make sense
> >> to put the unlikely() at the lone callsite to move the whole function call
> >> overhead out-of-line.
> >
> > I suppose so, both for this and for all the other _verify() functions.
>
> Right.
>
> >>> + return 0;
> >>> + return 1;
> >>> +}
> >>> +
> >>> +static void ext4_inode_csum_set(struct inode *inode, struct ext4_inode *raw)
> >>> +{
> >>> + if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
> >>> + cpu_to_le32(EXT4_OS_LINUX) ||
> >>> + !EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> >>> + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> >>> + return;
> >>> +
> >>> + raw->i_checksum = ext4_inode_csum(inode, raw);
> >>> +}
> >>> +
> >>> static inline int ext4_begin_ordered_truncate(struct inode *inode,
> >>> loff_t new_size)
> >>> {
> >>> @@ -3410,6 +3458,15 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
> >>> if (ret < 0)
> >>> goto bad_inode;
> >>> raw_inode = ext4_raw_inode(&iloc);
> >>> +
> >>> + if (!ext4_inode_csum_verify(inode, raw_inode)) {
> >>> + EXT4_ERROR_INODE(inode, "checksum invalid (0x%x != 0x%x)",
> >>> + le32_to_cpu(ext4_inode_csum(inode, raw_inode)),
> >>> + le32_to_cpu(raw_inode->i_checksum));
> >>> + ret = -EIO;
> >>> + goto bad_inode;
> >>> + }
> >>> +
> >>> inode->i_mode = le16_to_cpu(raw_inode->i_mode);
> >>> inode->i_uid = (uid_t)le16_to_cpu(raw_inode->i_uid_low);
> >>> inode->i_gid = (gid_t)le16_to_cpu(raw_inode->i_gid_low);
> >>> @@ -3490,6 +3547,9 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
> >>> ei->i_extra_isize = le16_to_cpu(raw_inode->i_extra_isize);
> >>> if (EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize >
> >>> EXT4_INODE_SIZE(inode->i_sb)) {
> >>> + EXT4_ERROR_INODE(inode, "bad extra_isize (%u != %u)",
> >>> + EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize,
> >>> + EXT4_INODE_SIZE(inode->i_sb));
> >>> ret = -EIO;
> >>> goto bad_inode;
> >>> }
> >>> @@ -3731,6 +3791,8 @@ static int ext4_do_update_inode(handle_t *handle,
> >>> raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
> >>> }
> >>>
> >>> + ext4_inode_csum_set(inode, raw_inode);
> >>
> >> This might warrant a comment to always be the last function before
> >> submitting the inode to the journal.
> >
> > Ok.
> >
> >>> BUFFER_TRACE(bh, "call ext4_handle_dirty_metadata");
> >>> rc = ext4_handle_dirty_metadata(handle, NULL, bh);
> >>> if (!err)
> >>
> >> Also, rather than just making the checksum be updated at commit time, it
> >> makes more sense to have ext4_do_update_inode() only be called once per
> >> commit, since this is an expensive function.
> >
> > If I made jbd2 responsible for calling back into ext4 to apply checksums just
> > prior to submit_bh()ing metadata blocks, I think that would take care of this.
>
> Yes, that would be the most desirable case, but it also means that the
> journal code needs to pin all of these inodes in memory until after it
> commits. Possibly the new ext4 ordered journal mode already does this,
> but not sure about other journal modes.
>
> I definitely like the idea of using the jbd2 pre-commit callbacks, but
> I don't think it is necessarily needed for the first version of the
> patches. Better to get the "simple" implementation working correctly
> (so that we are sure it is doing the right thing), and then migrate it
> over to using the commit callbacks so that we can verify it is still
> correct.
I wasn't planning to start on this optimization until I finish addressing all
the other comments/complaints.
--D
>
> Cheers, Andreas--
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
