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