Re: [PATCH v3 02/13] fs: fiemap: update fiemap_fill_next_extent() signature

[Brian Foster <bfoster@redhat.com>]

On Wed, Apr 03, 2024 at 03:22:43AM -0400, Sweet Tea Dorminy wrote:
> 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.
> 
> 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(-)
> 
...
> 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 = 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 = logical;
>  	extent.fe_physical = phys;
> -	extent.fe_logical_length = len;
> -	extent.fe_physical_length = len;
> +	extent.fe_logical_length = log_len;
> +	extent.fe_physical_length = phys_len;

Another nit, but would it simplify things to let this helper set the
_HAS_PHYS_LEN flag on phys_len != 0 or something rather than require
callers to get it right?

Brian

>  	extent.fe_flags = flags;
>  
>  	dest += 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,
>  
>  	return fiemap_fill_next_extent(fi, iomap->offset,
>  			iomap->addr != IOMAP_NULL_ADDR ? iomap->addr : 0,
> -			iomap->length, flags);
> +			iomap->length, 0, flags);
>  }
>  
>  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 = 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 |= FIEMAP_EXTENT_LAST;
>  
>  				ret = fiemap_fill_next_extent(
> -					fieinfo, logical, phys, size, flags);
> +					fieinfo, logical, phys, size, 0, flags);
>  				if (ret)
>  					break;
>  				size = 0;
> @@ -1256,7 +1256,7 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  					/* Terminate the current extent */
>  					ret = fiemap_fill_next_extent(
>  						fieinfo, logical, phys, size,
> -						flags);
> +						0, flags);
>  					if (ret || blkoff > end_blkoff)
>  						break;
>  
> 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 = 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 |= FIEMAP_EXTENT_LAST;
>  
>  			err = fiemap_fill_next_extent(fieinfo, vbo, lbo, dlen,
> -						      flags);
> +						      0, flags);
>  			if (err < 0)
>  				break;
>  			if (err == 1) {
> @@ -2062,7 +2062,8 @@ int ni_fiemap(struct ntfs_inode *ni, struct fiemap_extent_info *fieinfo,
>  		if (vbo + bytes >= end)
>  			flags |= FIEMAP_EXTENT_LAST;
>  
> -		err = fiemap_fill_next_extent(fieinfo, vbo, lbo, bytes, flags);
> +		err = fiemap_fill_next_extent(fieinfo, vbo, lbo, bytes, 0,
> +					      flags);
>  		if (err < 0)
>  			break;
>  		if (err == 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);
>  
>  		ret = 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 = (u64)le32_to_cpu(rec.e_cpos) << osb->s_clustersize_bits;
>  
>  		ret = fiemap_fill_next_extent(fieinfo, virt_bytes, phys_bytes,
> -					      len_bytes, fe_flags);
> +					      len_bytes, 0, fe_flags);
>  		if (ret)
>  			break;
>  
> 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);
>  
>  #endif /* _LINUX_FIEMAP_H 1 */
> -- 
> 2.43.0
> 
>