* [PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr @ 2025-08-18 5:13 ` Christoph Hellwig 2025-08-18 20:47 ` Darrick J. Wong 2025-08-20 8:23 ` Carlos Maiolino 0 siblings, 2 replies; 5+ messages in thread From: Christoph Hellwig @ 2025-08-18 5:13 UTC (permalink / raw) To: cem; +Cc: linux-xfs 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr 2025-08-18 5:13 ` [PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr Christoph Hellwig @ 2025-08-18 20:47 ` Darrick J. Wong 2025-08-20 8:23 ` Carlos Maiolino 1 sibling, 0 replies; 5+ messages in thread From: Darrick J. Wong @ 2025-08-18 20:47 UTC (permalink / raw) To: Christoph Hellwig; +Cc: cem, linux-xfs 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr 2025-08-18 5:13 ` [PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr Christoph Hellwig 2025-08-18 20:47 ` Darrick J. Wong @ 2025-08-20 8:23 ` Carlos Maiolino 2025-08-21 8:40 ` Christoph Hellwig 1 sibling, 1 reply; 5+ messages in thread From: Carlos Maiolino @ 2025-08-20 8:23 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr 2025-08-20 8:23 ` Carlos Maiolino @ 2025-08-21 8:40 ` Christoph Hellwig 2025-08-21 10:59 ` Carlos Maiolino 0 siblings, 1 reply; 5+ messages in thread From: Christoph Hellwig @ 2025-08-21 8:40 UTC (permalink / raw) To: Carlos Maiolino; +Cc: Christoph Hellwig, linux-xfs 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr 2025-08-21 8:40 ` Christoph Hellwig @ 2025-08-21 10:59 ` Carlos Maiolino 0 siblings, 0 replies; 5+ messages in thread From: Carlos Maiolino @ 2025-08-21 10:59 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-08-21 10:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <9aG8Tf3X2d-4A9_uy7q50gPfuQH-xjOf3Bdbw4mJ5ITHbBXXDwYG2uqAYoSKE-pRy5iYgqRbd79paOGW-Sk_SA==@protonmail.internalid>
2025-08-18 5:13 ` [PATCH] xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr Christoph Hellwig
2025-08-18 20:47 ` Darrick J. Wong
2025-08-20 8:23 ` Carlos Maiolino
2025-08-21 8:40 ` Christoph Hellwig
2025-08-21 10:59 ` Carlos Maiolino
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox