On May 29, 2025, at 1:12 AM, Anuj Gupta/Anuj Gupta <anuj20.g@xxxxxxxxxxx> wrote:
> +/* Protection info capability flags */
> +#define FILE_PI_CAP_INTEGRITY (1 << 0)
> +#define FILE_PI_CAP_REFTAG (1 << 1)
> +
> +/* Checksum types for Protection Information */
> +#define FS_PI_CSUM_NONE 0
> +#define FS_PI_CSUM_IP 1
> +#define FS_PI_CSUM_CRC 2
> +#define FS_PI_CSUM_CRC64 3
> +
> +/*
> + * struct fs_pi_cap - protection information(PI) capability descriptor
> + * @flags: Bitmask of capability flags
> + * @interval: Number of bytes of data per PI tuple
> + * @csum_type: Checksum type
> + * @metadata_size: Size in bytes of the metadata associated with each
> interval
> + * @tag_size: Size of the tag area within the tuple
> + * @pi_offset: Offset of protection information tuple within the metadata
> + * @ref_tag_size: Size in bytes of the reference tag
> + * @storage_tag_size: Size in bytes of the storage tag
> + * @rsvd: Reserved for future use
> + */
> +struct fs_pi_cap {
> + __u32 flags;
Minor nits on the struct.
It would be preferable to have a struct prefix on these fields, like "fpc_"
so that tags for "flags" don't return a million different structs.
> + __u16 interval;
> + __u8 csum_type;
> + __u8 tuple_size;
> + __u8 tag_size;
> + __u8 pi_offset;
> + __u8 ref_tag_size;
> + __u8 storage_tag_size;
> + __u8 rsvd[4];
> +};
It seems strange to have padding to align this struct to a 20-byte size.
Having 4 bytes of padding is probably insufficient for future expansion
(e.g. just a single int if that was needed), and 20 bytes isn't exactly
a "normal" power-of-two size.
Since ioctls take the size of the struct, you could either remove rsvd
entirely, and use the struct size to "version" the ioctl if it needs to
change (at the cost of consuming more ioctls), or expand the struct to
be large enough to allow proper expansion in the future, like 32 bytes
with fpc_rsvd[24] (using "flags" to indicate the validity of the new fields).
> #define FS_IOC_GETFSSYSFSPATH _IOR(0x15, 1, struct fs_sysfs_path)
> +/* Get protection info capability details */
> +#define FS_IOC_GETPICAP _IOR('f', 3, struct fs_pi_cap)
Note that _IOR('f', 3, int) is already used as EXT4_IOC32_GETVERSION, though
this does not strictly conflict because of the different struct size.
At a minimum, FS_IOC_GETPICAP should be declared right after FS_IOC_SETFLAGS
_IOR('f', 2,) so that it is clear that _IOR('f', 3,) is used.
However, in Documentation/userspace-api/ioctl/ioctl-number.rst it reports
that 'f' 00-1f is reserved for ext4, while 0x15 00-ff is reserved for generic
FS_IOC_* ioctls, so that would be the better one to use. Currently only
_IOR(0x15, 0,) and _IOR(0x15, 1,) are used, so _IOR(0x15, 3) should be safe.
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
