On Apr 9, 2024, at 10:22 AM, Darrick J. Wong <[email protected]> wrote: > > On Wed, Apr 03, 2024 at 03:22:42AM -0400, Sweet Tea Dorminy wrote: >> Some filesystems support compressed extents which have a larger logical >> size than physical, and for those filesystems, it can be useful for >> userspace to know how much space those extents actually use. For >> instance, the compsize [1] tool for btrfs currently uses btrfs-internal, >> root-only ioctl to find the actual disk space used by a file; it would >> be better and more useful for this information to require fewer >> privileges and to be usable on more filesystems. Therefore, use one of >> the padding u64s in the fiemap extent structure to return the actual >> physical length; and, for now, return this as equal to the logical >> length. >> >> [1] https://github.com/kilobyte/compsize >> >> Signed-off-by: Sweet Tea Dorminy <[email protected]> >> --- >> Documentation/filesystems/fiemap.rst | 28 +++++++++++++++++------- >> fs/ioctl.c | 3 ++- >> include/uapi/linux/fiemap.h | 32 ++++++++++++++++++++++------ >> 3 files changed, 47 insertions(+), 16 deletions(-) >> >> diff --git a/Documentation/filesystems/fiemap.rst >> b/Documentation/filesystems/fiemap.rst >> index 93fc96f760aa..c2bfa107c8d7 100644 >> --- a/Documentation/filesystems/fiemap.rst >> +++ b/Documentation/filesystems/fiemap.rst >> @@ -80,14 +80,24 @@ Each extent is described by a single fiemap_extent >> structure as >> returned in fm_extents:: >> >> struct fiemap_extent { >> - __u64 fe_logical; /* logical offset in bytes for the start of >> - * the extent */ >> - __u64 fe_physical; /* physical offset in bytes for the start >> - * of the extent */ >> - __u64 fe_length; /* length in bytes for the extent */ >> - __u64 fe_reserved64[2]; >> - __u32 fe_flags; /* FIEMAP_EXTENT_* flags for this extent */ >> - __u32 fe_reserved[3]; >> + /* >> + * logical offset in bytes for the start of >> + * the extent from the beginning of the file >> + */ >> + __u64 fe_logical; >> + /* >> + * physical offset in bytes for the start >> + * of the extent from the beginning of the disk >> + */ >> + __u64 fe_physical; >> + /* logical length in bytes for this extent */ >> + __u64 fe_logical_length; >> + /* physical length in bytes for this extent */ >> + __u64 fe_physical_length; >> + __u64 fe_reserved64[1]; >> + /* FIEMAP_EXTENT_* flags for this extent */ >> + __u32 fe_flags; >> + __u32 fe_reserved[3]; >> }; >> >> All offsets and lengths are in bytes and mirror those on disk. It is valid >> @@ -175,6 +185,8 @@ FIEMAP_EXTENT_MERGED >> userspace would be highly inefficient, the kernel will try to merge most >> adjacent blocks into 'extents'. >> >> +FIEMAP_EXTENT_HAS_PHYS_LEN >> + This will be set if the file system populated the physical length field. > > Just out of curiosity, should filesystems set this flag and > fe_physical_length if fe_physical_length == fe_logical_length? > Or just leave both blank?
In the original thread, Dave thought it would be better to always set fe_physical_length and the flag, so that userspace applications which do not properly check the flag will not be confused/broken by differences in filesystem behavior in the future when this is in use. https://patchwork.ozlabs.org/project/linux-ext4/patch/4f8d5dc5b51a43efaf16c39398c23a6276e40a30.1386778303.git.dste...@suse.cz/ > >> VFS -> File System Implementation >> --------------------------------- >> diff --git a/fs/ioctl.c b/fs/ioctl.c >> index 661b46125669..8afd32e1a27a 100644 >> --- a/fs/ioctl.c >> +++ b/fs/ioctl.c >> @@ -138,7 +138,8 @@ int fiemap_fill_next_extent(struct fiemap_extent_info >> *fieinfo, u64 logical, >> memset(&extent, 0, sizeof(extent)); >> extent.fe_logical = logical; >> extent.fe_physical = phys; >> - extent.fe_length = len; >> + extent.fe_logical_length = len; >> + extent.fe_physical_length = len; >> extent.fe_flags = flags; >> >> dest += fieinfo->fi_extents_mapped; >> diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h >> index 24ca0c00cae3..3079159b8e94 100644 >> --- a/include/uapi/linux/fiemap.h >> +++ b/include/uapi/linux/fiemap.h >> @@ -14,14 +14,30 @@ >> >> #include <linux/types.h> >> >> +/* >> + * For backward compatibility, where the member of the struct was called >> + * fe_length instead of fe_logical_length. >> + */ >> +#define fe_length fe_logical_length > > This #define has global scope; are you sure this isn't going to cause a > weird build problem downstream with some program that declares an > unrelated fe_length symbol? I guess it's possible. I'm not dead set on this part of the change. I thought it was cleaner to separate the two in the struct, but I can see the argument that a UAPI field struct should not change names. It would be possible to have: #define fe_logical_length fe_length which would have much less chance of namespace collisions I think. New applications can start to use this for some years, before making a permanent switch, but again not something I'm stuck on... Cheers, Andreas >> + >> struct fiemap_extent { >> - __u64 fe_logical; /* logical offset in bytes for the start of >> - * the extent from the beginning of the file */ >> - __u64 fe_physical; /* physical offset in bytes for the start >> - * of the extent from the beginning of the disk */ >> - __u64 fe_length; /* length in bytes for this extent */ >> - __u64 fe_reserved64[2]; >> - __u32 fe_flags; /* FIEMAP_EXTENT_* flags for this extent */ >> + /* >> + * logical offset in bytes for the start of >> + * the extent from the beginning of the file >> + */ >> + __u64 fe_logical; >> + /* >> + * physical offset in bytes for the start >> + * of the extent from the beginning of the disk >> + */ >> + __u64 fe_physical; >> + /* logical length in bytes for this extent */ >> + __u64 fe_logical_length; > > Or why not just leave the field name the same since the "logical length > in bytes" comment is present both here in the header and again in the > documentation? > > --D > >> + /* physical length in bytes for this extent */ >> + __u64 fe_physical_length; >> + __u64 fe_reserved64[1]; >> + /* FIEMAP_EXTENT_* flags for this extent */ >> + __u32 fe_flags; >> __u32 fe_reserved[3]; >> }; >> >> @@ -66,5 +82,7 @@ struct fiemap { >> * merged for efficiency. */ >> #define FIEMAP_EXTENT_SHARED 0x00002000 /* Space shared with other >> * files. */ >> +#define FIEMAP_EXTENT_HAS_PHYS_LEN 0x00004000 /* Physical length is valid >> + * and set by FS. */ >> >> #endif /* _UAPI_LINUX_FIEMAP_H */ >> -- >> 2.43.0 >> >> > Cheers, Andreas
signature.asc
Description: Message signed with OpenPGP
