[PATCH] xfs: update XFS_IOC_DIOINFO memory alignment value

[PATCH] xfs: update XFS_IOC_DIOINFO memory alignment value

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

As of v6.0, the DIO memory buffer alignment is no longer aligned to
the logical sector size of the underlying block device. There is now
a specific DMA alignment parameter that memory buffers should be
aligned to. statx(STATX_DIOALIGN) gets this right, but
XFS_IOC_DIOINFO does not - it still uses the older fixed alignment
defined by the block device logical sector size.

This was found because the s390 DASD driver increased DMA alignment
to PAGE_SIZE in commit bc792884b76f ("s390/dasd: Establish DMA
alignment") and DIO aligned to logical sector sizes have started
failing on kernels with that commit. Fixing the "userspace fails
because device alignment constraints increased" issue is not XFS's
problem, but we really should be reporting the correct device memory
alignment in XFS_IOC_DIOINFO.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_ioctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index f0117188f302..71eba4849e03 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1368,7 +1368,8 @@ xfs_file_ioctl(
 		struct xfs_buftarg	*target = xfs_inode_buftarg(ip);
 		struct dioattr		da;
 
-		da.d_mem =  da.d_miniosz = target->bt_logical_sectorsize;
+		da.d_mem = bdev_dma_alignment(target->bt_bdev);
+		da.d_miniosz = target->bt_logical_sectorsize;
 		da.d_maxiosz = INT_MAX & ~(da.d_miniosz - 1);
 
 		if (copy_to_user(arg, &da, sizeof(da)))
-- 
2.45.1

Re: [PATCH] xfs: update XFS_IOC_DIOINFO memory alignment value

[Darrick J. Wong]

On Thu, Jul 11, 2024 at 10:36:37AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> As of v6.0, the DIO memory buffer alignment is no longer aligned to
> the logical sector size of the underlying block device. There is now
> a specific DMA alignment parameter that memory buffers should be
> aligned to. statx(STATX_DIOALIGN) gets this right, but
> XFS_IOC_DIOINFO does not - it still uses the older fixed alignment
> defined by the block device logical sector size.
> 
> This was found because the s390 DASD driver increased DMA alignment
> to PAGE_SIZE in commit bc792884b76f ("s390/dasd: Establish DMA
> alignment") and DIO aligned to logical sector sizes have started
> failing on kernels with that commit. Fixing the "userspace fails
> because device alignment constraints increased" issue is not XFS's
> problem, but we really should be reporting the correct device memory
> alignment in XFS_IOC_DIOINFO.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_ioctl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index f0117188f302..71eba4849e03 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1368,7 +1368,8 @@ xfs_file_ioctl(
>  		struct xfs_buftarg	*target = xfs_inode_buftarg(ip);
>  		struct dioattr		da;
>  
> -		da.d_mem =  da.d_miniosz = target->bt_logical_sectorsize;
> +		da.d_mem = bdev_dma_alignment(target->bt_bdev);

bdev_dma_alignment returns a mask, so I think you want to add one here?

Though at this point, perhaps DIOINFO should query the STATX_DIOALIGN
information so xfs doesn't have to maintain this anymore?

(Or just make a helper that statx and DIOINFO can both call?)

--D

> +		da.d_miniosz = target->bt_logical_sectorsize;
>  		da.d_maxiosz = INT_MAX & ~(da.d_miniosz - 1);
>  
>  		if (copy_to_user(arg, &da, sizeof(da)))
> -- 
> 2.45.1
> 
> 

Re: [PATCH] xfs: update XFS_IOC_DIOINFO memory alignment value

[Christoph Hellwig]

On Wed, Jul 10, 2024 at 07:52:06PM -0700, Darrick J. Wong wrote:
> > -		da.d_mem =  da.d_miniosz = target->bt_logical_sectorsize;
> > +		da.d_mem = bdev_dma_alignment(target->bt_bdev);
> 
> bdev_dma_alignment returns a mask, so I think you want to add one here?

Yes.

> Though at this point, perhaps DIOINFO should query the STATX_DIOALIGN
> information so xfs doesn't have to maintain this anymore?
> 
> (Or just make a helper that statx and DIOINFO can both call?)

Lift DIOINFO to the VFS and back it using the statx data?

Re: [PATCH] xfs: update XFS_IOC_DIOINFO memory alignment value

[Darrick J. Wong]

On Wed, Jul 10, 2024 at 09:03:15PM -0700, Christoph Hellwig wrote:
> On Wed, Jul 10, 2024 at 07:52:06PM -0700, Darrick J. Wong wrote:
> > > -		da.d_mem =  da.d_miniosz = target->bt_logical_sectorsize;
> > > +		da.d_mem = bdev_dma_alignment(target->bt_bdev);
> > 
> > bdev_dma_alignment returns a mask, so I think you want to add one here?
> 
> Yes.
> 
> > Though at this point, perhaps DIOINFO should query the STATX_DIOALIGN
> > information so xfs doesn't have to maintain this anymore?
> > 
> > (Or just make a helper that statx and DIOINFO can both call?)
> 
> Lift DIOINFO to the VFS and back it using the statx data?

<shrug> Is there anything that DIOINFO provides that statx doesn't?
AFAICT XFS is the only fs that implements DIOINFO, so why expand that?

--D

Re: [PATCH] xfs: update XFS_IOC_DIOINFO memory alignment value

[Christoph Hellwig]

On Wed, Jul 10, 2024 at 09:36:14PM -0700, Darrick J. Wong wrote:
> <shrug> Is there anything that DIOINFO provides that statx doesn't?
> AFAICT XFS is the only fs that implements DIOINFO, so why expand that?

Because it'll just make all the existing software using it do the right
thing everywhere?

Re: [PATCH] xfs: update XFS_IOC_DIOINFO memory alignment value

[Dave Chinner]

On Wed, Jul 10, 2024 at 09:44:57PM -0700, Christoph Hellwig wrote:
> On Wed, Jul 10, 2024 at 09:36:14PM -0700, Darrick J. Wong wrote:
> > <shrug> Is there anything that DIOINFO provides that statx doesn't?
> > AFAICT XFS is the only fs that implements DIOINFO, so why expand that?
> 
> Because it'll just make all the existing software using it do the right
> thing everywhere?

I'm just fixing a bug. If you want to make DIOINFO a VFS ioctl, I'll
review the patches but it's way outside the scope of fixing a minor
oversight in a recent feature addition...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: update XFS_IOC_DIOINFO memory alignment value

[Dave Chinner]

On Wed, Jul 10, 2024 at 07:52:06PM -0700, Darrick J. Wong wrote:
> On Thu, Jul 11, 2024 at 10:36:37AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > As of v6.0, the DIO memory buffer alignment is no longer aligned to
> > the logical sector size of the underlying block device. There is now
> > a specific DMA alignment parameter that memory buffers should be
> > aligned to. statx(STATX_DIOALIGN) gets this right, but
> > XFS_IOC_DIOINFO does not - it still uses the older fixed alignment
> > defined by the block device logical sector size.
> > 
> > This was found because the s390 DASD driver increased DMA alignment
> > to PAGE_SIZE in commit bc792884b76f ("s390/dasd: Establish DMA
> > alignment") and DIO aligned to logical sector sizes have started
> > failing on kernels with that commit. Fixing the "userspace fails
> > because device alignment constraints increased" issue is not XFS's
> > problem, but we really should be reporting the correct device memory
> > alignment in XFS_IOC_DIOINFO.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_ioctl.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index f0117188f302..71eba4849e03 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1368,7 +1368,8 @@ xfs_file_ioctl(
> >  		struct xfs_buftarg	*target = xfs_inode_buftarg(ip);
> >  		struct dioattr		da;
> >  
> > -		da.d_mem =  da.d_miniosz = target->bt_logical_sectorsize;
> > +		da.d_mem = bdev_dma_alignment(target->bt_bdev);
> 
> bdev_dma_alignment returns a mask, so I think you want to add one here?

Ah, yes, good eyes, I forgot to refresh the patch. I'll send an
updated version to the list.

> Though at this point, perhaps DIOINFO should query the STATX_DIOALIGN
> information so xfs doesn't have to maintain this anymore?

We open code the STATX_DIOALIGN stuff in xfs_vn_getattr() ourselves
- there's no point using statx to query information we supply statx
with in the first place.

> (Or just make a helper that statx and DIOINFO can both call?)

If we grow more internal users, then maybe?

-Dave.
-- 
Dave Chinner
david@fromorbit.com