Add support for long xattr prefix #1
Conversation
Signed-off-by: Derek McGowan <[email protected]>
Signed-off-by: Derek McGowan <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for long xattr prefixes to the EROFS filesystem implementation, which was previously unimplemented and would return an error. The changes include reading and caching long prefixes from the filesystem metadata and updating the xattr parsing logic to handle them.
Key changes:
- Replaces TODO error returns with actual long prefix support in xattr parsing
- Adds caching mechanism for long prefixes with lazy loading from filesystem metadata
- Includes comprehensive test coverage for files with long xattr prefixes
Reviewed Changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| xattr.go | Replaces unimplemented long prefix errors with calls to retrieve cached long prefixes |
| erofs.go | Adds long prefix loading and caching infrastructure with detailed parsing logic |
| erofs_test.go | Adds test cases for files with long xattr prefixes to verify the new functionality |
| README.md | Updates documentation to reflect completed long xattr prefix support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // The packed inode is typically a compact inode (32 bytes) | ||
| // Address = MetaBlkAddr + (PackedNid * 32) + inode_size + (XattrPrefixStart * 4) | ||
| // Note: XattrPrefixStart is in units of 4 bytes from the end of the packed inode | ||
| baseAddr := img.blkOffset() + int64(img.sb.PackedNid)*32 + 32 + int64(img.sb.XattrPrefixStart)*4 |
There was a problem hiding this comment.
The magic number 32 appears twice in this address calculation. Consider defining a constant like packedInodeSize = 32 to make the calculation more self-documenting and maintainable.
| totalLen := 2 + 1 + prefixLen | ||
| if rem := totalLen % 4; rem != 0 { | ||
| padding := 4 - rem |
There was a problem hiding this comment.
The magic numbers 2, 1, and 4 in the alignment calculation should be defined as named constants (e.g., lengthFieldSize = 2, baseIndexSize = 1, alignmentBoundary = 4) to improve code clarity.
| return | ||
| } | ||
|
|
||
| // Long prefixes are stored in the packed inode, after the inode header |
There was a problem hiding this comment.
Hi Derek @dmcgowan, long xattr prefixes (Linux 6.4~6.16)were defined as:
if INCOMPAT_FRAGMENTS is set, the long prefix table is located in the packed inode (XattrPrefixStart * 4);
if INCOMPAT_FRAGMENTS is not set, the long prefix table is directly in the ondisk offset (XattrPrefixStart * 4);
However this way is flew because:
- The packed inode is used to compress unaligned tail data together for example, but some users may not need to compress the long xattr prefixes (although some part of the packed inode can be specified as uncompressed);
- Since metadata compression is introduced (INCOMPAT_METABOX, Linux 6.17+, including this year LTS), a dedicated metadata (metabox) inode is introduced to contain compressed metadata, so long xattr prefixes would be better to be located in the metabox too.
So long xattr prefixes placement is fixed as: https://git.kernel.org/xiang/erofs-utils/c/0cf1039782b2
I admit it was messy initially due to lack of more deeply thoughts when this feature was firstly landed, but it's now fixed.
the implementation can be refered: https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/tree/lib/xattr.c?id=0cf1039782b2#n1680
and
https://git.kernel.org/torvalds/c/1fcf686def19
| // The packed inode is typically a compact inode (32 bytes) | ||
| // Address = MetaBlkAddr + (PackedNid * 32) + inode_size + (XattrPrefixStart * 4) | ||
| // Note: XattrPrefixStart is in units of 4 bytes from the end of the packed inode | ||
| baseAddr := img.blkOffset() + int64(img.sb.PackedNid)*32 + 32 + int64(img.sb.XattrPrefixStart)*4 |
There was a problem hiding this comment.
If long xattr prefixes are located in packed inode or metabox inode, I suggest just using regular inode read logic to read XattrPrefixStart*4 directly, instead of just read data after the inode metadata (because the packed inode can be compressed, or non-inline, etc.)
2 participants
Updates testdata to include long prefixes and related tests. In first commit tests are added and fails. The second commit adds support for the prefixes, reading the entire set and caching the prefixes.