| Message ID | 20241002164500.2775775-1-maharmstone@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix comments in definition of struct btrfs_file_extent_item | expand |
On Wed, Oct 02, 2024 at 05:44:45PM +0100, Mark Harmstone wrote: > The comments in the definition of struct btrfs_file_extent_item were > written while the FS was still in flux, and are no longer accurate. > > The range [disk_bytenr, disk_num_bytes) is the same as the extent in the > extent tree. There's no difference here between csummed and non-csummed > extents, as the comments were implying. And the fields offset and > num_bytes are in bytes, not file blocks. > > Signed-off-by: Mark Harmstone <maharmstone@fb.com> Reviewed-by: Boris Burkov <boris@bur.io> > --- > include/uapi/linux/btrfs_tree.h | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h > index fc29d273845d..5df54a11c74c 100644 > --- a/include/uapi/linux/btrfs_tree.h > +++ b/include/uapi/linux/btrfs_tree.h > @@ -1094,24 +1094,23 @@ struct btrfs_file_extent_item { > __u8 type; > > /* > - * disk space consumed by the extent, checksum blocks are included > - * in these numbers > + * The address and size of the referenced extent. These should exactly > + * match an entry in the extent tree. > * > * At this offset in the structure, the inline extent data start. > */ > __le64 disk_bytenr; > __le64 disk_num_bytes; > /* > - * the logical offset in file blocks (no csums) > - * this extent record is for. This allows a file extent to point > - * into the middle of an existing extent on disk, sharing it > - * between two snapshots (useful if some bytes in the middle of the > - * extent have changed > + * The logical offset in bytes this extent record is for. > + * This allows a file extent to point into the middle of an existing > + * extent on disk, sharing it between two snapshots (useful if some > + * bytes in the middle of the extent have changed) > */ > __le64 offset; > /* > - * the logical number of file blocks (no csums included). This > - * always reflects the size uncompressed and without encoding. > + * The logical number of bytes. This always reflects the size > + * uncompressed and without encoding. > */ > __le64 num_bytes; > > -- > 2.44.2 >
在 2024/10/3 02:14, Mark Harmstone 写道: > The comments in the definition of struct btrfs_file_extent_item were > written while the FS was still in flux, and are no longer accurate. > > The range [disk_bytenr, disk_num_bytes) is the same as the extent in the > extent tree. There's no difference here between csummed and non-csummed > extents, as the comments were implying. And the fields offset and > num_bytes are in bytes, not file blocks. > > Signed-off-by: Mark Harmstone <maharmstone@fb.com> > --- > include/uapi/linux/btrfs_tree.h | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h > index fc29d273845d..5df54a11c74c 100644 > --- a/include/uapi/linux/btrfs_tree.h > +++ b/include/uapi/linux/btrfs_tree.h > @@ -1094,24 +1094,23 @@ struct btrfs_file_extent_item { > __u8 type; > > /* > - * disk space consumed by the extent, checksum blocks are included > - * in these numbers > + * The address and size of the referenced extent. These should exactly > + * match an entry in the extent tree. Recently I'm also helping Han Yuwei to understand all the ondisk format. Maybe he can provide a better advice from a new respective. And he is definitely not happy with the docs and comments on those structures. > * > * At this offset in the structure, the inline extent data start. > */ > __le64 disk_bytenr; > __le64 disk_num_bytes; > /* > - * the logical offset in file blocks (no csums) > - * this extent record is for. This allows a file extent to point > - * into the middle of an existing extent on disk, sharing it > - * between two snapshots (useful if some bytes in the middle of the > - * extent have changed > + * The logical offset in bytes this extent record is for. > + * This allows a file extent to point into the middle of an existing > + * extent on disk, sharing it between two snapshots (useful if some > + * bytes in the middle of the extent have changed) Maybe you want to add the offset is for the uncompressed data. Another thing is, maybe we want to have a more consistent wording. The word "extent record" may be a little confusing, I guess you mean "file extent". Since the structure is called "btrfs_file_extent_item", we may want to unify to "file extent" when referring to the file extent, and "data extent" to refer to the data extent. Thanks, Qu > */ > __le64 offset; > /* > - * the logical number of file blocks (no csums included). This > - * always reflects the size uncompressed and without encoding. > + * The logical number of bytes. This always reflects the size > + * uncompressed and without encoding. > */ > __le64 num_bytes; >
在 2024/10/3 05:21, Qu Wenruo 写道: > > > 在 2024/10/3 02:14, Mark Harmstone 写道: >> The comments in the definition of struct btrfs_file_extent_item were >> written while the FS was still in flux, and are no longer accurate. >> >> The range [disk_bytenr, disk_num_bytes) is the same as the extent in the >> extent tree. There's no difference here between csummed and non-csummed >> extents, as the comments were implying. And the fields offset and >> num_bytes are in bytes, not file blocks. >> >> Signed-off-by: Mark Harmstone <maharmstone@fb.com> >> --- >> include/uapi/linux/btrfs_tree.h | 17 ++++++++--------- >> 1 file changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/ >> btrfs_tree.h >> index fc29d273845d..5df54a11c74c 100644 >> --- a/include/uapi/linux/btrfs_tree.h >> +++ b/include/uapi/linux/btrfs_tree.h >> @@ -1094,24 +1094,23 @@ struct btrfs_file_extent_item { >> __u8 type; >> >> /* >> - * disk space consumed by the extent, checksum blocks are included >> - * in these numbers >> + * The address and size of the referenced extent. These should >> exactly >> + * match an entry in the extent tree. > > Recently I'm also helping Han Yuwei to understand all the ondisk format. > Maybe he can provide a better advice from a new respective. > > And he is definitely not happy with the docs and comments on those > structures. > I didn't read btrfs code in kernel tree, I just read Qu's adam900710/btrfs-fuse for reference, so correct me if I am wrong. If this is EXTENT_DATA(0x6c) in docs, I think the descriptions in btrfs doc is almost ok. >> * >> * At this offset in the structure, the inline extent data start. >> */ >> __le64 disk_bytenr; >> __le64 disk_num_bytes; >> /* >> - * the logical offset in file blocks (no csums) >> - * this extent record is for. This allows a file extent to point >> - * into the middle of an existing extent on disk, sharing it >> - * between two snapshots (useful if some bytes in the middle of the >> - * extent have changed >> + * The logical offset in bytes this extent record is for. >> + * This allows a file extent to point into the middle of an existing >> + * extent on disk, sharing it between two snapshots (useful if some >> + * bytes in the middle of the extent have changed) > > Maybe you want to add the offset is for the uncompressed data. > > Another thing is, maybe we want to have a more consistent wording. > > The word "extent record" may be a little confusing, I guess you mean > "file extent". > Since the structure is called "btrfs_file_extent_item", we may want to > unify to "file extent" when referring to the file extent, and "data > extent" to refer to the data extent. > > Thanks, > Qu >> */ >> __le64 offset; >> /* >> - * the logical number of file blocks (no csums included). This >> - * always reflects the size uncompressed and without encoding. >> + * The logical number of bytes. This always reflects the size >> + * uncompressed and without encoding. >> */ >> __le64 num_bytes; >> My description on these field would be this, disk_bytenr & disk_num_bytes: logical address and size of extent on disk. Should exactly match an EXTENT_ITEM. offset & num_bytes: offset and size of file content within the extent after decoding and decompressing. (removed "logical" since we don't need to do logical -> physical address translation) And write an e.g as doc did. > >
diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h index fc29d273845d..5df54a11c74c 100644 --- a/include/uapi/linux/btrfs_tree.h +++ b/include/uapi/linux/btrfs_tree.h @@ -1094,24 +1094,23 @@ struct btrfs_file_extent_item { __u8 type; /* - * disk space consumed by the extent, checksum blocks are included - * in these numbers + * The address and size of the referenced extent. These should exactly + * match an entry in the extent tree. * * At this offset in the structure, the inline extent data start. */ __le64 disk_bytenr; __le64 disk_num_bytes; /* - * the logical offset in file blocks (no csums) - * this extent record is for. This allows a file extent to point - * into the middle of an existing extent on disk, sharing it - * between two snapshots (useful if some bytes in the middle of the - * extent have changed + * The logical offset in bytes this extent record is for. + * This allows a file extent to point into the middle of an existing + * extent on disk, sharing it between two snapshots (useful if some + * bytes in the middle of the extent have changed) */ __le64 offset; /* - * the logical number of file blocks (no csums included). This - * always reflects the size uncompressed and without encoding. + * The logical number of bytes. This always reflects the size + * uncompressed and without encoding. */ __le64 num_bytes;
The comments in the definition of struct btrfs_file_extent_item were written while the FS was still in flux, and are no longer accurate. The range [disk_bytenr, disk_num_bytes) is the same as the extent in the extent tree. There's no difference here between csummed and non-csummed extents, as the comments were implying. And the fields offset and num_bytes are in bytes, not file blocks. Signed-off-by: Mark Harmstone <maharmstone@fb.com> --- include/uapi/linux/btrfs_tree.h | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)