Re: [PATCH] xfsprogs: remove platform_zero_range wrapper

["Darrick J. Wong" <djwong@kernel.org>]

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