Received-SPF: pass (google.com: domain of linux-kernel+bounces-137560-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Andreas Dilger <adilger@dilger.ca>
Message-Id: <32378212-7A10-4562-8670-3F0FB7126F9B@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_ECA1B460-583E-424F-86F4-A01D2C16C3A3";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH v3 01/13] fs: fiemap: add physical_length field to extents
Date: Tue, 9 Apr 2024 13:50:08 -0600
In-Reply-To: <20240409162232.GA6367@frogsfrogsfrogs>
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>,
 =?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?= <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
To: "Darrick J. Wong" <djwong@kernel.org>
References: <cover.1712126039.git.sweettea-kernel@dorminy.me>
 <1ba5bfccccbf4ff792f178268badde056797d0c4.1712126039.git.sweettea-kernel@dorminy.me>
 <20240409162232.GA6367@frogsfrogsfrogs>


--Apple-Mail=_ECA1B460-583E-424F-86F4-A01D2C16C3A3
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Apr 9, 2024, at 10:22 AM, Darrick J. Wong <djwong@kernel.org> wrote:
>=20
> 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.
>>=20
>> [1] https://github.com/kilobyte/compsize
>>=20
>> 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(-)
>>=20
>> 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::
>>=20
>>     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];
>>     };
>>=20
>> 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'.
>>=20
>> +FIEMAP_EXTENT_HAS_PHYS_LEN
>> +  This will be set if the file system populated the physical length =
field.
>=20
> Just out of curiosity, should filesystems set this flag and
> fe_physical_length if fe_physical_length =3D=3D 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/4f8d5dc5b51a43efaf16=
c39398c23a6276e40a30.1386778303.git.dsterba@suse.cz/

>=20
>> 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 =3D logical;
>> 	extent.fe_physical =3D phys;
>> -	extent.fe_length =3D len;
>> +	extent.fe_logical_length =3D len;
>> +	extent.fe_physical_length =3D len;
>> 	extent.fe_flags =3D flags;
>>=20
>> 	dest +=3D 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 @@
>>=20
>> #include <linux/types.h>
>>=20
>> +/*
>> + * For backward compatibility, where the member of the struct was =
called
>> + * fe_length instead of fe_logical_length.
>> + */
>> +#define fe_length fe_logical_length
>=20
> 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;
>=20
> 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?
>=20
> --D
>=20
>> +	/* 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];
>> };
>>=20
>> @@ -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. */
>>=20
>> #endif /* _UAPI_LINUX_FIEMAP_H */
>> --
>> 2.43.0
>>=20
>>=20
>=20


Cheers, Andreas


--Apple-Mail=_ECA1B460-583E-424F-86F4-A01D2C16C3A3
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename=signature.asc
Content-Type: application/pgp-signature;
	name=signature.asc
Content-Description: Message signed with OpenPGP

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org

iQIzBAEBCAAdFiEEDb73u6ZejP5ZMprvcqXauRfMH+AFAmYVm/AACgkQcqXauRfM
H+CxqRAAu2r0TTDsoXra17ylZesykQhj4IRUya7tEvmLkYpkPw2xkXPWHYjE18n9
I9ZcaDWdTabEUTDMG6ljdzbyYNJUqscMo3QqX+r9juPamhs95sSX7ZSct4EmDsTg
4vy0dE1z/gSICqhnnHC2CK4YGGcjDCr3wd29HS1QakiLtVPKNTTAb95JSaOgrokn
PQp9fge1bF4J8sfIeSC4pxd8fqnNo79HK7v0knS5LN9R8UhdYu+vqIOIE7OJHKol
ivzNeaMzRUXlwDfv/xxF5tYaEJLty20oVRR1LEjDz/caaox9CtfSJ0WsObka4Bzp
Oo7epgfbyrqhJps8vUH0u344YRxhwdJFMM4eC5CU/CyZ9KV3THjYh4Eh+H5c8giB
vsAwd8qqk1VI7XtrlZuYdnyGwmmJQSPdDFiJbLukzJC332UWsOUXbgVZ7Boxcbsc
s5Mnd/jFmx1QEYIT6pgF0AXzsCVO21vyKJiVtVJFwWiNxvyyKxZ/f2PWHjUtUe7l
G134UbM3qYKg5xn/C6yt6/uH5QYdoqM2QLjv3AemnYejt6gmMYv+VREYfQenz27a
pkXV9d8dKxaL99pqIdYvl8K4z/EboYZkcHlC6F5oRtaFI7mvURapN+3n3KXfxmJT
Di2YKi7MZg1mpxYAbzLS5YIhrpMCx1BGg3j4GJYdZSAqdQFJZuw=
=bJQI
-----END PGP SIGNATURE-----

--Apple-Mail=_ECA1B460-583E-424F-86F4-A01D2C16C3A3--