[PATCH blktests] Fix build failure for discontiguous-io on 32-bit platforms

linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH blktests] Fix build failure for discontiguous-io on 32-bit platforms
@ 2018-10-29 16:15 Theodore Ts'o
  2018-10-29 16:26 ` Bart Van Assche
  0 siblings, 1 reply; 5+ messages in thread
From: Theodore Ts'o @ 2018-10-29 16:15 UTC (permalink / raw)
  To: linux-block; +Cc: Theodore Ts'o

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 src/discontiguous-io.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/discontiguous-io.cpp b/src/discontiguous-io.cpp
index 5e0ee0f..a59c18d 100644
--- a/src/discontiguous-io.cpp
+++ b/src/discontiguous-io.cpp
@@ -291,7 +291,7 @@ int main(int argc, char **argv)
 			unsigned char *p = &*buf.begin();
 			for (int i = 0; i < len / 4; i++)
 				iov.append(p + 4 + i * 8,
-					   std::min(4ul, len - i * 4));
+					   std::min(4ul, (unsigned long) len - i * 4));
 		} else {
 			iov.append(&*buf.begin(), buf.size());
 		}
-- 
2.18.0.rc0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH blktests] Fix build failure for discontiguous-io on 32-bit platforms
  2018-10-29 16:15 [PATCH blktests] Fix build failure for discontiguous-io on 32-bit platforms Theodore Ts'o
@ 2018-10-29 16:26 ` Bart Van Assche
  2018-10-29 21:08   ` Theodore Y. Ts'o
  0 siblings, 1 reply; 5+ messages in thread
From: Bart Van Assche @ 2018-10-29 16:26 UTC (permalink / raw)
  To: Theodore Ts'o, linux-block

On Mon, 2018-10-29 at 12:15 -0400, Theodore Ts'o wrote:
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  src/discontiguous-io.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/discontiguous-io.cpp b/src/discontiguous-io.cpp
> index 5e0ee0f..a59c18d 100644
> --- a/src/discontiguous-io.cpp
> +++ b/src/discontiguous-io.cpp
> @@ -291,7 +291,7 @@ int main(int argc, char **argv)
>  			unsigned char *p = &*buf.begin();
>  			for (int i = 0; i < len / 4; i++)
>  				iov.append(p + 4 + i * 8,
> -					   std::min(4ul, len - i * 4));
> +					   std::min(4ul, (unsigned long) len - i * 4));
>  		} else {
>  			iov.append(&*buf.begin(), buf.size());
>  		}

Hi Ted,

Have you considered to change the data type of 'len' from size_t into unsigned long
instead of inserting this cast? That would make it clear that no integer truncation
happens in the iov.append() call.

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH blktests] Fix build failure for discontiguous-io on 32-bit platforms
  2018-10-29 16:26 ` Bart Van Assche
@ 2018-10-29 21:08   ` Theodore Y. Ts'o
  2018-10-29 21:32     ` Bart Van Assche
  0 siblings, 1 reply; 5+ messages in thread
From: Theodore Y. Ts'o @ 2018-10-29 21:08 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: linux-block

On Mon, Oct 29, 2018 at 09:26:43AM -0700, Bart Van Assche wrote:
> 
> Have you considered to change the data type of 'len' from size_t into unsigned long
> instead of inserting this cast? That would make it clear that no integer truncation
> happens in the iov.append() call.

Well the potential integer truncation that could happen is here:

                case 'l': len = strtoul(optarg, NULL, 0); break;

... but I really don't think the user would ever send a length >
2**32, and I have a sneaking suspicion that ib_srp has zero change of
working on 32-bit systems (which is what uses this function) so this
was more about making sure blktests would build when I'm building the
32-bit version of my test appliance VM.

(For that matter, I've been banging my head against a brick wall
trying to make the srp tests work in either a KVM or GCE environment,
even with a 64-bit VM, but that's a different issue.  It's not high
priority for me, at the moment, but eventually I would like the
{kvm,gce,android}-xfstest test appliance VM's generated by the
xfstests-bld repo to be able to run all of blktests.)

If folks have strong opinions about what's the best way to fix this,
I'm happy to adjust the patch.  (I personally don't care that much).
What offends me more is that later on there are scopes when len gets
shadowed with a "ssize_t len;" definition --- I whould have thought
gcc or clang would have complained bitterly about that, but I guess
not.  I didn't want to make that many clean up changes, especially
since I'm not in a position to test any changes that I make, since as
I mentioned the srp blktests are failing miserably for me.

  	    	    	     	 	 	   - Ted

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH blktests] Fix build failure for discontiguous-io on 32-bit platforms
  2018-10-29 21:08   ` Theodore Y. Ts'o
@ 2018-10-29 21:32     ` Bart Van Assche
  2018-10-29 23:22       ` Theodore Y. Ts'o
  0 siblings, 1 reply; 5+ messages in thread
From: Bart Van Assche @ 2018-10-29 21:32 UTC (permalink / raw)
  To: Theodore Y. Ts'o; +Cc: linux-block

On Mon, 2018-10-29 at 17:08 -0400, Theodore Y. Ts'o wrote:
> On Mon, Oct 29, 2018 at 09:26:43AM -0700, Bart Van Assche wrote:
> > 
> > Have you considered to change the data type of 'len' from size_t into unsigned long
> > instead of inserting this cast? That would make it clear that no integer truncation
> > happens in the iov.append() call.
> 
> Well the potential integer truncation that could happen is here:
> 
>                 case 'l': len = strtoul(optarg, NULL, 0); break;

If the data type of 'len' would be changed into unsigned long, how could that
assignment cause integer truncation since strtoul() returns an unsigned long
value? 

> I have a sneaking suspicion that ib_srp has zero change of
> working on 32-bit systems (which is what uses this function) so this
> was more about making sure blktests would build when I'm building the
> 32-bit version of my test appliance VM.

What makes you think that ib_srp won't work on 32-bit systems? I don't think
that anyone uses this driver on a 32-bit system. I'm not aware of any aspect
of that driver that restricts it to 64-bit systems only either.

> (For that matter, I've been banging my head against a brick wall
> trying to make the srp tests work in either a KVM or GCE environment,
> even with a 64-bit VM, but that's a different issue.  It's not high
> priority for me, at the moment, but eventually I would like the
> {kvm,gce,android}-xfstest test appliance VM's generated by the
> xfstests-bld repo to be able to run all of blktests.)

Can you be more specific about what didn't work? Were you unable to start the
tests or did a particular test fail?

> What offends me more is that later on there are scopes when len gets
> shadowed with a "ssize_t len;" definition --- I whould have thought
> gcc or clang would have complained bitterly about that, but I guess
> not.

I agree that it would be an improvement to get rid of variable shadowing.
Once that has been done the -Wshadow compiler flag can be added. I will look
into this if nobody else starts looking into this soon.

Bart.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH blktests] Fix build failure for discontiguous-io on 32-bit platforms
  2018-10-29 21:32     ` Bart Van Assche
@ 2018-10-29 23:22       ` Theodore Y. Ts'o
  0 siblings, 0 replies; 5+ messages in thread
From: Theodore Y. Ts'o @ 2018-10-29 23:22 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: linux-block

[-- Attachment #1: Type: text/plain, Size: 3651 bytes --]

On Mon, Oct 29, 2018 at 02:32:34PM -0700, Bart Van Assche wrote:
> > Well the potential integer truncation that could happen is here:
> > 
> >                 case 'l': len = strtoul(optarg, NULL, 0); break;
> 
> If the data type of 'len' would be changed into unsigned long, how could that
> assignment cause integer truncation since strtoul() returns an unsigned long
> value?

The potential truncation in the existing code is because len is a
size_t, and on a 32-bit platform, it's 32 bits while strotul returns a
64-bit value.  I also think it really doesn't matter in practice.  :-)

I'm happy making the len to be an unsigned long, but then I then we
would be passing passing a 64-bit to a vector's resize method on a
32-bit platform.  It looks like g++ isn't complaining, so sure.  I'll
resend with that change.

> What makes you think that ib_srp won't work on 32-bit systems? I don't think
> that anyone uses this driver on a 32-bit system. I'm not aware of any aspect
> of that driver that restricts it to 64-bit systems only either.

Given the restricted address space on a 32-bit system, I would expect
that this might be... challenging for RDMA; and it wasn't clear anyone
willing to pay $$$$ to for RDMA gear would be willing to settle for a
32-bit CPU.  The fact that support programs for blktests doesn't even
build on a 32-bit platform doesn't bode well for how well ib_srp has
been tested on 32-bit systems, in any case.  :-)

> Can you be more specific about what didn't work? Were you unable to start the
> tests or did a particular test fail?

It took a huge amount of work, but I finally was able to start the
tests; but then they *all* fail.  Attached is the results of running
"kvm-xfstests --blktests srp".

Part of the problem is I'm simply not familiar with the userspace
tools (or the kernel components for that matter) used by the srp
tests.  It might simply be that I'm running to some version dependency
on the userspace utilities?

One thing that would certainly help for newcomers is a minimum set of
kernel configs that are needed to allow blktests (and the srp tests in
particular) to run.  Better yet would be if the blktests listed all of
the missing modules, instead of just saying, "you need module <foo>",
and then after the user recompiled the kernel with <foo> enabled, then
it would say, "you need module <bar>".  I must have rebuilt the kernel
a dozen+ times, each time needing to figure out which kernel configs I
needed to enable to generate the missing module.  Eventually when I
get this all working, I'll update the suggested kernel defconfigs at

https://github.com/tytso/xfstests-bld/tree/master/kernel-configs

so others won't have to go through the pain that I did.  :-)

						- Ted

P.S.  Here are the kernel modules I've found that I've needed for
blk-tests on top of kernel defconfigs found above:

% grep =m /build/ext4-64/defconfig                                             
CONFIG_BLK_DEV_NULL_BLK=m
CONFIG_BLK_DEV_NBD=m
CONFIG_BLK_DEV_NVME=m
CONFIG_NVME_TARGET=m
CONFIG_NVME_TARGET_LOOP=m
CONFIG_BLK_DEV_SR=m
CONFIG_CHR_DEV_SG=m
CONFIG_SCSI_DEBUG=m
CONFIG_SCSI_DH_RDAC=m
CONFIG_SCSI_DH_EMC=m
CONFIG_SCSI_DH_ALUA=m
CONFIG_BLK_DEV_MD=m
CONFIG_MD_MULTIPATH=m
CONFIG_DM_MULTIPATH=m
CONFIG_DM_MULTIPATH_QL=m
CONFIG_DM_MULTIPATH_ST=m
CONFIG_TARGET_CORE=m
CONFIG_TCM_IBLOCK=m
CONFIG_TCM_FILEIO=m
CONFIG_TCM_PSCSI=m
CONFIG_TCM_USER2=m
CONFIG_INFINIBAND=m
CONFIG_INFINIBAND_USER_MAD=m
CONFIG_INFINIBAND_USER_ACCESS=m
CONFIG_INFINIBAND_IPOIB=m
CONFIG_INFINIBAND_SRP=m
CONFIG_INFINIBAND_SRPT=m
CONFIG_RDMA_RXE=m

(note the truly impressive number of modules needed in order to run
the srp tests :-)

[-- Attachment #2: log.201810291903.gz --]
[-- Type: application/gzip, Size: 18183 bytes --]

[-- Attachment #3: defconfig --]
[-- Type: text/plain, Size: 5597 bytes --]

CONFIG_LOCALVERSION="-xfstests"
CONFIG_SYSVIPC=y
CONFIG_POSIX_MQUEUE=y
CONFIG_NO_HZ=y
CONFIG_HIGH_RES_TIMERS=y
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y
CONFIG_CGROUPS=y
CONFIG_USER_NS=y
CONFIG_BLK_DEV_INITRD=y
# CONFIG_COMPAT_BRK is not set
CONFIG_SLAB=y
CONFIG_SMP=y
CONFIG_X86_X2APIC=y
# CONFIG_X86_EXTENDED_PLATFORM is not set
CONFIG_HYPERVISOR_GUEST=y
CONFIG_PARAVIRT=y
CONFIG_PARAVIRT_SPINLOCKS=y
CONFIG_PARAVIRT_TIME_ACCOUNTING=y
CONFIG_MCORE2=y
CONFIG_NR_CPUS=48
# CONFIG_X86_MCE_AMD is not set
# CONFIG_MICROCODE is not set
CONFIG_NUMA=y
# CONFIG_AMD_NUMA is not set
CONFIG_X86_PMEM_LEGACY=y
CONFIG_X86_CHECK_BIOS_CORRUPTION=y
CONFIG_HZ_300=y
CONFIG_KEXEC=y
# CONFIG_SUSPEND is not set
# CONFIG_ACPI_REV_OVERRIDE_POSSIBLE is not set
# CONFIG_ACPI_TABLE_UPGRADE is not set
# CONFIG_PCI_MMCONFIG is not set
CONFIG_PCI_MSI=y
CONFIG_IA32_EMULATION=y
# CONFIG_DMIID is not set
CONFIG_JUMP_LABEL=y
CONFIG_REFCOUNT_FULL=y
CONFIG_MODULES=y
CONFIG_MODULE_UNLOAD=y
CONFIG_MEMORY_HOTPLUG=y
CONFIG_MEMORY_HOTREMOVE=y
CONFIG_ZONE_DEVICE=y
CONFIG_NET=y
CONFIG_PACKET=y
CONFIG_PACKET_DIAG=y
CONFIG_UNIX=y
CONFIG_UNIX_DIAG=y
CONFIG_INET=y
CONFIG_SYN_COOKIES=y
# CONFIG_INET_XFRM_MODE_TRANSPORT is not set
# CONFIG_INET_XFRM_MODE_TUNNEL is not set
# CONFIG_INET_XFRM_MODE_BEET is not set
CONFIG_INET_UDP_DIAG=y
# CONFIG_INET6_XFRM_MODE_TRANSPORT is not set
# CONFIG_INET6_XFRM_MODE_TUNNEL is not set
# CONFIG_INET6_XFRM_MODE_BEET is not set
CONFIG_NETLINK_DIAG=y
# CONFIG_WIRELESS is not set
CONFIG_NET_9P=y
CONFIG_NET_9P_VIRTIO=y
CONFIG_DEVTMPFS=y
CONFIG_MTD=y
CONFIG_MTD_BLOCK2MTD=y
CONFIG_MTD_UBI=y
CONFIG_BLK_DEV_NULL_BLK=m
CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_BLK_DEV_NBD=m
CONFIG_VIRTIO_BLK=y
CONFIG_BLK_DEV_NVME=m
CONFIG_NVME_TARGET=m
CONFIG_NVME_TARGET_LOOP=m
CONFIG_SCSI=y
# CONFIG_SCSI_MQ_DEFAULT is not set
CONFIG_BLK_DEV_SD=y
CONFIG_BLK_DEV_SR=m
CONFIG_CHR_DEV_SG=m
CONFIG_SCSI_DEBUG=m
CONFIG_SCSI_VIRTIO=y
CONFIG_SCSI_DH=y
CONFIG_SCSI_DH_RDAC=m
CONFIG_SCSI_DH_EMC=m
CONFIG_SCSI_DH_ALUA=m
CONFIG_MD=y
CONFIG_BLK_DEV_MD=m
CONFIG_MD_MULTIPATH=m
CONFIG_BLK_DEV_DM=y
CONFIG_DM_SNAPSHOT=y
CONFIG_DM_THIN_PROVISIONING=y
CONFIG_DM_ZERO=y
CONFIG_DM_MULTIPATH=m
CONFIG_DM_MULTIPATH_QL=m
CONFIG_DM_MULTIPATH_ST=m
CONFIG_DM_UEVENT=y
CONFIG_DM_FLAKEY=y
CONFIG_DM_LOG_WRITES=y
CONFIG_TARGET_CORE=m
CONFIG_TCM_IBLOCK=m
CONFIG_TCM_FILEIO=m
CONFIG_TCM_PSCSI=m
CONFIG_TCM_USER2=m
CONFIG_NETDEVICES=y
CONFIG_VIRTIO_NET=y
# CONFIG_ETHERNET is not set
# CONFIG_WLAN is not set
# CONFIG_INPUT_MOUSE is not set
# CONFIG_SERIO_SERPORT is not set
# CONFIG_LEGACY_PTYS is not set
CONFIG_SERIAL_8250=y
# CONFIG_SERIAL_8250_DEPRECATED_OPTIONS is not set
CONFIG_SERIAL_8250_CONSOLE=y
CONFIG_SERIAL_8250_NR_UARTS=32
CONFIG_SERIAL_8250_RUNTIME_UARTS=32
CONFIG_HW_RANDOM=y
# CONFIG_HW_RANDOM_INTEL is not set
# CONFIG_HW_RANDOM_AMD is not set
# CONFIG_HW_RANDOM_VIA is not set
CONFIG_HW_RANDOM_VIRTIO=y
# CONFIG_HWMON is not set
# CONFIG_X86_PKG_TEMP_THERMAL is not set
# CONFIG_HID is not set
# CONFIG_USB_SUPPORT is not set
CONFIG_INFINIBAND=m
CONFIG_INFINIBAND_USER_MAD=m
CONFIG_INFINIBAND_USER_ACCESS=m
CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI=y
CONFIG_INFINIBAND_IPOIB=m
CONFIG_INFINIBAND_SRP=m
CONFIG_INFINIBAND_SRPT=m
CONFIG_RDMA_RXE=m
CONFIG_RTC_CLASS=y
# CONFIG_RTC_DRV_CMOS is not set
CONFIG_UIO=y
CONFIG_VIRT_DRIVERS=y
CONFIG_VIRTIO_PCI=y
CONFIG_VIRTIO_BALLOON=y
# CONFIG_X86_PLATFORM_DEVICES is not set
# CONFIG_IOMMU_SUPPORT is not set
CONFIG_EXT2_FS=y
CONFIG_EXT2_FS_XATTR=y
CONFIG_EXT2_FS_POSIX_ACL=y
CONFIG_EXT2_FS_SECURITY=y
CONFIG_EXT4_FS=y
CONFIG_EXT4_FS_POSIX_ACL=y
CONFIG_EXT4_FS_SECURITY=y
CONFIG_EXT4_ENCRYPTION=y
CONFIG_EXT4_DEBUG=y
CONFIG_JBD2_DEBUG=y
CONFIG_XFS_FS=y
CONFIG_XFS_QUOTA=y
CONFIG_XFS_POSIX_ACL=y
CONFIG_XFS_RT=y
CONFIG_BTRFS_FS=y
CONFIG_BTRFS_FS_POSIX_ACL=y
CONFIG_BTRFS_DEBUG=y
CONFIG_BTRFS_ASSERT=y
CONFIG_F2FS_FS=y
CONFIG_F2FS_FS_SECURITY=y
CONFIG_F2FS_CHECK_FS=y
CONFIG_F2FS_FS_ENCRYPTION=y
CONFIG_FS_DAX=y
CONFIG_QUOTA=y
CONFIG_QUOTA_NETLINK_INTERFACE=y
# CONFIG_PRINT_QUOTA_WARNING is not set
CONFIG_QFMT_V2=y
CONFIG_AUTOFS4_FS=y
CONFIG_OVERLAY_FS=y
CONFIG_PROC_KCORE=y
CONFIG_PROC_CHILDREN=y
CONFIG_TMPFS=y
CONFIG_TMPFS_POSIX_ACL=y
CONFIG_CONFIGFS_FS=y
CONFIG_UBIFS_FS=y
CONFIG_UBIFS_FS_ENCRYPTION=y
CONFIG_9P_FS=y
CONFIG_NLS_DEFAULT="utf8"
CONFIG_NLS_ASCII=y
CONFIG_NLS_UTF8=y
CONFIG_SECURITY=y
CONFIG_FORTIFY_SOURCE=y
CONFIG_INTEGRITY_SIGNATURE=y
CONFIG_IMA=y
CONFIG_IMA_WRITE_POLICY=y
CONFIG_IMA_APPRAISE=y
CONFIG_EVM=y
# CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
CONFIG_CRYPTO_ECHAINIV=y
CONFIG_CRYPTO_CRC32C_INTEL=y
CONFIG_CRYPTO_CRC32_PCLMUL=y
CONFIG_CRYPTO_AES_NI_INTEL=y
# CONFIG_CRYPTO_HW is not set
CONFIG_ASYMMETRIC_KEY_TYPE=y
CONFIG_SYSTEM_TRUSTED_KEYRING=y
CONFIG_SYSTEM_TRUSTED_KEYS="certs/cert.pem"
CONFIG_PRINTK_TIME=y
CONFIG_DYNAMIC_DEBUG=y
CONFIG_DEBUG_INFO=y
CONFIG_DEBUG_INFO_REDUCED=y
CONFIG_DEBUG_SECTION_MISMATCH=y
CONFIG_MAGIC_SYSRQ=y
CONFIG_DEBUG_KERNEL=y
CONFIG_DEBUG_PAGEALLOC=y
CONFIG_DEBUG_OBJECTS=y
CONFIG_DEBUG_KMEMLEAK=y
CONFIG_DEBUG_KMEMLEAK_EARLY_LOG_SIZE=3000
CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF=y
CONFIG_DEBUG_STACKOVERFLOW=y
CONFIG_HARDLOCKUP_DETECTOR=y
CONFIG_WQ_WATCHDOG=y
CONFIG_PANIC_TIMEOUT=5
CONFIG_PROVE_LOCKING=y
CONFIG_LOCK_STAT=y
CONFIG_DEBUG_ATOMIC_SLEEP=y
CONFIG_DEBUG_LIST=y
CONFIG_DEBUG_SG=y
CONFIG_RCU_EQS_DEBUG=y
CONFIG_FAULT_INJECTION=y
CONFIG_FAIL_MAKE_REQUEST=y
CONFIG_FAULT_INJECTION_DEBUG_FS=y
CONFIG_FUNCTION_TRACER=y
CONFIG_FTRACE_SYSCALLS=y
CONFIG_TRACER_SNAPSHOT=y
CONFIG_BLK_DEV_IO_TRACE=y
CONFIG_FUNCTION_PROFILER=y
# CONFIG_RUNTIME_TESTING_MENU is not set
CONFIG_DEBUG_WX=y

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-10-30  8:13 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-29 16:15 [PATCH blktests] Fix build failure for discontiguous-io on 32-bit platforms Theodore Ts'o
2018-10-29 16:26 ` Bart Van Assche
2018-10-29 21:08   ` Theodore Y. Ts'o
2018-10-29 21:32     ` Bart Van Assche
2018-10-29 23:22       ` Theodore Y. Ts'o

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).