From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 011B5C00144 for ; Fri, 29 Jul 2022 15:49:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229548AbiG2PtD (ORCPT ); Fri, 29 Jul 2022 11:49:03 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51510 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234214AbiG2PtC (ORCPT ); Fri, 29 Jul 2022 11:49:02 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 280508875F for ; Fri, 29 Jul 2022 08:49:01 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id D89B3B8284B for ; Fri, 29 Jul 2022 15:48:59 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9B0B0C433D6; Fri, 29 Jul 2022 15:48:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1659109738; bh=TchS88iJ7ejnTfEJDx1ylXI3JFh4xlhWaRtaJTdiW8Q=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=lC/Z11CD0DlGJRmPSknGaYivCxlB8AhcXKDH0/7unU7evabxhVPZX21zVpTtu5VQI UQnz1q7X7q4NmRAKQzK/ayTcZh7YPtwK5KE2KbvGa7afCodPBRad/cv887YWjcMs+f NFhhjTL8+NliaNurrhg6ye3WVoQRwPIxh2coK7PkthQTw1pNGO4T6x7ej37DfS2LQS jiu0Z5UUdcBR41IyBsDRO9T1v47yMyKRFpCLSFGxlUQlAD1pErhtjnSjIuj/xSd1AM WKdR3CIj29fjLQetMQJNM/LkUqtFV+4iNcCVMuLC+iSzg4YuBgbbLjbvJQsDM3VA++ Dc0D5ok3+CA1w== Date: Fri, 29 Jul 2022 08:48:58 -0700 From: "Darrick J. Wong" To: Florian Fainelli Cc: xfs , Fabrice Fontaine Subject: Re: [RFC PATCH] libxfs: stop overriding MAP_SYNC in publicly exported header files Message-ID: References: <20220721121128.yyxnvkn4opjdgcln@orion> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-xfs@vger.kernel.org On Thu, Jul 28, 2022 at 03:29:49PM -0700, Florian Fainelli wrote: > On 7/21/22 05:11, Carlos Maiolino wrote: > > On Wed, Jul 20, 2022 at 04:28:00PM -0700, Darrick J. Wong wrote: > >> Can one of you please apply this patch and see if it'll build in musl on > >> mips, please? Sorry it's taken so long to address this. :/ > >> > >> --D > >> > >> --- > >> From: Darrick J. Wong > >> > >> Florian Fainelli most recently reported that xfsprogs doesn't build with > >> musl on mips: > >> > >> "MIPS platforms building with recent kernel headers and the musl-libc > >> toolchain will expose the following build failure: > >> > >> mmap.c: In function 'mmap_f': > >> mmap.c:196:12: error: 'MAP_SYNC' undeclared (first use in this function); did you mean 'MS_SYNC'? > >> 196 | flags = MAP_SYNC | MAP_SHARED_VALIDATE; > >> | ^~~~~~~~ > >> | MS_SYNC > >> mmap.c:196:12: note: each undeclared identifier is reported only once for each function it appears in > >> make[4]: *** [../include/buildrules:81: mmap.o] Error 1" > >> > >> At first glance, the build failure here is caused by the fact that: > >> > >> 1. The configure script doesn't detect MAP_SYNC support > >> 2. The build system doesn't set HAVE_MAP_SYNC > >> 2. io/mmap.c includes input.h -> projects.h -> xfs.h and later sys/mman.h > >> 3. include/linux.h #define's MAP_SYNC to 0 if HAVE_MAP_SYNC is not set > >> 4. musl's sys/mman.h #undef MAP_SYNC on platforms that don't support it > >> 5. io/mmap.c tries to use MAP_SYNC, not realizing that libc undefined it > >> > >> Normally, xfs_io only exports functionality that is defined by the libc > >> and/or kernel headers on the build system. We often make exceptions for > >> new functionality so that we have a way to test them before the header > >> file packages catch up, hence this '#ifndef HAVE_FOO #define FOO' > >> paradigm. > >> > >> MAP_SYNC is a gross and horribly broken example of this. These support > >> crutches are supposed to be *private* to xfsprogs for benefit of early > >> testing, but they were instead added to include/linux.h, which we > >> provide to user programs in the xfslibs-dev package. IOWs, we've been > >> #defining MAP_SYNC to zero for unsuspecting programs. > >> > >> Worst yet, gcc 11.3 doesn't even warn about overriding a #define to 0: > >> > >> #include > >> #include > >> #ifdef STUPID > >> # include > >> #endif > >> > >> int main(int argc, char *argv[]) { > >> printf("MAP_SYNC 0x%x\n", MAP_SYNC); > >> } > >> > >> $ gcc -o a a.c -Wall > >> $ ./a > >> MAP_SYNC 0x80000 > >> $ gcc -DSTUPID -o a a.c -Wall > >> $ ./a > >> MAP_SYNC 0x0 > >> > >> Four years have gone by since the introduction of MAP_SYNC, so let's get > >> rid of the override code entirely -- any platform that supports MAP_SYNC > >> has had plenty of chances to ensure their header files have the right > >> bits. While we're at it, fix AC_HAVE_MAP_SYNC to look for MAP_SYNC in > >> the same header file that the one user (io/mmap.c) uses -- sys/mman.h. > >> > >> Annoyingly, I had to test this by hand because the sole fstest that > >> exercises MAP_SYNC (generic/470) requires dm-logwrites and dm-thinp, > >> neither of which support fsdax on current kernels. > >> > >> Reported-by: info@mobile-stream.com > >> Reported-by: Fabrice Fontaine > >> Reported-by: Florian Fainelli > >> Signed-off-by: Darrick J. Wong > >> --- > >> include/linux.h | 8 -------- > >> io/io.h | 2 +- > >> io/mmap.c | 25 +++++++++++++------------ > >> m4/package_libcdev.m4 | 3 +-- > >> 4 files changed, 15 insertions(+), 23 deletions(-) > >> > >> diff --git a/include/linux.h b/include/linux.h > >> index 3d9f4e3d..eddc4ad9 100644 > >> --- a/include/linux.h > >> +++ b/include/linux.h > >> @@ -251,14 +251,6 @@ struct fsxattr { > >> #define FS_XFLAG_COWEXTSIZE 0x00010000 /* CoW extent size allocator hint */ > >> #endif > >> > >> -#ifndef HAVE_MAP_SYNC > >> -#define MAP_SYNC 0 > >> -#define MAP_SHARED_VALIDATE 0 > >> -#else > >> -#include > >> -#include > >> -#endif /* HAVE_MAP_SYNC */ > >> - > >> /* > >> * Reminder: anything added to this file will be compiled into downstream > >> * userspace projects! > >> diff --git a/io/io.h b/io/io.h > >> index ada0a149..de4ef607 100644 > >> --- a/io/io.h > >> +++ b/io/io.h > >> @@ -58,7 +58,7 @@ typedef struct mmap_region { > >> size_t length; /* length of mapping */ > >> off64_t offset; /* start offset into backing file */ > >> int prot; /* protection mode of the mapping */ > >> - bool map_sync; /* is this a MAP_SYNC mapping? */ > >> + int flags; /* MAP_* flags passed to mmap() */ > >> char *name; /* name of backing file */ > >> } mmap_region_t; > >> > >> diff --git a/io/mmap.c b/io/mmap.c > >> index 8c048a0a..425957d4 100644 > >> --- a/io/mmap.c > >> +++ b/io/mmap.c > >> @@ -46,8 +46,11 @@ print_mapping( > >> for (i = 0, p = pflags; p->prot != PROT_NONE; i++, p++) > >> buffer[i] = (map->prot & p->prot) ? p->mode : '-'; > >> > >> - if (map->map_sync) > >> +#ifdef HAVE_MAP_SYNC > >> + if ((map->flags & (MAP_SYNC | MAP_SHARED_VALIDATE)) == > >> + (MAP_SYNC | MAP_SHARED_VALIDATE)) > >> sprintf(&buffer[i], " S"); > >> +#endif > >> > >> printf("%c%03d%c 0x%lx - 0x%lx %s %14s (%lld : %ld)\n", > >> braces? '[' : ' ', index, braces? ']' : ' ', > >> @@ -139,7 +142,9 @@ mmap_help(void) > >> " -r -- map with PROT_READ protection\n" > >> " -w -- map with PROT_WRITE protection\n" > >> " -x -- map with PROT_EXEC protection\n" > >> +#ifdef HAVE_MAP_SYNC > >> " -S -- map with MAP_SYNC and MAP_SHARED_VALIDATE flags\n" > >> +#endif > >> " -s -- first do mmap(size)/munmap(size), try to reserve some free space\n" > >> " If no protection mode is specified, all are used by default.\n" > >> "\n")); > >> @@ -193,18 +198,14 @@ mmap_f( > >> prot |= PROT_EXEC; > >> break; > >> case 'S': > >> +#ifdef HAVE_MAP_SYNC > >> flags = MAP_SYNC | MAP_SHARED_VALIDATE; > >> - > >> - /* > >> - * If MAP_SYNC and MAP_SHARED_VALIDATE aren't defined > >> - * in the system headers we will have defined them > >> - * both as 0. > >> - */ > >> - if (!flags) { > >> - printf("MAP_SYNC not supported\n"); > >> - return 0; > >> - } > >> break; > >> +#else > >> + printf("MAP_SYNC not supported\n"); > >> + exitcode = 1; > >> + return command_usage(&mmap_cmd); > >> +#endif > >> case 's': > >> length2 = cvtnum(blocksize, sectsize, optarg); > >> break; > >> @@ -281,7 +282,7 @@ mmap_f( > >> mapping->offset = offset; > >> mapping->name = filename; > >> mapping->prot = prot; > >> - mapping->map_sync = (flags == (MAP_SYNC | MAP_SHARED_VALIDATE)); > >> + mapping->flags = flags; > >> return 0; > >> } > >> > >> diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4 > >> index df44174d..5293dd1a 100644 > >> --- a/m4/package_libcdev.m4 > >> +++ b/m4/package_libcdev.m4 > >> @@ -387,8 +387,7 @@ AC_DEFUN([AC_HAVE_MAP_SYNC], > >> [ AC_MSG_CHECKING([for MAP_SYNC]) > >> AC_COMPILE_IFELSE( > >> [ AC_LANG_PROGRAM([[ > >> -#include > >> -#include > >> +#include > >> ]], [[ > >> int flags = MAP_SYNC | MAP_SHARED_VALIDATE; > >> ]]) > > > > Patch looks good, if you plan to make it into a non-RFC patch, feel free to > > include: > > > > Reviewed-by: Carlos Maiolino > > > > Darrick, do you need to re-post, or can the maintainers pick up the patch directly? I already did: https://lore.kernel.org/linux-xfs/YtmB005kkkErb5uw@magnolia/ (It's August, so I think the xfsprogs maintainer might be on vacation? Either way, I'll make sure he's aware of it before the next release.) --D > -- > Florian