From: Andreas Dilger <adilger@dilger.ca>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: "Sweet Tea Dorminy" <sweettea-kernel@dorminy.me>,
"Jonathan Corbet" <corbet@lwn.net>,
"Kent Overstreet" <kent.overstreet@linux.dev>,
"Brian Foster" <bfoster@redhat.com>, "Chris Mason" <clm@fb.com>,
"Josef Bacik" <josef@toxicpanda.com>,
"David Sterba" <dsterba@suse.com>,
"Jaegeuk Kim" <jaegeuk@kernel.org>, "Chao Yu" <chao@kernel.org>,
"Alexander Viro" <viro@zeniv.linux.org.uk>,
"Christian Brauner" <brauner@kernel.org>,
"Jan Kara" <jack@suse.cz>, "Mickaël Salaün" <mic@digikod.net>,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-bcachefs@vger.kernel.org, linux-btrfs@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net,
linux-fsdevel@vger.kernel.org, kernel-team@meta.com
Subject: Re: [PATCH v3 01/13] fs: fiemap: add physical_length field to extents
Date: Tue, 9 Apr 2024 13:50:08 -0600 [thread overview]
Message-ID: <32378212-7A10-4562-8670-3F0FB7126F9B@dilger.ca> (raw)
In-Reply-To: <20240409162232.GA6367@frogsfrogsfrogs>
[-- Attachment #1: Type: text/plain, Size: 7078 bytes --]
On Apr 9, 2024, at 10:22 AM, Darrick J. Wong <djwong@kernel.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@dorminy.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
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
next prev parent reply other threads:[~2024-04-09 19:50 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-03 7:22 [PATCH v3 00/13] fiemap extension for more physical information Sweet Tea Dorminy
2024-04-03 7:22 ` [PATCH v3 01/13] fs: fiemap: add physical_length field to extents Sweet Tea Dorminy
2024-04-03 16:57 ` Brian Foster
2024-04-05 18:47 ` [PATCH v3 01/13] " Andreas Dilger
2024-04-09 16:22 ` [PATCH v3 01/13] fs: " Darrick J. Wong
2024-04-09 19:50 ` Andreas Dilger [this message]
2024-04-03 7:22 ` [PATCH v3 02/13] fs: fiemap: update fiemap_fill_next_extent() signature Sweet Tea Dorminy
2024-04-03 16:58 ` Brian Foster
2024-04-05 19:05 ` [PATCH v3 02/13] " Andreas Dilger
2024-04-05 19:06 ` Kent Overstreet
2024-04-03 7:22 ` [PATCH v3 03/13] fs: fiemap: add new COMPRESSED extent state Sweet Tea Dorminy
2024-04-05 19:06 ` [PATCH v3 03/13] " Andreas Dilger
2024-04-03 7:22 ` [PATCH v3 04/13] btrfs: fiemap: emit new COMPRESSED state Sweet Tea Dorminy
2024-04-05 19:10 ` Andreas Dilger
2024-04-03 7:22 ` [PATCH v3 05/13] btrfs: fiemap: return extent physical size Sweet Tea Dorminy
2024-04-03 7:22 ` [PATCH v3 06/13] nilfs2: fiemap: return correct extent physical length Sweet Tea Dorminy
2024-04-05 19:26 ` Andreas Dilger
2024-04-03 7:22 ` [PATCH v3 07/13] ext4: " Sweet Tea Dorminy
2024-04-03 11:22 ` Jan Kara
2024-04-03 7:22 ` [PATCH v3 08/13] f2fs: fiemap: add physical length to trace_f2fs_fiemap Sweet Tea Dorminy
2024-04-05 19:28 ` Andreas Dilger
2024-04-03 7:22 ` [PATCH v3 09/13] f2fs: fiemap: return correct extent physical length Sweet Tea Dorminy
2024-04-03 7:22 ` [PATCH v3 10/13] ocfs2: " Sweet Tea Dorminy
2024-04-03 11:25 ` Jan Kara
2024-04-03 7:22 ` [PATCH v3 11/13] bcachefs: " Sweet Tea Dorminy
2024-04-03 17:00 ` Brian Foster
2024-04-03 18:15 ` Kent Overstreet
2024-04-03 7:22 ` [PATCH v3 12/13] f2fs: fiemap: emit new COMPRESSED state Sweet Tea Dorminy
2024-04-03 7:22 ` [PATCH v3 13/13] bcachefs: " Sweet Tea Dorminy
2024-04-05 19:17 ` Andreas Dilger
2024-04-05 19:34 ` Andreas Dilger
2024-04-06 5:20 ` Kent Overstreet
2024-04-03 8:29 ` [PATCH v3 00/13] fiemap extension for more physical information Gao Xiang
2024-04-03 15:11 ` Sweet Tea Dorminy
2024-04-04 0:43 ` Gao Xiang
2024-04-03 18:17 ` Kent Overstreet
2024-04-03 18:20 ` Darrick J. Wong
2024-04-05 18:20 ` Andreas Dilger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=32378212-7A10-4562-8670-3F0FB7126F9B@dilger.ca \
--to=adilger@dilger.ca \
--cc=bfoster@redhat.com \
--cc=brauner@kernel.org \
--cc=chao@kernel.org \
--cc=clm@fb.com \
--cc=corbet@lwn.net \
--cc=djwong@kernel.org \
--cc=dsterba@suse.com \
--cc=jack@suse.cz \
--cc=jaegeuk@kernel.org \
--cc=josef@toxicpanda.com \
--cc=kent.overstreet@linux.dev \
--cc=kernel-team@meta.com \
--cc=linux-bcachefs@vger.kernel.org \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mic@digikod.net \
--cc=sweettea-kernel@dorminy.me \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).