| Message ID | 20221101190031.6766-3-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | file-posix: alignment probing improvements | expand |
On Tue, Nov 01, 2022 at 03:00:31PM -0400, Stefan Hajnoczi wrote: > /* Let's try to use the logical blocksize for the alignment. */ > - if (probe_logical_blocksize(fd, &bs->bl.request_alignment) < 0) { > - bs->bl.request_alignment = 0; > + if (!bs->bl.request_alignment) { > + if (probe_logical_blocksize(fd, &bs->bl.request_alignment) < 0) { > + bs->bl.request_alignment = 0; > + } > } > > #ifdef __linux__ > - /* > - * The XFS ioctl definitions are shipped in extra packages that might > - * not always be available. Since we just need the XFS_IOC_DIOINFO ioctl > - * here, we simply use our own definition instead: > - */ > - struct xfs_dioattr { > - uint32_t d_mem; > - uint32_t d_miniosz; > - uint32_t d_maxiosz; > - } da; > - if (ioctl(fd, _IOR('X', 30, struct xfs_dioattr), &da) >= 0) { > - bs->bl.request_alignment = da.d_miniosz; > - /* The kernel returns wrong information for d_mem */ > - /* s->buf_align = da.d_mem; */ > + if (!bs->bl.request_alignment) { This patch changes the fallback code to make the request_alignment value from probe_logical_blocksize() override the value from XFS_IOC_DIOINFO. Is that intentional? > + if (ioctl(fd, _IOR('X', 30, struct xfs_dioattr), &da) >= 0) { > + bs->bl.request_alignment = da.d_miniosz; > + /* The kernel returns wrong information for d_mem */ > + /* s->buf_align = da.d_mem; */ Has this bug been reported to the XFS developers (linux-xfs@vger.kernel.org)? - Eric
On Tue, Nov 01, 2022 at 08:32:30PM -0700, Eric Biggers wrote: > On Tue, Nov 01, 2022 at 03:00:31PM -0400, Stefan Hajnoczi wrote: > > /* Let's try to use the logical blocksize for the alignment. */ > > - if (probe_logical_blocksize(fd, &bs->bl.request_alignment) < 0) { > > - bs->bl.request_alignment = 0; > > + if (!bs->bl.request_alignment) { > > + if (probe_logical_blocksize(fd, &bs->bl.request_alignment) < 0) { > > + bs->bl.request_alignment = 0; > > + } > > } > > > > #ifdef __linux__ > > - /* > > - * The XFS ioctl definitions are shipped in extra packages that might > > - * not always be available. Since we just need the XFS_IOC_DIOINFO ioctl > > - * here, we simply use our own definition instead: > > - */ > > - struct xfs_dioattr { > > - uint32_t d_mem; > > - uint32_t d_miniosz; > > - uint32_t d_maxiosz; > > - } da; > > - if (ioctl(fd, _IOR('X', 30, struct xfs_dioattr), &da) >= 0) { > > - bs->bl.request_alignment = da.d_miniosz; > > - /* The kernel returns wrong information for d_mem */ > > - /* s->buf_align = da.d_mem; */ > > + if (!bs->bl.request_alignment) { > > This patch changes the fallback code to make the request_alignment value from > probe_logical_blocksize() override the value from XFS_IOC_DIOINFO. Is that > intentional? Thanks for pointing out the bug. That was not intentional. Will fix. > > + if (ioctl(fd, _IOR('X', 30, struct xfs_dioattr), &da) >= 0) { > > + bs->bl.request_alignment = da.d_miniosz; > > + /* The kernel returns wrong information for d_mem */ > > + /* s->buf_align = da.d_mem; */ > > Has this bug been reported to the XFS developers (linux-xfs@vger.kernel.org)? Paolo: Do you remember if you reported this when you wrote commit c25f53b06eba ("raw: Probe required direct I/O alignment")? Stefan
diff --git a/block/file-posix.c b/block/file-posix.c index b9d62f52fe..00d8191880 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -372,28 +372,48 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) bs->bl.request_alignment = 0; s->buf_align = 0; + +#if defined(__linux__) && defined(STATX_DIOALIGN) + struct statx stx; + + /* + * Linux 6.1 introduced an interface for both block devices and file + * systems. The system call returns with the STATX_DIOALIGN bit cleared + * when the information is unavailable. + */ + if (statx(fd, "", AT_EMPTY_PATH, STATX_DIOALIGN, &stx) == 0 && + (stx.stx_mask & STATX_DIOALIGN)) { + bs->bl.request_alignment = stx.stx_dio_offset_align; + s->buf_align = stx.stx_dio_mem_align; + } +#endif /* defined(__linux__) && defined(STATX_DIOALIGN) */ + /* Let's try to use the logical blocksize for the alignment. */ - if (probe_logical_blocksize(fd, &bs->bl.request_alignment) < 0) { - bs->bl.request_alignment = 0; + if (!bs->bl.request_alignment) { + if (probe_logical_blocksize(fd, &bs->bl.request_alignment) < 0) { + bs->bl.request_alignment = 0; + } } #ifdef __linux__ - /* - * The XFS ioctl definitions are shipped in extra packages that might - * not always be available. Since we just need the XFS_IOC_DIOINFO ioctl - * here, we simply use our own definition instead: - */ - struct xfs_dioattr { - uint32_t d_mem; - uint32_t d_miniosz; - uint32_t d_maxiosz; - } da; - if (ioctl(fd, _IOR('X', 30, struct xfs_dioattr), &da) >= 0) { - bs->bl.request_alignment = da.d_miniosz; - /* The kernel returns wrong information for d_mem */ - /* s->buf_align = da.d_mem; */ + if (!bs->bl.request_alignment) { + /* + * The XFS ioctl definitions are shipped in extra packages that might + * not always be available. Since we just need the XFS_IOC_DIOINFO ioctl + * here, we simply use our own definition instead: + */ + struct xfs_dioattr { + uint32_t d_mem; + uint32_t d_miniosz; + uint32_t d_maxiosz; + } da; + if (ioctl(fd, _IOR('X', 30, struct xfs_dioattr), &da) >= 0) { + bs->bl.request_alignment = da.d_miniosz; + /* The kernel returns wrong information for d_mem */ + /* s->buf_align = da.d_mem; */ + } } -#endif +#endif /* __linux__ */ /* * If we could not get the sizes so far, we can only guess them. First try
Linux commit 825cf206ed51 ("statx: add direct I/O alignment information") added an interface to fetch O_DIRECT alignment values for block devices and file systems. Prefer STATX_DIOALIGN to older interfaces and probing, but keep them as fallbacks in case STATX_DIOALIGN cannot provide the information. Testing shows the status of STATX_DIOALIGN support in Linux 6.1-rc3 appears to be: - btrfs: no - ext4: yes - XFS: yes - NVMe block devices: yes - dm-crypt: yes Cc: Eric Biggers <ebiggers@google.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/file-posix.c | 54 +++++++++++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 17 deletions(-)