[PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr

[PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr

[Christoph Hellwig]

Use the direct I/O alignment reporting from ->getattr instead of
reimplementing it.  This exposes the relaxation of the memory
alignment in the XFS_IOC_DIOINFO info and ensure the information will
stay in sync.  Note that randholes.c in xfstests has a bug where it
incorrectly fails when the required memory alignment is smaller than the
pointer size.  Round up the reported value as there is a fair chance that
this code got copied into various applications.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_ioctl.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index e1051a530a50..21ae68896caa 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1209,21 +1209,24 @@ xfs_file_ioctl(
 				current->comm);
 		return -ENOTTY;
 	case XFS_IOC_DIOINFO: {
-		struct xfs_buftarg	*target = xfs_inode_buftarg(ip);
+		struct kstat		st;
 		struct dioattr		da;
 
-		da.d_mem = target->bt_logical_sectorsize;
+		error = vfs_getattr(&filp->f_path, &st, STATX_DIOALIGN, 0);
+		if (error)
+			return error;
 
 		/*
-		 * See xfs_report_dioalign() for an explanation about why this
-		 * reports a value larger than the sector size for COW inodes.
+		 * The randholes tool in xfstests expects the alignment to not
+		 * be smaller than the size of a pointer for whatever reason.
+		 *
+		 * Align the report value to that so that the dword (4 byte)
+		 * alignment supported by many storage devices doesn't trip it
+		 * up.
 		 */
-		if (xfs_is_cow_inode(ip))
-			da.d_miniosz = xfs_inode_alloc_unitsize(ip);
-		else
-			da.d_miniosz = target->bt_logical_sectorsize;
+		da.d_mem = roundup(st.dio_mem_align, sizeof(void *));
+		da.d_miniosz = st.dio_offset_align;
 		da.d_maxiosz = INT_MAX & ~(da.d_miniosz - 1);
-
 		if (copy_to_user(arg, &da, sizeof(da)))
 			return -EFAULT;
 		return 0;
-- 
2.47.2

Re: [PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr

[Darrick J. Wong]

On Mon, Aug 18, 2025 at 07:13:43AM +0200, Christoph Hellwig wrote:
> Use the direct I/O alignment reporting from ->getattr instead of
> reimplementing it.  This exposes the relaxation of the memory
> alignment in the XFS_IOC_DIOINFO info and ensure the information will
> stay in sync.  Note that randholes.c in xfstests has a bug where it
> incorrectly fails when the required memory alignment is smaller than the
> pointer size.  Round up the reported value as there is a fair chance that
> this code got copied into various applications.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_ioctl.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index e1051a530a50..21ae68896caa 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1209,21 +1209,24 @@ xfs_file_ioctl(
>  				current->comm);
>  		return -ENOTTY;
>  	case XFS_IOC_DIOINFO: {
> -		struct xfs_buftarg	*target = xfs_inode_buftarg(ip);
> +		struct kstat		st;
>  		struct dioattr		da;
>  
> -		da.d_mem = target->bt_logical_sectorsize;
> +		error = vfs_getattr(&filp->f_path, &st, STATX_DIOALIGN, 0);
> +		if (error)
> +			return error;
>  
>  		/*
> -		 * See xfs_report_dioalign() for an explanation about why this
> -		 * reports a value larger than the sector size for COW inodes.
> +		 * The randholes tool in xfstests expects the alignment to not
> +		 * be smaller than the size of a pointer for whatever reason.

Userspace is grossssss

But I do see the value in not having two implementations of the directio
geometry gathering since we already had bugs wrt that vs. statx.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> +		 *
> +		 * Align the report value to that so that the dword (4 byte)
> +		 * alignment supported by many storage devices doesn't trip it
> +		 * up.
>  		 */
> -		if (xfs_is_cow_inode(ip))
> -			da.d_miniosz = xfs_inode_alloc_unitsize(ip);
> -		else
> -			da.d_miniosz = target->bt_logical_sectorsize;
> +		da.d_mem = roundup(st.dio_mem_align, sizeof(void *));
> +		da.d_miniosz = st.dio_offset_align;
>  		da.d_maxiosz = INT_MAX & ~(da.d_miniosz - 1);
> -
>  		if (copy_to_user(arg, &da, sizeof(da)))
>  			return -EFAULT;
>  		return 0;
> -- 
> 2.47.2
> 
> 

Re: [PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr

[Carlos Maiolino]

On Mon, Aug 18, 2025 at 07:13:43AM +0200, Christoph Hellwig wrote:
> Use the direct I/O alignment reporting from ->getattr instead of
> reimplementing it.  This exposes the relaxation of the memory
> alignment in the XFS_IOC_DIOINFO info and ensure the information will
> stay in sync.  Note that randholes.c in xfstests has a bug where it
> incorrectly fails when the required memory alignment is smaller than the
> pointer size.  Round up the reported value as there is a fair chance that
> this code got copied into various applications.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_ioctl.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index e1051a530a50..21ae68896caa 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1209,21 +1209,24 @@ xfs_file_ioctl(
>  				current->comm);
>  		return -ENOTTY;
>  	case XFS_IOC_DIOINFO: {
> -		struct xfs_buftarg	*target = xfs_inode_buftarg(ip);
> +		struct kstat		st;
>  		struct dioattr		da;
> 
> -		da.d_mem = target->bt_logical_sectorsize;
> +		error = vfs_getattr(&filp->f_path, &st, STATX_DIOALIGN, 0);
> +		if (error)
> +			return error;
> 
>  		/*
> -		 * See xfs_report_dioalign() for an explanation about why this
> -		 * reports a value larger than the sector size for COW inodes.
> +		 * The randholes tool in xfstests expects the alignment to not
> +		 * be smaller than the size of a pointer for whatever reason.
> +		 *

Do we need to keep this comment that tied to an userspace tool? It just
looks weird to have a comment about alignment constraints changes for a single
tool.

The issue with randholes is that it uses posix_memalign, and the pointer
size constraint comes from that.

I couldn't find any details on why this is required, but I'm assuming
it's to keep posix_memalign architecture/implementation independent?!

So, perhaps instead of being 'randholes' specific, it should specify to
be posix compliant or because posix requires this way?


Otherwise it looks good to me
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


> +		 * Align the report value to that so that the dword (4 byte)
> +		 * alignment supported by many storage devices doesn't trip it
> +		 * up.

>  		 */
> -		if (xfs_is_cow_inode(ip))
> -			da.d_miniosz = xfs_inode_alloc_unitsize(ip);
> -		else
> -			da.d_miniosz = target->bt_logical_sectorsize;
> +		da.d_mem = roundup(st.dio_mem_align, sizeof(void *));
> +		da.d_miniosz = st.dio_offset_align;
>  		da.d_maxiosz = INT_MAX & ~(da.d_miniosz - 1);
> -
>  		if (copy_to_user(arg, &da, sizeof(da)))
>  			return -EFAULT;
>  		return 0;
> --
> 2.47.2
> 

Re: [PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr

[Christoph Hellwig]

On Wed, Aug 20, 2025 at 10:23:27AM +0200, Carlos Maiolino wrote:
> Do we need to keep this comment that tied to an userspace tool?

It think it is a pretty good reminder why it is here.

> The issue with randholes is that it uses posix_memalign, and the pointer
> size constraint comes from that.
> 
> I couldn't find any details on why this is required, but I'm assuming
> it's to keep posix_memalign architecture/implementation independent?!
> 
> So, perhaps instead of being 'randholes' specific, it should specify to
> be posix compliant or because posix requires this way?

Posix does not require the alignment to be larger than void *.
Applications that directly feed the value to posix_memalign do.
And maybe that what could go into the comment.

Re: [PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr

[Carlos Maiolino]

On Thu, Aug 21, 2025 at 10:40:37AM +0200, Christoph Hellwig wrote:
> On Wed, Aug 20, 2025 at 10:23:27AM +0200, Carlos Maiolino wrote:
> > Do we need to keep this comment that tied to an userspace tool?
> 
> It think it is a pretty good reminder why it is here.

Fair enough. I'm not opposing to it, just looks weird to me.

> 
> > The issue with randholes is that it uses posix_memalign, and the pointer
> > size constraint comes from that.
> >
> > I couldn't find any details on why this is required, but I'm assuming
> > it's to keep posix_memalign architecture/implementation independent?!
> >
> > So, perhaps instead of being 'randholes' specific, it should specify to
> > be posix compliant or because posix requires this way?
> 
> Posix does not require the alignment to be larger than void *.

Sorry, I'm not sure if I got what you mean here, perhaps I phrased it
wrong, but I didn't mean to infer posix requires an alignment larger
than void*, but that posix_memalign requires the 'alignment' to be a
multiple of sizeof(void*). Although the smallest alignment, well, would
be sizeof(void*) per se.

FWIW, I'm not questioning your patch anymore, I'm just curious about
these posix constraints.

> Applications that directly feed the value to posix_memalign do.
> And maybe that what could go into the comment.

yeah, that would be nice to have.

Cheers.