Checking for xfsctl() can be done more easily in meson.build. Also,
this is not a "real" feature like the other features that we provide
with the "--enable-xxx" and "--disable-xxx" switches for the
configure script, since this does not influence lots of code (it's
only about one call to xfsctl() in file-posix.c), so people don't
gain much with the ability to disable this with "--disable-xfsctl".
Let's rather treat this like the other cc.has_function() checks in
meson.build, i.e. don't add a new option for this in meson_options.txt.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
configure | 31 -------------------------------
meson.build | 2 +-
2 files changed, 1 insertion(+), 32 deletions(-)
diff --git a/configure b/configure
index 170b1b237a..2296c3e194 100755
--- a/configure
+++ b/configure
@@ -287,7 +287,6 @@ for opt do
done
xen_ctrl_version="$default_feature"
-xfs="$default_feature"
membarrier="$default_feature"
vhost_kernel="$default_feature"
vhost_net="$default_feature"
@@ -1019,10 +1018,6 @@ for opt do
;;
--enable-opengl) opengl="yes"
;;
- --disable-xfsctl) xfs="no"
- ;;
- --enable-xfsctl) xfs="yes"
- ;;
--disable-zlib-test)
;;
--enable-guest-agent) guest_agent="yes"
@@ -1477,7 +1472,6 @@ cat << EOF
avx512f AVX512F optimization support
replication replication support
opengl opengl support
- xfsctl xfsctl support
qom-cast-debug cast debugging support
tools build qemu-io, qemu-nbd and qemu-img tools
bochs bochs image format support
@@ -2385,28 +2379,6 @@ EOF
fi
fi
-##########################################
-# xfsctl() probe, used for file-posix.c
-if test "$xfs" != "no" ; then
- cat > $TMPC << EOF
-#include <stddef.h> /* NULL */
-#include <xfs/xfs.h>
-int main(void)
-{
- xfsctl(NULL, 0, 0, NULL);
- return 0;
-}
-EOF
- if compile_prog "" "" ; then
- xfs="yes"
- else
- if test "$xfs" = "yes" ; then
- feature_not_found "xfs" "Install xfsprogs/xfslibs devel"
- fi
- xfs=no
- fi
-fi
-
##########################################
# plugin linker support probe
@@ -3538,9 +3510,6 @@ echo "CONFIG_BDRV_RO_WHITELIST=$block_drv_ro_whitelist" >> $config_host_mak
if test "$block_drv_whitelist_tools" = "yes" ; then
echo "CONFIG_BDRV_WHITELIST_TOOLS=y" >> $config_host_mak
fi
-if test "$xfs" = "yes" ; then
- echo "CONFIG_XFS=y" >> $config_host_mak
-fi
qemu_version=$(head $source_path/VERSION)
echo "PKGVERSION=$pkgversion" >>$config_host_mak
echo "SRC_PATH=$source_path" >> $config_host_mak
diff --git a/meson.build b/meson.build
index 5bb6b901b0..2bd922f2f3 100644
--- a/meson.build
+++ b/meson.build
@@ -1532,6 +1532,7 @@ config_host_data.set('CONFIG_SETNS', cc.has_function('setns') and cc.has_functio
config_host_data.set('CONFIG_SYNCFS', cc.has_function('syncfs'))
config_host_data.set('CONFIG_SYNC_FILE_RANGE', cc.has_function('sync_file_range'))
config_host_data.set('CONFIG_TIMERFD', cc.has_function('timerfd_create'))
+config_host_data.set('CONFIG_XFS', cc.has_function('xfsctl', prefix: '#include <xfs/xfs.h>'))
config_host_data.set('HAVE_COPY_FILE_RANGE', cc.has_function('copy_file_range'))
config_host_data.set('HAVE_OPENPTY', cc.has_function('openpty', dependencies: util))
config_host_data.set('HAVE_STRCHRNUL', cc.has_function('strchrnul'))
@@ -3415,7 +3416,6 @@ if spice_protocol.found()
summary_info += {' spice server support': spice}
endif
summary_info += {'rbd support': rbd}
-summary_info += {'xfsctl support': config_host.has_key('CONFIG_XFS')}
summary_info += {'smartcard support': cacard}
summary_info += {'U2F support': u2f}
summary_info += {'libusb': libusb}
--
2.27.0
On 28/10/21 20:59, Thomas Huth wrote:
> Checking for xfsctl() can be done more easily in meson.build. Also,
> this is not a "real" feature like the other features that we provide
> with the "--enable-xxx" and "--disable-xxx" switches for the
> configure script, since this does not influence lots of code (it's
> only about one call to xfsctl() in file-posix.c), so people don't
> gain much with the ability to disable this with "--disable-xfsctl".
> Let's rather treat this like the other cc.has_function() checks in
> meson.build, i.e. don't add a new option for this in meson_options.txt.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
I think we should just use ioctl and copy the relevant definitions from
Linux:
struct dioattr {
u32 d_mem; /* data buffer memory alignment */
u32 d_miniosz; /* min xfer size */
u32 d_maxiosz; /* max xfer size */
};
#define XFS_IOC_DIOINFO _IOR ('X', 30, struct dioattr)
Paolo
> ---
> configure | 31 -------------------------------
> meson.build | 2 +-
> 2 files changed, 1 insertion(+), 32 deletions(-)
>
> diff --git a/configure b/configure
> index 170b1b237a..2296c3e194 100755
> --- a/configure
> +++ b/configure
> @@ -287,7 +287,6 @@ for opt do
> done
>
> xen_ctrl_version="$default_feature"
> -xfs="$default_feature"
> membarrier="$default_feature"
> vhost_kernel="$default_feature"
> vhost_net="$default_feature"
> @@ -1019,10 +1018,6 @@ for opt do
> ;;
> --enable-opengl) opengl="yes"
> ;;
> - --disable-xfsctl) xfs="no"
> - ;;
> - --enable-xfsctl) xfs="yes"
> - ;;
> --disable-zlib-test)
> ;;
> --enable-guest-agent) guest_agent="yes"
> @@ -1477,7 +1472,6 @@ cat << EOF
> avx512f AVX512F optimization support
> replication replication support
> opengl opengl support
> - xfsctl xfsctl support
> qom-cast-debug cast debugging support
> tools build qemu-io, qemu-nbd and qemu-img tools
> bochs bochs image format support
> @@ -2385,28 +2379,6 @@ EOF
> fi
> fi
>
> -##########################################
> -# xfsctl() probe, used for file-posix.c
> -if test "$xfs" != "no" ; then
> - cat > $TMPC << EOF
> -#include <stddef.h> /* NULL */
> -#include <xfs/xfs.h>
> -int main(void)
> -{
> - xfsctl(NULL, 0, 0, NULL);
> - return 0;
> -}
> -EOF
> - if compile_prog "" "" ; then
> - xfs="yes"
> - else
> - if test "$xfs" = "yes" ; then
> - feature_not_found "xfs" "Install xfsprogs/xfslibs devel"
> - fi
> - xfs=no
> - fi
> -fi
> -
> ##########################################
> # plugin linker support probe
>
> @@ -3538,9 +3510,6 @@ echo "CONFIG_BDRV_RO_WHITELIST=$block_drv_ro_whitelist" >> $config_host_mak
> if test "$block_drv_whitelist_tools" = "yes" ; then
> echo "CONFIG_BDRV_WHITELIST_TOOLS=y" >> $config_host_mak
> fi
> -if test "$xfs" = "yes" ; then
> - echo "CONFIG_XFS=y" >> $config_host_mak
> -fi
> qemu_version=$(head $source_path/VERSION)
> echo "PKGVERSION=$pkgversion" >>$config_host_mak
> echo "SRC_PATH=$source_path" >> $config_host_mak
> diff --git a/meson.build b/meson.build
> index 5bb6b901b0..2bd922f2f3 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1532,6 +1532,7 @@ config_host_data.set('CONFIG_SETNS', cc.has_function('setns') and cc.has_functio
> config_host_data.set('CONFIG_SYNCFS', cc.has_function('syncfs'))
> config_host_data.set('CONFIG_SYNC_FILE_RANGE', cc.has_function('sync_file_range'))
> config_host_data.set('CONFIG_TIMERFD', cc.has_function('timerfd_create'))
> +config_host_data.set('CONFIG_XFS', cc.has_function('xfsctl', prefix: '#include <xfs/xfs.h>'))
> config_host_data.set('HAVE_COPY_FILE_RANGE', cc.has_function('copy_file_range'))
> config_host_data.set('HAVE_OPENPTY', cc.has_function('openpty', dependencies: util))
> config_host_data.set('HAVE_STRCHRNUL', cc.has_function('strchrnul'))
> @@ -3415,7 +3416,6 @@ if spice_protocol.found()
> summary_info += {' spice server support': spice}
> endif
> summary_info += {'rbd support': rbd}
> -summary_info += {'xfsctl support': config_host.has_key('CONFIG_XFS')}
> summary_info += {'smartcard support': cacard}
> summary_info += {'U2F support': u2f}
> summary_info += {'libusb': libusb}
>
On 02/11/2021 12.34, Paolo Bonzini wrote:
> On 28/10/21 20:59, Thomas Huth wrote:
>> Checking for xfsctl() can be done more easily in meson.build. Also,
>> this is not a "real" feature like the other features that we provide
>> with the "--enable-xxx" and "--disable-xxx" switches for the
>> configure script, since this does not influence lots of code (it's
>> only about one call to xfsctl() in file-posix.c), so people don't
>> gain much with the ability to disable this with "--disable-xfsctl".
>> Let's rather treat this like the other cc.has_function() checks in
>> meson.build, i.e. don't add a new option for this in meson_options.txt.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>
> I think we should just use ioctl and copy the relevant definitions from Linux:
>
> struct dioattr {
> u32 d_mem; /* data buffer memory alignment */
> u32 d_miniosz; /* min xfer size */
> u32 d_maxiosz; /* max xfer size */
> };
>
> #define XFS_IOC_DIOINFO _IOR ('X', 30, struct dioattr)
I thought about something like that, too, but I'm not sure whether xfs/xfs.h
exists on some non-Linux systems, too and might be implemented differently
there?
Thomas
>
>> ---
>> configure | 31 -------------------------------
>> meson.build | 2 +-
>> 2 files changed, 1 insertion(+), 32 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 170b1b237a..2296c3e194 100755
>> --- a/configure
>> +++ b/configure
>> @@ -287,7 +287,6 @@ for opt do
>> done
>> xen_ctrl_version="$default_feature"
>> -xfs="$default_feature"
>> membarrier="$default_feature"
>> vhost_kernel="$default_feature"
>> vhost_net="$default_feature"
>> @@ -1019,10 +1018,6 @@ for opt do
>> ;;
>> --enable-opengl) opengl="yes"
>> ;;
>> - --disable-xfsctl) xfs="no"
>> - ;;
>> - --enable-xfsctl) xfs="yes"
>> - ;;
>> --disable-zlib-test)
>> ;;
>> --enable-guest-agent) guest_agent="yes"
>> @@ -1477,7 +1472,6 @@ cat << EOF
>> avx512f AVX512F optimization support
>> replication replication support
>> opengl opengl support
>> - xfsctl xfsctl support
>> qom-cast-debug cast debugging support
>> tools build qemu-io, qemu-nbd and qemu-img tools
>> bochs bochs image format support
>> @@ -2385,28 +2379,6 @@ EOF
>> fi
>> fi
>> -##########################################
>> -# xfsctl() probe, used for file-posix.c
>> -if test "$xfs" != "no" ; then
>> - cat > $TMPC << EOF
>> -#include <stddef.h> /* NULL */
>> -#include <xfs/xfs.h>
>> -int main(void)
>> -{
>> - xfsctl(NULL, 0, 0, NULL);
>> - return 0;
>> -}
>> -EOF
>> - if compile_prog "" "" ; then
>> - xfs="yes"
>> - else
>> - if test "$xfs" = "yes" ; then
>> - feature_not_found "xfs" "Install xfsprogs/xfslibs devel"
>> - fi
>> - xfs=no
>> - fi
>> -fi
>> -
>> ##########################################
>> # plugin linker support probe
>> @@ -3538,9 +3510,6 @@ echo
>> "CONFIG_BDRV_RO_WHITELIST=$block_drv_ro_whitelist" >> $config_host_mak
>> if test "$block_drv_whitelist_tools" = "yes" ; then
>> echo "CONFIG_BDRV_WHITELIST_TOOLS=y" >> $config_host_mak
>> fi
>> -if test "$xfs" = "yes" ; then
>> - echo "CONFIG_XFS=y" >> $config_host_mak
>> -fi
>> qemu_version=$(head $source_path/VERSION)
>> echo "PKGVERSION=$pkgversion" >>$config_host_mak
>> echo "SRC_PATH=$source_path" >> $config_host_mak
>> diff --git a/meson.build b/meson.build
>> index 5bb6b901b0..2bd922f2f3 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -1532,6 +1532,7 @@ config_host_data.set('CONFIG_SETNS',
>> cc.has_function('setns') and cc.has_functio
>> config_host_data.set('CONFIG_SYNCFS', cc.has_function('syncfs'))
>> config_host_data.set('CONFIG_SYNC_FILE_RANGE',
>> cc.has_function('sync_file_range'))
>> config_host_data.set('CONFIG_TIMERFD', cc.has_function('timerfd_create'))
>> +config_host_data.set('CONFIG_XFS', cc.has_function('xfsctl', prefix:
>> '#include <xfs/xfs.h>'))
>> config_host_data.set('HAVE_COPY_FILE_RANGE',
>> cc.has_function('copy_file_range'))
>> config_host_data.set('HAVE_OPENPTY', cc.has_function('openpty',
>> dependencies: util))
>> config_host_data.set('HAVE_STRCHRNUL', cc.has_function('strchrnul'))
>> @@ -3415,7 +3416,6 @@ if spice_protocol.found()
>> summary_info += {' spice server support': spice}
>> endif
>> summary_info += {'rbd support': rbd}
>> -summary_info += {'xfsctl support': config_host.has_key('CONFIG_XFS')}
>> summary_info += {'smartcard support': cacard}
>> summary_info += {'U2F support': u2f}
>> summary_info += {'libusb': libusb}
>>
>
On 02/11/21 12:38, Thomas Huth wrote:
>>
>>
>> struct dioattr {
>> u32 d_mem; /* data buffer memory alignment */
>> u32 d_miniosz; /* min xfer size */
>> u32 d_maxiosz; /* max xfer size */
>> };
>>
>> #define XFS_IOC_DIOINFO _IOR ('X', 30, struct dioattr)
>
> I thought about something like that, too, but I'm not sure whether
> xfs/xfs.h exists on some non-Linux systems, too and might be implemented
> differently there?
In theory on IRIX XFS exists, but I'm not sure about xfs/xfs.h and
anyway we don't support it.
Paolo
On 02/11/2021 12.34, Paolo Bonzini wrote:
> On 28/10/21 20:59, Thomas Huth wrote:
>> Checking for xfsctl() can be done more easily in meson.build. Also,
>> this is not a "real" feature like the other features that we provide
>> with the "--enable-xxx" and "--disable-xxx" switches for the
>> configure script, since this does not influence lots of code (it's
>> only about one call to xfsctl() in file-posix.c), so people don't
>> gain much with the ability to disable this with "--disable-xfsctl".
>> Let's rather treat this like the other cc.has_function() checks in
>> meson.build, i.e. don't add a new option for this in meson_options.txt.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>
> I think we should just use ioctl and copy the relevant definitions from Linux:
>
> struct dioattr {
> u32 d_mem; /* data buffer memory alignment */
> u32 d_miniosz; /* min xfer size */
> u32 d_maxiosz; /* max xfer size */
> };
>
> #define XFS_IOC_DIOINFO _IOR ('X', 30, struct dioattr)
I've now had a closer look at this idea, but it's getting messy: We'd
additionally also need the platform_test_xfs_fd() function that is called
from file-posix.c ... sure it's not big, but the XFS header stuff is
licensed as LGPL, so it feels wrong to copy this over into file-posix.c that
has a MIT license. Of course, it could be rewritten, or put into a separate
file ... but that is already way more cumbersome for such a small benefit.
So I think I prefer to rather keep my patch in the current shape that has a
way nicer diffstat with way less risk of messing things up here.
Thomas
On 12/10/21 08:53, Thomas Huth wrote:
> On 02/11/2021 12.34, Paolo Bonzini wrote:
>> On 28/10/21 20:59, Thomas Huth wrote:
>>> Checking for xfsctl() can be done more easily in meson.build. Also,
>>> this is not a "real" feature like the other features that we provide
>>> with the "--enable-xxx" and "--disable-xxx" switches for the
>>> configure script, since this does not influence lots of code (it's
>>> only about one call to xfsctl() in file-posix.c), so people don't
>>> gain much with the ability to disable this with "--disable-xfsctl".
>>> Let's rather treat this like the other cc.has_function() checks in
>>> meson.build, i.e. don't add a new option for this in meson_options.txt.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>
>> I think we should just use ioctl and copy the relevant definitions
>> from Linux:
>>
>> struct dioattr {
>> u32 d_mem; /* data buffer memory alignment */
>> u32 d_miniosz; /* min xfer size */
>> u32 d_maxiosz; /* max xfer size */
>> };
>>
>> #define XFS_IOC_DIOINFO _IOR ('X', 30, struct dioattr)
>
> I've now had a closer look at this idea, but it's getting messy: We'd
> additionally also need the platform_test_xfs_fd() function that is
> called from file-posix.c ...
platform_test_xfs_fd() is only used to decide whether to invoke
XFS_IOC_DIOINFO; but failures of XFS_IOC_DIOINFO are ignored anyway, so
we can get rid of is_xfs in BDRVRawState, too.
Paolo
On 10/12/2021 09.39, Paolo Bonzini wrote:
> On 12/10/21 08:53, Thomas Huth wrote:
>> On 02/11/2021 12.34, Paolo Bonzini wrote:
>>> On 28/10/21 20:59, Thomas Huth wrote:
>>>> Checking for xfsctl() can be done more easily in meson.build. Also,
>>>> this is not a "real" feature like the other features that we provide
>>>> with the "--enable-xxx" and "--disable-xxx" switches for the
>>>> configure script, since this does not influence lots of code (it's
>>>> only about one call to xfsctl() in file-posix.c), so people don't
>>>> gain much with the ability to disable this with "--disable-xfsctl".
>>>> Let's rather treat this like the other cc.has_function() checks in
>>>> meson.build, i.e. don't add a new option for this in meson_options.txt.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>
>>> I think we should just use ioctl and copy the relevant definitions from
>>> Linux:
>>>
>>> struct dioattr {
>>> u32 d_mem; /* data buffer memory alignment */
>>> u32 d_miniosz; /* min xfer size */
>>> u32 d_maxiosz; /* max xfer size */
>>> };
>>>
>>> #define XFS_IOC_DIOINFO _IOR ('X', 30, struct dioattr)
>>
>> I've now had a closer look at this idea, but it's getting messy: We'd
>> additionally also need the platform_test_xfs_fd() function that is called
>> from file-posix.c ...
>
> platform_test_xfs_fd() is only used to decide whether to invoke
> XFS_IOC_DIOINFO; but failures of XFS_IOC_DIOINFO are ignored anyway, so we
> can get rid of is_xfs in BDRVRawState, too.
After staring at the code for a while, I wonder why we're not simply using
fstat() here instead to get the st_blksize value... wouldn't that be better
anyway since it also works with other file system types?
Thomas
On 12/10/21 09:46, Thomas Huth wrote: >> >> platform_test_xfs_fd() is only used to decide whether to invoke >> XFS_IOC_DIOINFO; but failures of XFS_IOC_DIOINFO are ignored anyway, >> so we can get rid of is_xfs in BDRVRawState, too. > > After staring at the code for a while, I wonder why we're not simply > using fstat() here instead to get the st_blksize value... wouldn't that > be better anyway since it also works with other file system types? The value that XFS_IOC_DIOINFO returns is the logical sector size of the underlying device; it should be 512 or 4096, but more likely 512. It can be smaller than st_blksize, because often it will be if it is 512 but the st_blksize is usually 4096. If it is wrong, QEMU will do unnecessary read/modify/write operations for disk writes that are not 4K-aligned. Paolo
On 10/12/2021 11.10, Paolo Bonzini wrote: > On 12/10/21 09:46, Thomas Huth wrote: >>> >>> platform_test_xfs_fd() is only used to decide whether to invoke >>> XFS_IOC_DIOINFO; but failures of XFS_IOC_DIOINFO are ignored anyway, so >>> we can get rid of is_xfs in BDRVRawState, too. >> >> After staring at the code for a while, I wonder why we're not simply using >> fstat() here instead to get the st_blksize value... wouldn't that be >> better anyway since it also works with other file system types? > > The value that XFS_IOC_DIOINFO returns is the logical sector size of the > underlying device; it should be 512 or 4096, but more likely 512. It can be > smaller than st_blksize, because often it will be if it is 512 but the > st_blksize is usually 4096. > > If it is wrong, QEMU will do unnecessary read/modify/write operations for > disk writes that are not 4K-aligned. Ok, true, I've checked it and XFS_IOC_DIOINFO return 512 on my laptop indeed, while fstat->st_blksize is 4096 instead. So it's not the same :-/ Thomas
© 2016 - 2026 Red Hat, Inc.