Page MenuHomeFreeBSD

Add metadata_csum feature support.
ClosedPublic

Authored by fsu on Jan 9 2018, 10:25 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 1 2024, 9:04 PM
Unknown Object (File)
Dec 20 2023, 3:34 AM
Unknown Object (File)
Oct 24 2023, 11:25 PM
Unknown Object (File)
Sep 5 2023, 5:58 PM
Unknown Object (File)
Sep 5 2023, 5:57 PM
Unknown Object (File)
Sep 5 2023, 5:56 PM
Unknown Object (File)
Aug 31 2023, 7:03 PM
Unknown Object (File)
Aug 20 2023, 4:32 PM
Subscribers

Details

Summary

This patch adds RW support for metadata_csum feature.
All items, excluding MMP, are checksummed.
List of items from ext4 specification:

  • Superblock.
  • MMP => Not supported because the feature is not implemented.
  • Extended Attributes.
  • Directory Entries.
  • HTREE Nodes.
  • Extents.
  • Bitmaps.
  • Inodes.
  • Group Descriptors.
Test Plan

Tested using next script, which use different features sets including metadata_csum.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

A thought : would it be possible to checksum only what we are going to write to disk?
Data will not, generally, corrupt in memory and the kernel has no use for the checksums at runtime so we only need to write stuff when it's going to be converted to the on-disk format in ext2_inode_cnv.c.

In D13810#289543, @pfg wrote:

A thought : would it be possible to checksum only what we are going to write to disk?
Data will not, generally, corrupt in memory and the kernel has no use for the checksums at runtime so we only need to write stuff when it's going to be converted to the on-disk format in ext2_inode_cnv.c.

All fields, which are mentioned in SUMMERY, should be written to disk. If it will not be updated, we will have errors reported by e2fsck, or bunch of error messages from driver on linux side.

In D13810#289828, @fsu wrote:
In D13810#289543, @pfg wrote:

A thought : would it be possible to checksum only what we are going to write to disk?
Data will not, generally, corrupt in memory and the kernel has no use for the checksums at runtime so we only need to write stuff when it's going to be converted to the on-disk format in ext2_inode_cnv.c.

All fields, which are mentioned in SUMMERY, should be written to disk. If it will not be updated, we will have errors reported by e2fsck, or bunch of error messages from driver on linux side.

I am not complaining at all about what is being checksummed: I understand you want to checksum everything that is needed. The main doubt is when to checksum.

There are two possibilities: you either checksum everything as soon as you get it or you checksum at the last moment before writing to disk. Both options have their advantages It seems like the checksumming is happening all over the filesystem so you are doing the former, I was only wondering if we could do all the checksums in a single place, right when we are converting to the ext2fs format for writing.

I am not sure if such thing is poosible though, and your change is not incorrect so I'll let you think about it but I'll approve the patch.

This revision is now accepted and ready to land.Jan 10 2018, 3:44 PM
In D13810#289909, @pfg wrote:
In D13810#289828, @fsu wrote:
In D13810#289543, @pfg wrote:

A thought : would it be possible to checksum only what we are going to write to disk?
Data will not, generally, corrupt in memory and the kernel has no use for the checksums at runtime so we only need to write stuff when it's going to be converted to the on-disk format in ext2_inode_cnv.c.

All fields, which are mentioned in SUMMERY, should be written to disk. If it will not be updated, we will have errors reported by e2fsck, or bunch of error messages from driver on linux side.

I am not complaining at all about what is being checksummed: I understand you want to checksum everything that is needed. The main doubt is when to checksum.

There are two possibilities: you either checksum everything as soon as you get it or you checksum at the last moment before writing to disk. Both options have their advantages It seems like the checksumming is happening all over the filesystem so you are doing the former, I was only wondering if we could do all the checksums in a single place, right when we are converting to the ext2fs format for writing.

I am not sure if such thing is poosible though, and your change is not incorrect so I'll let you think about it but I'll approve the patch.

I use second approach, checksum every field before bwrite() call.
I can not imagine to do it in single place. I can see only cosmetic improvement, which can come on my mind, it is wrap bwrite() calls together with checksumming... Ok I will try to think, may be something useful will appear.

In D13810#289917, @fsu wrote:
In D13810#289909, @pfg wrote:
In D13810#289828, @fsu wrote:
In D13810#289543, @pfg wrote:

A thought : would it be possible to checksum only what we are going to write to disk?
Data will not, generally, corrupt in memory and the kernel has no use for the checksums at runtime so we only need to write stuff when it's going to be converted to the on-disk format in ext2_inode_cnv.c.

All fields, which are mentioned in SUMMERY, should be written to disk. If it will not be updated, we will have errors reported by e2fsck, or bunch of error messages from driver on linux side.

I am not complaining at all about what is being checksummed: I understand you want to checksum everything that is needed. The main doubt is when to checksum.

There are two possibilities: you either checksum everything as soon as you get it or you checksum at the last moment before writing to disk. Both options have their advantages It seems like the checksumming is happening all over the filesystem so you are doing the former, I was only wondering if we could do all the checksums in a single place, right when we are converting to the ext2fs format for writing.

I am not sure if such thing is poosible though, and your change is not incorrect so I'll let you think about it but I'll approve the patch.

I use second approach, checksum every field before bwrite() call.

OK, I see ...

I can not imagine to do it in single place. I can see only cosmetic improvement, which can come on my mind, it is wrap bwrite() calls together with checksumming... Ok I will try to think, may be something useful will appear.

A wrapper is not a good idea, we clearly want to keep the bwrite independent. I was cosidering more thidea of doing the checksuming on ext2_inode_cnv.c. but that only applies to the inode. I guess it is OK as-is.

This revision was automatically updated to reflect the committed changes.