Received-SPF: pass (google.com: domain of linux-kernel+bounces-133495-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Andreas Dilger <adilger@dilger.ca>
Message-Id: <BDD29EBF-3A5F-4241-B9F2-789605D99817@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_B18047D7-F546-4BB3-8288-3143CAF3C184";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH v3 02/13] fiemap: update fiemap_fill_next_extent()
 signature
Date: Fri, 5 Apr 2024 13:05:01 -0600
In-Reply-To: <58f9c9eef8b0e33a8d46a3ad8a8db46890e1fbe8.1712126039.git.sweettea-kernel@dorminy.me>
Cc: 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 Mailing List <linux-kernel@vger.kernel.org>,
 linux-bcachefs@vger.kernel.org,
 linux-btrfs <linux-btrfs@vger.kernel.org>,
 linux-f2fs-devel@lists.sourceforge.net,
 linux-fsdevel <linux-fsdevel@vger.kernel.org>,
 kernel-team@meta.com
To: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
References: <cover.1712126039.git.sweettea-kernel@dorminy.me>
 <58f9c9eef8b0e33a8d46a3ad8a8db46890e1fbe8.1712126039.git.sweettea-kernel@dorminy.me>


--Apple-Mail=_B18047D7-F546-4BB3-8288-3143CAF3C184
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Apr 3, 2024, at 1:22 AM, Sweet Tea Dorminy =
<sweettea-kernel@dorminy.me> wrote:
>=20
> Update the signature of fiemap_fill_next_extent() to allow passing a
> physical length. Update all callers to pass a 0 physical length -- =
since
> none set the EXTENT_HAS_PHYS_LEN flag, this value doesn't matter.

Patch-structure-wise, it doesn't make sense to me to change all of the =
callers
to pass "0" as the argument to this function, and then submit a whole =
series
of per-filesystem patches that sets only FIEMAP_EXTENT_HAS_PHYS_LEN (but =
also
passes phys_len =3D 0, which is wrong AFAICS).

A cleaner approach would be to rename the existing =
fiemap_fill_next_extent()
to fiemap_fill_next_extent_phys() that takes phys_len as an argument, =
and then
add a simple wrapper until all of the filesystems are updated:

#define fiemap_fill_next_extent(info, logical, phys, log_len, flags, =
dev) \
   fiemap_fill_next_extent_phys(info, logical, phys, log_len, 0, flags, =
dev)

Then the per-filesystem patches would involve changing over the callers =
to
use fiemap_fill_next_extent_phys() and setting =
FIEMAP_EXTENT_HAS_PHYS_LEN.


It would be possible in fiemap_fill_next_extent_phys() to add something =
like
the following in this patch that is adding the phys_len argument:

        if (phys_len =3D=3D 0 && !(flags & FIEMAP_EXTENT_HAS_PHYS_LEN)) =
{
                phys_len =3D log_len;
                flags |=3D FIEMAP_EXTENT_HAS_PHYS_LEN;
        }

which would immediately enable the fe_phyisical_length argument for all =
of
the callers.  This would reduce the chance of errors by the filesystems, =
or
avoid the need to change the filesystems at all unless they want to set
phys_len and log_len differently for compression.

Cheers, Andreas

>=20
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
> Documentation/filesystems/fiemap.rst | 3 ++-
> fs/bcachefs/fs.c                     | 7 ++++---
> fs/btrfs/extent_io.c                 | 4 ++--
> fs/ext4/extents.c                    | 1 +
> fs/f2fs/data.c                       | 8 +++++---
> fs/f2fs/inline.c                     | 3 ++-
> fs/ioctl.c                           | 9 +++++----
> fs/iomap/fiemap.c                    | 2 +-
> fs/nilfs2/inode.c                    | 6 +++---
> fs/ntfs3/frecord.c                   | 7 ++++---
> fs/ocfs2/extent_map.c                | 4 ++--
> fs/smb/client/smb2ops.c              | 1 +
> include/linux/fiemap.h               | 2 +-
> 13 files changed, 33 insertions(+), 24 deletions(-)
>=20
> diff --git a/Documentation/filesystems/fiemap.rst =
b/Documentation/filesystems/fiemap.rst
> index c2bfa107c8d7..c060bb83f5d8 100644
> --- a/Documentation/filesystems/fiemap.rst
> +++ b/Documentation/filesystems/fiemap.rst
> @@ -236,7 +236,8 @@ For each extent in the request range, the file =
system should call
> the helper function, fiemap_fill_next_extent()::
>=20
>   int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 =
logical,
> -			      u64 phys, u64 len, u32 flags, u32 dev);
> +			      u64 phys, u64 log_len, u64 phys_len, u32 =
flags,
> +                              u32 dev);
>=20
> fiemap_fill_next_extent() will use the passed values to populate the
> next free extent in the fm_extents array. 'General' extent flags will
> diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> index 71013256fc39..f830578a9cd1 100644
> --- a/fs/bcachefs/fs.c
> +++ b/fs/bcachefs/fs.c
> @@ -931,7 +931,8 @@ static int bch2_fill_extent(struct bch_fs *c,
> 			ret =3D fiemap_fill_next_extent(info,
> 						bkey_start_offset(k.k) =
<< 9,
> 						offset << 9,
> -						k.k->size << 9, =
flags|flags2);
> +						k.k->size << 9, 0,
> +						flags|flags2);
> 			if (ret)
> 				return ret;
> 		}
> @@ -940,13 +941,13 @@ static int bch2_fill_extent(struct bch_fs *c,
> 	} else if (bkey_extent_is_inline_data(k.k)) {
> 		return fiemap_fill_next_extent(info,
> 					       bkey_start_offset(k.k) << =
9,
> -					       0, k.k->size << 9,
> +					       0, k.k->size << 9, 0,
> 					       flags|
> 					       =
FIEMAP_EXTENT_DATA_INLINE);
> 	} else if (k.k->type =3D=3D KEY_TYPE_reservation) {
> 		return fiemap_fill_next_extent(info,
> 					       bkey_start_offset(k.k) << =
9,
> -					       0, k.k->size << 9,
> +					       0, k.k->size << 9, 0,
> 					       flags|
> 					       FIEMAP_EXTENT_DELALLOC|
> 					       FIEMAP_EXTENT_UNWRITTEN);
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index eceef5ff780b..9e421d99fd5c 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2577,7 +2577,7 @@ static int flush_fiemap_cache(struct =
fiemap_extent_info *fieinfo,
> 		int ret;
>=20
> 		ret =3D fiemap_fill_next_extent(fieinfo, entry->offset,
> -					      entry->phys, entry->len,
> +					      entry->phys, entry->len, =
0,
> 					      entry->flags);
> 		/*
> 		 * Ignore 1 (reached max entries) because we keep track =
of that
> @@ -2793,7 +2793,7 @@ static int emit_last_fiemap_cache(struct =
fiemap_extent_info *fieinfo,
> 		return 0;
>=20
> 	ret =3D fiemap_fill_next_extent(fieinfo, cache->offset, =
cache->phys,
> -				      cache->len, cache->flags);
> +				      cache->len, 0, cache->flags);
> 	cache->cached =3D false;
> 	if (ret > 0)
> 		ret =3D 0;
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index e57054bdc5fd..2adade3c202a 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2215,6 +2215,7 @@ static int ext4_fill_es_cache_info(struct inode =
*inode,
> 				(__u64)es.es_lblk << blksize_bits,
> 				(__u64)es.es_pblk << blksize_bits,
> 				(__u64)es.es_len << blksize_bits,
> +				0,
> 				flags);
> 		if (next =3D=3D 0)
> 			break;
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index d9494b5fc7c1..87f8d828e038 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1834,7 +1834,8 @@ static int f2fs_xattr_fiemap(struct inode =
*inode,
> 		if (!xnid)
> 			flags |=3D FIEMAP_EXTENT_LAST;
>=20
> -		err =3D fiemap_fill_next_extent(fieinfo, 0, phys, len, =
flags);
> +		err =3D fiemap_fill_next_extent(
> +				fieinfo, 0, phys, len, 0, flags);
> 		trace_f2fs_fiemap(inode, 0, phys, len, flags, err);
> 		if (err)
> 			return err;
> @@ -1860,7 +1861,8 @@ static int f2fs_xattr_fiemap(struct inode =
*inode,
> 	}
>=20
> 	if (phys) {
> -		err =3D fiemap_fill_next_extent(fieinfo, 0, phys, len, =
flags);
> +		err =3D fiemap_fill_next_extent(
> +				fieinfo, 0, phys, len, 0, flags);
> 		trace_f2fs_fiemap(inode, 0, phys, len, flags, err);
> 	}
>=20
> @@ -1979,7 +1981,7 @@ int f2fs_fiemap(struct inode *inode, struct =
fiemap_extent_info *fieinfo,
> 			flags |=3D FIEMAP_EXTENT_DATA_ENCRYPTED;
>=20
> 		ret =3D fiemap_fill_next_extent(fieinfo, logical,
> -				phys, size, flags);
> +				phys, size, 0, flags);
> 		trace_f2fs_fiemap(inode, logical, phys, size, flags, =
ret);
> 		if (ret)
> 			goto out;
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index ac00423f117b..49d2f87fe048 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -806,7 +806,8 @@ int f2fs_inline_data_fiemap(struct inode *inode,
> 	byteaddr =3D (__u64)ni.blk_addr << =
inode->i_sb->s_blocksize_bits;
> 	byteaddr +=3D (char *)inline_data_addr(inode, ipage) -
> 					(char *)F2FS_INODE(ipage);
> -	err =3D fiemap_fill_next_extent(fieinfo, start, byteaddr, ilen, =
flags);
> +	err =3D fiemap_fill_next_extent(
> +			fieinfo, start, byteaddr, ilen, 0, flags);
> 	trace_f2fs_fiemap(inode, start, byteaddr, ilen, flags, err);
> out:
> 	f2fs_put_page(ipage, 1);
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 8afd32e1a27a..1830baca532b 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -99,7 +99,8 @@ static int ioctl_fibmap(struct file *filp, int =
__user *p)
>  * @fieinfo:	Fiemap context passed into ->fiemap
>  * @logical:	Extent logical start offset, in bytes
>  * @phys:	Extent physical start offset, in bytes
> - * @len:	Extent length, in bytes
> + * @log_len:	Extent logical length, in bytes
> + * @phys_len:	Extent physical length, in bytes (optional)
>  * @flags:	FIEMAP_EXTENT flags that describe this extent
>  *
>  * Called from file system ->fiemap callback. Will populate extent
> @@ -110,7 +111,7 @@ static int ioctl_fibmap(struct file *filp, int =
__user *p)
>  * extent that will fit in user array.
>  */
> int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 =
logical,
> -			    u64 phys, u64 len, u32 flags)
> +			    u64 phys, u64 log_len, u64 phys_len, u32 =
flags)
> {
> 	struct fiemap_extent extent;
> 	struct fiemap_extent __user *dest =3D fieinfo->fi_extents_start;
> @@ -138,8 +139,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_logical_length =3D len;
> -	extent.fe_physical_length =3D len;
> +	extent.fe_logical_length =3D log_len;
> +	extent.fe_physical_length =3D phys_len;
> 	extent.fe_flags =3D flags;
>=20
> 	dest +=3D fieinfo->fi_extents_mapped;
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index 610ca6f1ec9b..013e843c8d10 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -36,7 +36,7 @@ static int iomap_to_fiemap(struct fiemap_extent_info =
*fi,
>=20
> 	return fiemap_fill_next_extent(fi, iomap->offset,
> 			iomap->addr !=3D IOMAP_NULL_ADDR ? iomap->addr : =
0,
> -			iomap->length, flags);
> +			iomap->length, 0, flags);
> }
>=20
> static loff_t iomap_fiemap_iter(const struct iomap_iter *iter,
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index 7340a01d80e1..4d3c347c982b 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -1190,7 +1190,7 @@ int nilfs_fiemap(struct inode *inode, struct =
fiemap_extent_info *fieinfo,
> 			if (size) {
> 				/* End of the current extent */
> 				ret =3D fiemap_fill_next_extent(
> -					fieinfo, logical, phys, size, =
flags);
> +					fieinfo, logical, phys, size, 0, =
flags);
> 				if (ret)
> 					break;
> 			}
> @@ -1240,7 +1240,7 @@ int nilfs_fiemap(struct inode *inode, struct =
fiemap_extent_info *fieinfo,
> 					flags |=3D FIEMAP_EXTENT_LAST;
>=20
> 				ret =3D fiemap_fill_next_extent(
> -					fieinfo, logical, phys, size, =
flags);
> +					fieinfo, logical, phys, size, 0, =
flags);
> 				if (ret)
> 					break;
> 				size =3D 0;
> @@ -1256,7 +1256,7 @@ int nilfs_fiemap(struct inode *inode, struct =
fiemap_extent_info *fieinfo,
> 					/* Terminate the current extent =
*/
> 					ret =3D fiemap_fill_next_extent(
> 						fieinfo, logical, phys, =
size,
> -						flags);
> +						0, flags);
> 					if (ret || blkoff > end_blkoff)
> 						break;
>=20
> diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
> index 7f27382e0ce2..ef0ed913428b 100644
> --- a/fs/ntfs3/frecord.c
> +++ b/fs/ntfs3/frecord.c
> @@ -1947,7 +1947,7 @@ int ni_fiemap(struct ntfs_inode *ni, struct =
fiemap_extent_info *fieinfo,
> 	if (!attr || !attr->non_res) {
> 		err =3D fiemap_fill_next_extent(
> 			fieinfo, 0, 0,
> -			attr ? le32_to_cpu(attr->res.data_size) : 0,
> +			attr ? le32_to_cpu(attr->res.data_size) : 0, 0,
> 			FIEMAP_EXTENT_DATA_INLINE | FIEMAP_EXTENT_LAST |
> 				FIEMAP_EXTENT_MERGED);
> 		goto out;
> @@ -2042,7 +2042,7 @@ int ni_fiemap(struct ntfs_inode *ni, struct =
fiemap_extent_info *fieinfo,
> 				flags |=3D FIEMAP_EXTENT_LAST;
>=20
> 			err =3D fiemap_fill_next_extent(fieinfo, vbo, =
lbo, dlen,
> -						      flags);
> +						      0, flags);
> 			if (err < 0)
> 				break;
> 			if (err =3D=3D 1) {
> @@ -2062,7 +2062,8 @@ int ni_fiemap(struct ntfs_inode *ni, struct =
fiemap_extent_info *fieinfo,
> 		if (vbo + bytes >=3D end)
> 			flags |=3D FIEMAP_EXTENT_LAST;
>=20
> -		err =3D fiemap_fill_next_extent(fieinfo, vbo, lbo, =
bytes, flags);
> +		err =3D fiemap_fill_next_extent(fieinfo, vbo, lbo, =
bytes, 0,
> +					      flags);
> 		if (err < 0)
> 			break;
> 		if (err =3D=3D 1) {
> diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
> index 70a768b623cf..eabdf97cd685 100644
> --- a/fs/ocfs2/extent_map.c
> +++ b/fs/ocfs2/extent_map.c
> @@ -723,7 +723,7 @@ static int ocfs2_fiemap_inline(struct inode =
*inode, struct buffer_head *di_bh,
> 					 id2.i_data.id_data);
>=20
> 		ret =3D fiemap_fill_next_extent(fieinfo, 0, phys, =
id_count,
> -					      flags);
> +					      0, flags);
> 		if (ret < 0)
> 			return ret;
> 	}
> @@ -794,7 +794,7 @@ int ocfs2_fiemap(struct inode *inode, struct =
fiemap_extent_info *fieinfo,
> 		virt_bytes =3D (u64)le32_to_cpu(rec.e_cpos) << =
osb->s_clustersize_bits;
>=20
> 		ret =3D fiemap_fill_next_extent(fieinfo, virt_bytes, =
phys_bytes,
> -					      len_bytes, fe_flags);
> +					      len_bytes, 0, fe_flags);
> 		if (ret)
> 			break;
>=20
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 87b63f6ad2e2..23a193512f96 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -3779,6 +3779,7 @@ static int smb3_fiemap(struct cifs_tcon *tcon,
> 				le64_to_cpu(out_data[i].file_offset),
> 				le64_to_cpu(out_data[i].file_offset),
> 				le64_to_cpu(out_data[i].length),
> +				0,
> 				flags);
> 		if (rc < 0)
> 			goto out;
> diff --git a/include/linux/fiemap.h b/include/linux/fiemap.h
> index c50882f19235..17a6c32cdf3f 100644
> --- a/include/linux/fiemap.h
> +++ b/include/linux/fiemap.h
> @@ -16,6 +16,6 @@ struct fiemap_extent_info {
> int fiemap_prep(struct inode *inode, struct fiemap_extent_info =
*fieinfo,
> 		u64 start, u64 *len, u32 supported_flags);
> int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 =
logical,
> -			    u64 phys, u64 len, u32 flags);
> +			    u64 phys, u64 log_len, u64 phys_len, u32 =
flags);
>=20
> #endif /* _LINUX_FIEMAP_H 1 */
> --
> 2.43.0
>=20
>=20


Cheers, Andreas


--Apple-Mail=_B18047D7-F546-4BB3-8288-3143CAF3C184
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+AFAmYQS10ACgkQcqXauRfM
H+DZLxAAlJo/2IlGKAQFhRORGteq1SC3CeTXMaw8zigT87v3L/nSocfglYY1IEBA
7rfwHBBBNU5M948fSD9ltzMn/yTHO3FcucB3pHPVZ4AvKaz0PgsKIg61Qku83Ypx
FdRD9YXWAv5mpYfeABRrUoUGPkxMxAha/IK2BKE9zg6WxlK2CHPUtsPCgDT9eMdw
Qmdmau6v2sDn4sYTzB7TyCBx+4NrTxWQNRP3BTVSN1VKYbguVim0Qvuaw+Soq1zB
JR7inoHDjLZ2oDIxa0AUroTQO8vAWAGNy1868pdRCa4bthXDRJjQhDXZmRnn7PWP
yGavQddXPpUKqb0bGJXgrU6pAgeUdquLvChcQcRR0I37EChMZwlxN2YqCk+qFkhC
iSv5awLT72KNf+i1o3HM3Hn/+ztdtyTJpRVW9P4bxgeWRPlQYKFr8UPjXljAcTIv
jU8+c6JXGcuiFdfScFCoVW2sA4+EgF+QyqtghubPs9IVHxteJwll+9cPddIHI//8
SRgo65RFc811yGAdoLv0pEISrjTLG0kDTjoiXFXMwE60dB0gz9lbdmqkBr6DJkJE
bzyMxwGml6sdOUHJtm0Z84SDsOpRcyRvr1pHu7HyQ6bq4uE2eJqR7SwLyhKYaREX
DeTkbswx8/UaHADZ7SKxuRXJlWg1jfPc3zlznyCXDJlci8XCj/I=
=vehB
-----END PGP SIGNATURE-----

--Apple-Mail=_B18047D7-F546-4BB3-8288-3143CAF3C184--