| 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: <32378212-7A10-4562-8670-3F0FB7126F9B@dilger.ca>
Date: Tue, 9 Apr 2024 13:50:08 -0600
From: Andreas Dilger <adilger@...ger.ca>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: Sweet Tea Dorminy <sweettea-kernel@...miny.me>,
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 Apr 9, 2024, at 10:22 AM, Darrick J. Wong <djwong@...nel.org> 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 <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?
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.dsterba@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
Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)
Powered by blists - more mailing lists