[PATCH] xfsprogs: remove platform_zero_range wrapper

[PATCH] xfsprogs: remove platform_zero_range wrapper

[Eric Sandeen]

Now that the guard around including <linux/falloc.h> in
linux/xfs.h has been removed via
15fb447f ("configure: don't check for fallocate"),
bad things can happen because we reference fallocate in
<xfs/linux.h> without defining _GNU_SOURCE:

$ cat test.c
#include <xfs/linux.h>

int main(void)
{
	return 0;
}

$ gcc -o test test.c
In file included from test.c:1:
/usr/include/xfs/linux.h: In function ‘platform_zero_range’:
/usr/include/xfs/linux.h:186:15: error: implicit declaration of function ‘fallocate’ [-Wimplicit-function-declaration]
  186 |         ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
      |               ^~~~~~~~~

i.e. xfs/linux.h includes fcntl.h without _GNU_SOURCE, so we
don't get an fallocate prototype.

Rather than playing games with header files, just remove the
platform_zero_range() wrapper - we have only one platform, and
only one caller after all - and simply call fallocate directly
if we have the FALLOC_FL_ZERO_RANGE flag defined.

(LTP also runs into this sort of problem at configure time ...)

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

NOTE: compile tested only

diff --git a/include/linux.h b/include/linux.h
index 95a0deee..a13072d2 100644
--- a/include/linux.h
+++ b/include/linux.h
@@ -174,24 +174,6 @@ static inline void platform_mntent_close(struct mntent_cursor * cursor)
 	endmntent(cursor->mtabp);
 }
 
-#if defined(FALLOC_FL_ZERO_RANGE)
-static inline int
-platform_zero_range(
-	int		fd,
-	xfs_off_t	start,
-	size_t		len)
-{
-	int ret;
-
-	ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
-	if (!ret)
-		return 0;
-	return -errno;
-}
-#else
-#define platform_zero_range(fd, s, l)	(-EOPNOTSUPP)
-#endif
-
 /*
  * Use SIGKILL to simulate an immediate program crash, without a chance to run
  * atexit handlers.
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 153007d5..e5b6b5de 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -67,17 +67,19 @@ libxfs_device_zero(struct xfs_buftarg *btp, xfs_daddr_t start, uint len)
 	ssize_t		zsize, bytes;
 	size_t		len_bytes;
 	char		*z;
-	int		error;
+	int		error = 0;
 
 	start_offset = LIBXFS_BBTOOFF64(start);
 
 	/* try to use special zeroing methods, fall back to writes if needed */
 	len_bytes = LIBXFS_BBTOOFF64(len);
-	error = platform_zero_range(fd, start_offset, len_bytes);
+#if defined(FALLOC_FL_ZERO_RANGE)
+	error = fallocate(fd, FALLOC_FL_ZERO_RANGE, start_offset, len_bytes);
 	if (!error) {
 		xfs_buftarg_trip_write(btp);
 		return 0;
 	}
+#endif
 
 	zsize = min(BDSTRAT_SIZE, BBTOB(len));
 	if ((z = memalign(libxfs_device_alignment(), zsize)) == NULL) {

Re: [PATCH] xfsprogs: remove platform_zero_range wrapper

[Darrick J. Wong]

On Wed, Jun 05, 2024 at 10:38:20PM -0500, Eric Sandeen wrote:
> Now that the guard around including <linux/falloc.h> in
> linux/xfs.h has been removed via
> 15fb447f ("configure: don't check for fallocate"),
> bad things can happen because we reference fallocate in
> <xfs/linux.h> without defining _GNU_SOURCE:
> 
> $ cat test.c
> #include <xfs/linux.h>
> 
> int main(void)
> {
> 	return 0;
> }
> 
> $ gcc -o test test.c
> In file included from test.c:1:
> /usr/include/xfs/linux.h: In function ‘platform_zero_range’:
> /usr/include/xfs/linux.h:186:15: error: implicit declaration of function ‘fallocate’ [-Wimplicit-function-declaration]
>   186 |         ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
>       |               ^~~~~~~~~
> 
> i.e. xfs/linux.h includes fcntl.h without _GNU_SOURCE, so we
> don't get an fallocate prototype.
> 
> Rather than playing games with header files, just remove the
> platform_zero_range() wrapper - we have only one platform, and
> only one caller after all - and simply call fallocate directly
> if we have the FALLOC_FL_ZERO_RANGE flag defined.
> 
> (LTP also runs into this sort of problem at configure time ...)
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> NOTE: compile tested only
> 
> diff --git a/include/linux.h b/include/linux.h
> index 95a0deee..a13072d2 100644
> --- a/include/linux.h
> +++ b/include/linux.h
> @@ -174,24 +174,6 @@ static inline void platform_mntent_close(struct mntent_cursor * cursor)
>  	endmntent(cursor->mtabp);
>  }
>  
> -#if defined(FALLOC_FL_ZERO_RANGE)
> -static inline int
> -platform_zero_range(
> -	int		fd,
> -	xfs_off_t	start,
> -	size_t		len)
> -{
> -	int ret;
> -
> -	ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
> -	if (!ret)
> -		return 0;
> -	return -errno;
> -}
> -#else
> -#define platform_zero_range(fd, s, l)	(-EOPNOTSUPP)
> -#endif

Technically speaking, this is an abi change in the xfs library headers
so you can't just yank this without a deprecation period.  That said,
debian codesearch doesn't show any users ... so if there's nothing in
RHEL/Fedora then perhaps it's ok to do that?

Fedora magazine pointed me at "sourcegraph" so I tried:
https://sourcegraph.com/search?q=context:global+repo:%5Esrc.fedoraproject.org/+platform_zero_range&patternType=regexp&sm=0

It shows no callers, but it doesn't show the definition either.

> -
>  /*
>   * Use SIGKILL to simulate an immediate program crash, without a chance to run
>   * atexit handlers.
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 153007d5..e5b6b5de 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -67,17 +67,19 @@ libxfs_device_zero(struct xfs_buftarg *btp, xfs_daddr_t start, uint len)
>  	ssize_t		zsize, bytes;
>  	size_t		len_bytes;
>  	char		*z;
> -	int		error;
> +	int		error = 0;

Is this declaration going to cause build warnings about unused variables
if built on a system that doesn't have FALLOC_FL_ZERO_RANGE?

(Maybe we don't care?)

--D

>  
>  	start_offset = LIBXFS_BBTOOFF64(start);
>  
>  	/* try to use special zeroing methods, fall back to writes if needed */
>  	len_bytes = LIBXFS_BBTOOFF64(len);
> -	error = platform_zero_range(fd, start_offset, len_bytes);
> +#if defined(FALLOC_FL_ZERO_RANGE)
> +	error = fallocate(fd, FALLOC_FL_ZERO_RANGE, start_offset, len_bytes);
>  	if (!error) {
>  		xfs_buftarg_trip_write(btp);
>  		return 0;
>  	}
> +#endif
>  
>  	zsize = min(BDSTRAT_SIZE, BBTOB(len));
>  	if ((z = memalign(libxfs_device_alignment(), zsize)) == NULL) {
> 
> 

Re: [PATCH] xfsprogs: remove platform_zero_range wrapper

[Eric Sandeen]

On 6/6/24 10:28 AM, Darrick J. Wong wrote:
> On Wed, Jun 05, 2024 at 10:38:20PM -0500, Eric Sandeen wrote:
>> Now that the guard around including <linux/falloc.h> in
>> linux/xfs.h has been removed via
>> 15fb447f ("configure: don't check for fallocate"),
>> bad things can happen because we reference fallocate in
>> <xfs/linux.h> without defining _GNU_SOURCE:
>>
>> $ cat test.c
>> #include <xfs/linux.h>
>>
>> int main(void)
>> {
>> 	return 0;
>> }
>>
>> $ gcc -o test test.c
>> In file included from test.c:1:
>> /usr/include/xfs/linux.h: In function ‘platform_zero_range’:
>> /usr/include/xfs/linux.h:186:15: error: implicit declaration of function ‘fallocate’ [-Wimplicit-function-declaration]
>>   186 |         ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
>>       |               ^~~~~~~~~
>>
>> i.e. xfs/linux.h includes fcntl.h without _GNU_SOURCE, so we
>> don't get an fallocate prototype.
>>
>> Rather than playing games with header files, just remove the
>> platform_zero_range() wrapper - we have only one platform, and
>> only one caller after all - and simply call fallocate directly
>> if we have the FALLOC_FL_ZERO_RANGE flag defined.
>>
>> (LTP also runs into this sort of problem at configure time ...)
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> NOTE: compile tested only
>>
>> diff --git a/include/linux.h b/include/linux.h
>> index 95a0deee..a13072d2 100644
>> --- a/include/linux.h
>> +++ b/include/linux.h
>> @@ -174,24 +174,6 @@ static inline void platform_mntent_close(struct mntent_cursor * cursor)
>>  	endmntent(cursor->mtabp);
>>  }
>>  
>> -#if defined(FALLOC_FL_ZERO_RANGE)
>> -static inline int
>> -platform_zero_range(
>> -	int		fd,
>> -	xfs_off_t	start,
>> -	size_t		len)
>> -{
>> -	int ret;
>> -
>> -	ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
>> -	if (!ret)
>> -		return 0;
>> -	return -errno;
>> -}
>> -#else
>> -#define platform_zero_range(fd, s, l)	(-EOPNOTSUPP)
>> -#endif
> 
> Technically speaking, this is an abi change in the xfs library headers
> so you can't just yank this without a deprecation period.  That said,
> debian codesearch doesn't show any users ... so if there's nothing in
> RHEL/Fedora then perhaps it's ok to do that?
> 
> Fedora magazine pointed me at "sourcegraph" so I tried:
> https://sourcegraph.com/search?q=context:global+repo:%5Esrc.fedoraproject.org/+platform_zero_range&patternType=regexp&sm=0
> 
> It shows no callers, but it doesn't show the definition either.

Uh, yeah, I suppose so. It probably never should have been here, as it's
only there for mkfs log discard fun.

I don't see any good way around this. We could #define _GNU_SOURCE at the
top, but if anyone else does:

#include <fcntl.h>
#include <xfs/linux.h> // <- #defines _GNU_SOURCE before fcntl.h

we'd already have the fcntl.h guards and still not enable fallocate.

The only thing that saved us in the past was the guard around including
<falloc.h> because nobody (*) #defined HAVE_FALLOCATE

so arguably removing that guard was an "abi change" because now it's exposed
by default.

(I guess that also means that nobody got platform_zero_range() without
first defining HAVE_FALLOCATE which would be ... unexpected?)

* except LTP at configure time, LOLZ
 
>> -
>>  /*
>>   * Use SIGKILL to simulate an immediate program crash, without a chance to run
>>   * atexit handlers.
>> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
>> index 153007d5..e5b6b5de 100644
>> --- a/libxfs/rdwr.c
>> +++ b/libxfs/rdwr.c
>> @@ -67,17 +67,19 @@ libxfs_device_zero(struct xfs_buftarg *btp, xfs_daddr_t start, uint len)
>>  	ssize_t		zsize, bytes;
>>  	size_t		len_bytes;
>>  	char		*z;
>> -	int		error;
>> +	int		error = 0;
> 
> Is this declaration going to cause build warnings about unused variables
> if built on a system that doesn't have FALLOC_FL_ZERO_RANGE?

I suppose.

> (Maybe we don't care?)

Maybe not!

Maybe I should have omitted the initialization so you didn't notice :P

I could #ifdef around the variable declaration, or I could drop the
error variable altogether and do:

	if (!fallocate(fd, FALLOC_FL_ZERO_RANGE, start_offset, len_bytes)) {
		xfs_buftarg_trip_write(btp);
		return 0;
	}

if that's better?

Thanks,
-Eric

> --D
> 
>>  
>>  	start_offset = LIBXFS_BBTOOFF64(start);
>>  
>>  	/* try to use special zeroing methods, fall back to writes if needed */
>>  	len_bytes = LIBXFS_BBTOOFF64(len);
>> -	error = platform_zero_range(fd, start_offset, len_bytes);
>> +#if defined(FALLOC_FL_ZERO_RANGE)
>> +	error = fallocate(fd, FALLOC_FL_ZERO_RANGE, start_offset, len_bytes);
>>  	if (!error) {
>>  		xfs_buftarg_trip_write(btp);
>>  		return 0;
>>  	}
>> +#endif
>>  
>>  	zsize = min(BDSTRAT_SIZE, BBTOB(len));
>>  	if ((z = memalign(libxfs_device_alignment(), zsize)) == NULL) {
>>
>>
> 

Re: [PATCH] xfsprogs: remove platform_zero_range wrapper

[Darrick J. Wong]

On Thu, Jun 06, 2024 at 02:27:34PM -0500, Eric Sandeen wrote:
> On 6/6/24 10:28 AM, Darrick J. Wong wrote:
> > On Wed, Jun 05, 2024 at 10:38:20PM -0500, Eric Sandeen wrote:
> >> Now that the guard around including <linux/falloc.h> in
> >> linux/xfs.h has been removed via
> >> 15fb447f ("configure: don't check for fallocate"),
> >> bad things can happen because we reference fallocate in
> >> <xfs/linux.h> without defining _GNU_SOURCE:
> >>
> >> $ cat test.c
> >> #include <xfs/linux.h>
> >>
> >> int main(void)
> >> {
> >> 	return 0;
> >> }
> >>
> >> $ gcc -o test test.c
> >> In file included from test.c:1:
> >> /usr/include/xfs/linux.h: In function ‘platform_zero_range’:
> >> /usr/include/xfs/linux.h:186:15: error: implicit declaration of function ‘fallocate’ [-Wimplicit-function-declaration]
> >>   186 |         ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
> >>       |               ^~~~~~~~~
> >>
> >> i.e. xfs/linux.h includes fcntl.h without _GNU_SOURCE, so we
> >> don't get an fallocate prototype.
> >>
> >> Rather than playing games with header files, just remove the
> >> platform_zero_range() wrapper - we have only one platform, and
> >> only one caller after all - and simply call fallocate directly
> >> if we have the FALLOC_FL_ZERO_RANGE flag defined.
> >>
> >> (LTP also runs into this sort of problem at configure time ...)
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> NOTE: compile tested only
> >>
> >> diff --git a/include/linux.h b/include/linux.h
> >> index 95a0deee..a13072d2 100644
> >> --- a/include/linux.h
> >> +++ b/include/linux.h
> >> @@ -174,24 +174,6 @@ static inline void platform_mntent_close(struct mntent_cursor * cursor)
> >>  	endmntent(cursor->mtabp);
> >>  }
> >>  
> >> -#if defined(FALLOC_FL_ZERO_RANGE)
> >> -static inline int
> >> -platform_zero_range(
> >> -	int		fd,
> >> -	xfs_off_t	start,
> >> -	size_t		len)
> >> -{
> >> -	int ret;
> >> -
> >> -	ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
> >> -	if (!ret)
> >> -		return 0;
> >> -	return -errno;
> >> -}
> >> -#else
> >> -#define platform_zero_range(fd, s, l)	(-EOPNOTSUPP)
> >> -#endif
> > 
> > Technically speaking, this is an abi change in the xfs library headers
> > so you can't just yank this without a deprecation period.  That said,
> > debian codesearch doesn't show any users ... so if there's nothing in
> > RHEL/Fedora then perhaps it's ok to do that?
> > 
> > Fedora magazine pointed me at "sourcegraph" so I tried:
> > https://sourcegraph.com/search?q=context:global+repo:%5Esrc.fedoraproject.org/+platform_zero_range&patternType=regexp&sm=0
> > 
> > It shows no callers, but it doesn't show the definition either.
> 
> Uh, yeah, I suppose so. It probably never should have been here, as it's
> only there for mkfs log discard fun.
> 
> I don't see any good way around this. We could #define _GNU_SOURCE at the
> top, but if anyone else does:
> 
> #include <fcntl.h>
> #include <xfs/linux.h> // <- #defines _GNU_SOURCE before fcntl.h
> 
> we'd already have the fcntl.h guards and still not enable fallocate.
> 
> The only thing that saved us in the past was the guard around including
> <falloc.h> because nobody (*) #defined HAVE_FALLOCATE

HAH.  You're right, nobody did taht.

> so arguably removing that guard was an "abi change" because now it's exposed
> by default.
> 
> (I guess that also means that nobody got platform_zero_range() without
> first defining HAVE_FALLOCATE which would be ... unexpected?)
> 
> * except LTP at configure time, LOLZ

Heh.  Ok, this is fine with me then.

> >> -
> >>  /*
> >>   * Use SIGKILL to simulate an immediate program crash, without a chance to run
> >>   * atexit handlers.
> >> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> >> index 153007d5..e5b6b5de 100644
> >> --- a/libxfs/rdwr.c
> >> +++ b/libxfs/rdwr.c
> >> @@ -67,17 +67,19 @@ libxfs_device_zero(struct xfs_buftarg *btp, xfs_daddr_t start, uint len)
> >>  	ssize_t		zsize, bytes;
> >>  	size_t		len_bytes;
> >>  	char		*z;
> >> -	int		error;
> >> +	int		error = 0;
> > 
> > Is this declaration going to cause build warnings about unused variables
> > if built on a system that doesn't have FALLOC_FL_ZERO_RANGE?
> 
> I suppose.
> 
> > (Maybe we don't care?)
> 
> Maybe not!
> 
> Maybe I should have omitted the initialization so you didn't notice :P

Oh I'd have noticed anyway. :P

> I could #ifdef around the variable declaration, or I could drop the
> error variable altogether and do:
> 
> 	if (!fallocate(fd, FALLOC_FL_ZERO_RANGE, start_offset, len_bytes)) {
> 		xfs_buftarg_trip_write(btp);
> 		return 0;
> 	}
> 
> if that's better?

Yeah I guess so.  Better than more #ifdef around the declarations.

--D

> Thanks,
> -Eric
> 
> > --D
> > 
> >>  
> >>  	start_offset = LIBXFS_BBTOOFF64(start);
> >>  
> >>  	/* try to use special zeroing methods, fall back to writes if needed */
> >>  	len_bytes = LIBXFS_BBTOOFF64(len);
> >> -	error = platform_zero_range(fd, start_offset, len_bytes);
> >> +#if defined(FALLOC_FL_ZERO_RANGE)
> >> +	error = fallocate(fd, FALLOC_FL_ZERO_RANGE, start_offset, len_bytes);
> >>  	if (!error) {
> >>  		xfs_buftarg_trip_write(btp);
> >>  		return 0;
> >>  	}
> >> +#endif
> >>  
> >>  	zsize = min(BDSTRAT_SIZE, BBTOB(len));
> >>  	if ((z = memalign(libxfs_device_alignment(), zsize)) == NULL) {
> >>
> >>
> > 
> 
>