| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240409162232.GA6367@frogsfrogsfrogs>
Date: Tue, 9 Apr 2024 09:22:32 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: Sweet Tea Dorminy <sweettea-kernel@...miny.me>
Cc: Jonathan Corbet <corbet@....net>,
Kent Overstreet <kent.overstreet@...ux.dev>,
Brian Foster <bfoster@...hat.com>, Chris Mason <clm@...com>,
Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>,
Jaegeuk Kim <jaegeuk@...nel.org>, Chao Yu <chao@...nel.org>,
Alexander Viro <viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
Mickaël Salaün <mic@...ikod.net>,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-bcachefs@...r.kernel.org, linux-btrfs@...r.kernel.org,
linux-f2fs-devel@...ts.sourceforge.net,
linux-fsdevel@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH v3 01/13] fs: fiemap: add physical_length field to extents
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 <sweettea-kernel@...miny.me>
> ---
> 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?
> 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?
> +
> 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
>
>
Powered by blists - more mailing lists