| Message ID | a216140e-1c8a-4d04-ba46-670646498622@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfsprogs: remove platform_zero_range wrapper | expand |
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) { > >
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) { >> >> >
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) { > >> > >> > > > >
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) {
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