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@xxxxxxxxxx>
>> ---
>>
>> 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) {
>>
>>
>
