[RFC PATCH] libxfs: stop overriding MAP_SYNC in publicly exported header files

[RFC PATCH] libxfs: stop overriding MAP_SYNC in publicly exported header files

[Darrick J. Wong]

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 <djwong@kernel.org>

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 <stdio.h>
#include <sys/mman.h>
#ifdef STUPID
# include <xfs/xfs.h>
#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 <fontaine.fabrice@gmail.com>
Reported-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 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 <asm-generic/mman.h>
-#include <asm-generic/mman-common.h>
-#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 <size> -- 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 <asm-generic/mman.h>
-#include <asm-generic/mman-common.h>
+#include <sys/mman.h>
 	]], [[
 int flags = MAP_SYNC | MAP_SHARED_VALIDATE;
 	]])

Re: [RFC PATCH] libxfs: stop overriding MAP_SYNC in publicly exported header files

[Florian Fainelli]

On 7/20/2022 4:28 PM, 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 <djwong@kernel.org>
> 
> 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 <stdio.h>
> #include <sys/mman.h>
> #ifdef STUPID
> # include <xfs/xfs.h>
> #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 <fontaine.fabrice@gmail.com>
> Reported-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Tested-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!
-- 
Florian

Re: [RFC PATCH] libxfs: stop overriding MAP_SYNC in publicly exported header files

[Carlos Maiolino]

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 <djwong@kernel.org>
> 
> 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 <stdio.h>
> #include <sys/mman.h>
> #ifdef STUPID
> # include <xfs/xfs.h>
> #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 <fontaine.fabrice@gmail.com>
> Reported-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  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 <asm-generic/mman.h>
> -#include <asm-generic/mman-common.h>
> -#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 <size> -- 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 <asm-generic/mman.h>
> -#include <asm-generic/mman-common.h>
> +#include <sys/mman.h>
>  	]], [[
>  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 <cmaiolino@redhat.com>


Cheers.

-- 
Carlos Maiolino

Re: [RFC PATCH] libxfs: stop overriding MAP_SYNC in publicly exported header files

[Florian Fainelli]

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 <djwong@kernel.org>
>>
>> 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 <stdio.h>
>> #include <sys/mman.h>
>> #ifdef STUPID
>> # include <xfs/xfs.h>
>> #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 <fontaine.fabrice@gmail.com>
>> Reported-by: Florian Fainelli <f.fainelli@gmail.com>
>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>> ---
>>  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 <asm-generic/mman.h>
>> -#include <asm-generic/mman-common.h>
>> -#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 <size> -- 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 <asm-generic/mman.h>
>> -#include <asm-generic/mman-common.h>
>> +#include <sys/mman.h>
>>  	]], [[
>>  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 <cmaiolino@redhat.com>
> 

Darrick, do you need to re-post, or can the maintainers pick up the patch directly?
-- 
Florian

Re: [RFC PATCH] libxfs: stop overriding MAP_SYNC in publicly exported header files

[Darrick J. Wong]

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 <djwong@kernel.org>
> >>
> >> 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 <stdio.h>
> >> #include <sys/mman.h>
> >> #ifdef STUPID
> >> # include <xfs/xfs.h>
> >> #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 <fontaine.fabrice@gmail.com>
> >> Reported-by: Florian Fainelli <f.fainelli@gmail.com>
> >> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> >> ---
> >>  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 <asm-generic/mman.h>
> >> -#include <asm-generic/mman-common.h>
> >> -#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 <size> -- 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 <asm-generic/mman.h>
> >> -#include <asm-generic/mman-common.h>
> >> +#include <sys/mman.h>
> >>  	]], [[
> >>  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 <cmaiolino@redhat.com>
> > 
> 
> 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

Re: [RFC PATCH] libxfs: stop overriding MAP_SYNC in publicly exported header files

[Eric Sandeen]

On 7/29/22 10:48 AM, Darrick J. Wong wrote:
>> 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
> 

Yep I was, picking it up now. Thanks for the pointer Darrick.

-Eric

Re: [RFC PATCH] libxfs: stop overriding MAP_SYNC in publicly exported header files

[Florian Fainelli]

On 8/4/22 19:11, Eric Sandeen wrote:
> 
> 
> On 7/29/22 10:48 AM, Darrick J. Wong wrote:
>>> 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
>>
> 
> Yep I was, picking it up now. Thanks for the pointer Darrick.
> 
> -Eric

Eric, any chance this could be pushed to xfsprogs-dev.git so we can take 
the patch and submit it to buildroot, OpenWrt and other projects that 
depend upon that build fix?

Thanks!
-- 
Florian

Re: [RFC PATCH] libxfs: stop overriding MAP_SYNC in publicly exported header files

[Darrick J. Wong]

On Mon, Aug 08, 2022 at 10:13:06AM -0700, Florian Fainelli wrote:
> On 8/4/22 19:11, Eric Sandeen wrote:
> > 
> > 
> > On 7/29/22 10:48 AM, Darrick J. Wong wrote:
> > > > 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
> > > 
> > 
> > Yep I was, picking it up now. Thanks for the pointer Darrick.
> > 
> > -Eric
> 
> Eric, any chance this could be pushed to xfsprogs-dev.git so we can take the
> patch and submit it to buildroot, OpenWrt and other projects that depend
> upon that build fix?

It's queued in for-next, so the git commit ids should be stable if you
want to get that started now.

https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/commit/?h=for-next&id=28965957f4ea5c79fc0b91b997168c656a4426c5

--D

> Thanks!
> -- 
> Florian

Re: [RFC PATCH] libxfs: stop overriding MAP_SYNC in publicly exported header files

[Florian Fainelli]

On 8/8/22 10:36, Darrick J. Wong wrote:
> On Mon, Aug 08, 2022 at 10:13:06AM -0700, Florian Fainelli wrote:
>> On 8/4/22 19:11, Eric Sandeen wrote:
>>>
>>>
>>> On 7/29/22 10:48 AM, Darrick J. Wong wrote:
>>>>> 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
>>>>
>>>
>>> Yep I was, picking it up now. Thanks for the pointer Darrick.
>>>
>>> -Eric
>>
>> Eric, any chance this could be pushed to xfsprogs-dev.git so we can take the
>> patch and submit it to buildroot, OpenWrt and other projects that depend
>> upon that build fix?
> 
> It's queued in for-next, so the git commit ids should be stable if you
> want to get that started now.
> 
> https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/commit/?h=for-next&id=28965957f4ea5c79fc0b91b997168c656a4426c5

Woah, sorry for not noticing, thanks!
-- 
Florian