| Message ID | 20240926144147.106685-3-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsx: support unshare range fallocate mode | expand |
On Thu, Sep 26, 2024 at 10:41:47AM -0400, Brian Foster wrote: > The various fallocate flags are mostly ifdef'd for backward > compatibility with the exception of the associated test_fallocate() > calls to verify functionality at runtime. I suspect the reason for > this was to avoid ifdef ugliness around having to clear the runtime > flag for each operation, but unfortunately this defeats the purpose > of the ifdef protection everywhere else. > > Factor out the fallocate related test calls into a new helper and > add the appropriate ifdefs. > > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > ltp/fsx.c | 59 ++++++++++++++++++++++++++++++++++++++++++------------- > 1 file changed, 45 insertions(+), 14 deletions(-) > > diff --git a/ltp/fsx.c b/ltp/fsx.c > index 677f8c9f..417743c5 100644 > --- a/ltp/fsx.c > +++ b/ltp/fsx.c > @@ -2833,6 +2833,50 @@ __test_fallocate(int mode, const char *mode_str) > #endif > } > > +void > +test_fallocate_calls(void) > +{ > + if (fallocate_calls) > + fallocate_calls = test_fallocate(0); > + if (keep_size_calls) > + keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > + > +#ifdef FALLOC_FL_UNSHARE_RANGE > + if (unshare_range_calls) > + unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > +#else > + unshare_range_calls = 0; > +#endif > + > +#ifdef FALLOC_FL_PUNCH_HOLE > + if (punch_hole_calls) > + punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > +#else > + punch_hole_calls = 0; > +#endif > + > +#ifdef FALLOC_FL_ZERO_RANGE > + if (zero_range_calls) > + zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > +#else > + zero_range_calls = 0; > +#endif > + > +#ifdef FALLOC_FL_COLLAPSE_RANGE > + if (collapse_range_calls) > + collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > +#else > + collapse_range_calls = 0; > +#endif The concept looks fine, but collapse and zero range have been in the kernel for a decade now, do we really need to have ifdef tests for them? --D > + > +#ifdef FALLOC_FL_INSERT_RANGE > + if (insert_range_calls) > + insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > +#else > + insert_range_calls = 0; > +#endif > +} > + > bool > keep_running(void) > { > @@ -3271,20 +3315,7 @@ main(int argc, char **argv) > check_trunc_hack(); > } > > - if (fallocate_calls) > - fallocate_calls = test_fallocate(0); > - if (keep_size_calls) > - keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > - if (unshare_range_calls) > - unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > - if (punch_hole_calls) > - punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > - if (zero_range_calls) > - zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > - if (collapse_range_calls) > - collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > - if (insert_range_calls) > - insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > + test_fallocate_calls(); > if (clone_range_calls) > clone_range_calls = test_clone_range(); > if (dedupe_range_calls) > -- > 2.46.1 > >
On Thu, Sep 26, 2024 at 07:50:28AM -0700, Darrick J. Wong wrote: > On Thu, Sep 26, 2024 at 10:41:47AM -0400, Brian Foster wrote: > > The various fallocate flags are mostly ifdef'd for backward > > compatibility with the exception of the associated test_fallocate() > > calls to verify functionality at runtime. I suspect the reason for > > this was to avoid ifdef ugliness around having to clear the runtime > > flag for each operation, but unfortunately this defeats the purpose > > of the ifdef protection everywhere else. > > > > Factor out the fallocate related test calls into a new helper and > > add the appropriate ifdefs. > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > --- > > ltp/fsx.c | 59 ++++++++++++++++++++++++++++++++++++++++++------------- > > 1 file changed, 45 insertions(+), 14 deletions(-) > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c > > index 677f8c9f..417743c5 100644 > > --- a/ltp/fsx.c > > +++ b/ltp/fsx.c > > @@ -2833,6 +2833,50 @@ __test_fallocate(int mode, const char *mode_str) > > #endif > > } > > > > +void > > +test_fallocate_calls(void) > > +{ > > + if (fallocate_calls) > > + fallocate_calls = test_fallocate(0); > > + if (keep_size_calls) > > + keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > > + > > +#ifdef FALLOC_FL_UNSHARE_RANGE > > + if (unshare_range_calls) > > + unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > > +#else > > + unshare_range_calls = 0; > > +#endif > > + > > +#ifdef FALLOC_FL_PUNCH_HOLE > > + if (punch_hole_calls) > > + punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > > +#else > > + punch_hole_calls = 0; > > +#endif > > + > > +#ifdef FALLOC_FL_ZERO_RANGE > > + if (zero_range_calls) > > + zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > > +#else > > + zero_range_calls = 0; > > +#endif > > + > > +#ifdef FALLOC_FL_COLLAPSE_RANGE > > + if (collapse_range_calls) > > + collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > > +#else > > + collapse_range_calls = 0; > > +#endif > > The concept looks fine, but collapse and zero range have been in the > kernel for a decade now, do we really need to have ifdef tests for them? > Probably not.. but why even bother worrying about individual flags? The insert and unshare flags have been around for 9 and 8 years respectively, none of these were fully ifdef'd from the beginning, and I'm not aware of anyone that has actually complained. I'm not convinced that this patch matters for anybody in practice. I included it just because it was simple enough to include the minimum mechanical fix and I was slightly curious if somebody could come up with a more elegant solution. In the spirit of being practical, maybe the better approach here is to just remove the (at least the falloc flag related) ifdefs entirely? We can always add them back if somebody complains... Brian > --D > > > + > > +#ifdef FALLOC_FL_INSERT_RANGE > > + if (insert_range_calls) > > + insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > > +#else > > + insert_range_calls = 0; > > +#endif > > +} > > + > > bool > > keep_running(void) > > { > > @@ -3271,20 +3315,7 @@ main(int argc, char **argv) > > check_trunc_hack(); > > } > > > > - if (fallocate_calls) > > - fallocate_calls = test_fallocate(0); > > - if (keep_size_calls) > > - keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > > - if (unshare_range_calls) > > - unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > > - if (punch_hole_calls) > > - punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > > - if (zero_range_calls) > > - zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > > - if (collapse_range_calls) > > - collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > > - if (insert_range_calls) > > - insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > > + test_fallocate_calls(); > > if (clone_range_calls) > > clone_range_calls = test_clone_range(); > > if (dedupe_range_calls) > > -- > > 2.46.1 > > > > >
On Thu, Sep 26, 2024 at 11:55:21AM -0400, Brian Foster wrote: > On Thu, Sep 26, 2024 at 07:50:28AM -0700, Darrick J. Wong wrote: > > On Thu, Sep 26, 2024 at 10:41:47AM -0400, Brian Foster wrote: > > > The various fallocate flags are mostly ifdef'd for backward > > > compatibility with the exception of the associated test_fallocate() > > > calls to verify functionality at runtime. I suspect the reason for > > > this was to avoid ifdef ugliness around having to clear the runtime > > > flag for each operation, but unfortunately this defeats the purpose > > > of the ifdef protection everywhere else. > > > > > > Factor out the fallocate related test calls into a new helper and > > > add the appropriate ifdefs. > > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > --- > > > ltp/fsx.c | 59 ++++++++++++++++++++++++++++++++++++++++++------------- > > > 1 file changed, 45 insertions(+), 14 deletions(-) > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c > > > index 677f8c9f..417743c5 100644 > > > --- a/ltp/fsx.c > > > +++ b/ltp/fsx.c > > > @@ -2833,6 +2833,50 @@ __test_fallocate(int mode, const char *mode_str) > > > #endif > > > } > > > > > > +void > > > +test_fallocate_calls(void) > > > +{ > > > + if (fallocate_calls) > > > + fallocate_calls = test_fallocate(0); > > > + if (keep_size_calls) > > > + keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > > > + > > > +#ifdef FALLOC_FL_UNSHARE_RANGE > > > + if (unshare_range_calls) > > > + unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > > > +#else > > > + unshare_range_calls = 0; > > > +#endif > > > + > > > +#ifdef FALLOC_FL_PUNCH_HOLE > > > + if (punch_hole_calls) > > > + punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > > > +#else > > > + punch_hole_calls = 0; > > > +#endif > > > + > > > +#ifdef FALLOC_FL_ZERO_RANGE > > > + if (zero_range_calls) > > > + zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > > > +#else > > > + zero_range_calls = 0; > > > +#endif > > > + > > > +#ifdef FALLOC_FL_COLLAPSE_RANGE > > > + if (collapse_range_calls) > > > + collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > > > +#else > > > + collapse_range_calls = 0; > > > +#endif > > > > The concept looks fine, but collapse and zero range have been in the > > kernel for a decade now, do we really need to have ifdef tests for them? > > > > Probably not.. but why even bother worrying about individual flags? The > insert and unshare flags have been around for 9 and 8 years > respectively, none of these were fully ifdef'd from the beginning, and > I'm not aware of anyone that has actually complained. > > I'm not convinced that this patch matters for anybody in practice. I > included it just because it was simple enough to include the minimum > mechanical fix and I was slightly curious if somebody could come up with > a more elegant solution. In the spirit of being practical, maybe the > better approach here is to just remove the (at least the falloc flag > related) ifdefs entirely? We can always add them back if somebody > complains... As this patch is still controversial, I'll merge the other one at first, to catch up the release of this week. We can talk this one later, if you still hope to have it :) Thanks, Zorro > > Brian > > > --D > > > > > + > > > +#ifdef FALLOC_FL_INSERT_RANGE > > > + if (insert_range_calls) > > > + insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > > > +#else > > > + insert_range_calls = 0; > > > +#endif > > > +} > > > + > > > bool > > > keep_running(void) > > > { > > > @@ -3271,20 +3315,7 @@ main(int argc, char **argv) > > > check_trunc_hack(); > > > } > > > > > > - if (fallocate_calls) > > > - fallocate_calls = test_fallocate(0); > > > - if (keep_size_calls) > > > - keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > > > - if (unshare_range_calls) > > > - unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > > > - if (punch_hole_calls) > > > - punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > > > - if (zero_range_calls) > > > - zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > > > - if (collapse_range_calls) > > > - collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > > > - if (insert_range_calls) > > > - insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > > > + test_fallocate_calls(); > > > if (clone_range_calls) > > > clone_range_calls = test_clone_range(); > > > if (dedupe_range_calls) > > > -- > > > 2.46.1 > > > > > > > > > >
On Fri, Sep 27, 2024 at 01:42:31PM +0800, Zorro Lang wrote: > On Thu, Sep 26, 2024 at 11:55:21AM -0400, Brian Foster wrote: > > On Thu, Sep 26, 2024 at 07:50:28AM -0700, Darrick J. Wong wrote: > > > On Thu, Sep 26, 2024 at 10:41:47AM -0400, Brian Foster wrote: > > > > The various fallocate flags are mostly ifdef'd for backward > > > > compatibility with the exception of the associated test_fallocate() > > > > calls to verify functionality at runtime. I suspect the reason for > > > > this was to avoid ifdef ugliness around having to clear the runtime > > > > flag for each operation, but unfortunately this defeats the purpose > > > > of the ifdef protection everywhere else. > > > > > > > > Factor out the fallocate related test calls into a new helper and > > > > add the appropriate ifdefs. > > > > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > > --- > > > > ltp/fsx.c | 59 ++++++++++++++++++++++++++++++++++++++++++------------- > > > > 1 file changed, 45 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c > > > > index 677f8c9f..417743c5 100644 > > > > --- a/ltp/fsx.c > > > > +++ b/ltp/fsx.c > > > > @@ -2833,6 +2833,50 @@ __test_fallocate(int mode, const char *mode_str) > > > > #endif > > > > } > > > > > > > > +void > > > > +test_fallocate_calls(void) > > > > +{ > > > > + if (fallocate_calls) > > > > + fallocate_calls = test_fallocate(0); > > > > + if (keep_size_calls) > > > > + keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > > > > + > > > > +#ifdef FALLOC_FL_UNSHARE_RANGE > > > > + if (unshare_range_calls) > > > > + unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > > > > +#else > > > > + unshare_range_calls = 0; > > > > +#endif > > > > + > > > > +#ifdef FALLOC_FL_PUNCH_HOLE > > > > + if (punch_hole_calls) > > > > + punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > > > > +#else > > > > + punch_hole_calls = 0; > > > > +#endif > > > > + > > > > +#ifdef FALLOC_FL_ZERO_RANGE > > > > + if (zero_range_calls) > > > > + zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > > > > +#else > > > > + zero_range_calls = 0; > > > > +#endif > > > > + > > > > +#ifdef FALLOC_FL_COLLAPSE_RANGE > > > > + if (collapse_range_calls) > > > > + collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > > > > +#else > > > > + collapse_range_calls = 0; > > > > +#endif > > > > > > The concept looks fine, but collapse and zero range have been in the > > > kernel for a decade now, do we really need to have ifdef tests for them? > > > > > > > Probably not.. but why even bother worrying about individual flags? The > > insert and unshare flags have been around for 9 and 8 years > > respectively, none of these were fully ifdef'd from the beginning, and > > I'm not aware of anyone that has actually complained. > > > > I'm not convinced that this patch matters for anybody in practice. I > > included it just because it was simple enough to include the minimum > > mechanical fix and I was slightly curious if somebody could come up with > > a more elegant solution. In the spirit of being practical, maybe the > > better approach here is to just remove the (at least the falloc flag > > related) ifdefs entirely? We can always add them back if somebody > > complains... > > As this patch is still controversial, I'll merge the other one at first, to > catch up the release of this week. We can talk this one later, if you still > hope to have it :) > Thanks. In thinking more about it.. my reasoning above was that it seems like the value of these ifdefs is to avoid disruption when new functionality is introduced, but at the same time the fstests user base may not be necessarily all that interested in eternal backwards compatibility for ancient runtimes, etc. Therefore, I wonder if it's reasonable to have an (informal) expiration date for when we can clear out some of this cruft to keep the code cleaner and more maintainable going forward. So I largely agree with Darrick's point, it's just that personally I'm less interested in discussion over which fallocate flags to include or not because to my mind that suggests we might as well just drop the ifdefs entirely. That said, I'm not all that invested beyond just trying to be proactive since I happened to be hacking in this area, so if you guys want to leave things as is, or agree on a subset of flags to ifdef, just let me know and I'll drop it or send a v2. Brian > Thanks, > Zorro > > > > > Brian > > > > > --D > > > > > > > + > > > > +#ifdef FALLOC_FL_INSERT_RANGE > > > > + if (insert_range_calls) > > > > + insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > > > > +#else > > > > + insert_range_calls = 0; > > > > +#endif > > > > +} > > > > + > > > > bool > > > > keep_running(void) > > > > { > > > > @@ -3271,20 +3315,7 @@ main(int argc, char **argv) > > > > check_trunc_hack(); > > > > } > > > > > > > > - if (fallocate_calls) > > > > - fallocate_calls = test_fallocate(0); > > > > - if (keep_size_calls) > > > > - keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > > > > - if (unshare_range_calls) > > > > - unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > > > > - if (punch_hole_calls) > > > > - punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > > > > - if (zero_range_calls) > > > > - zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > > > > - if (collapse_range_calls) > > > > - collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > > > > - if (insert_range_calls) > > > > - insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > > > > + test_fallocate_calls(); > > > > if (clone_range_calls) > > > > clone_range_calls = test_clone_range(); > > > > if (dedupe_range_calls) > > > > -- > > > > 2.46.1 > > > > > > > > > > > > > > > >
On Fri, Sep 27, 2024 at 08:07:49AM -0400, Brian Foster wrote: > On Fri, Sep 27, 2024 at 01:42:31PM +0800, Zorro Lang wrote: > > On Thu, Sep 26, 2024 at 11:55:21AM -0400, Brian Foster wrote: > > > On Thu, Sep 26, 2024 at 07:50:28AM -0700, Darrick J. Wong wrote: > > > > On Thu, Sep 26, 2024 at 10:41:47AM -0400, Brian Foster wrote: > > > > > The various fallocate flags are mostly ifdef'd for backward > > > > > compatibility with the exception of the associated test_fallocate() > > > > > calls to verify functionality at runtime. I suspect the reason for > > > > > this was to avoid ifdef ugliness around having to clear the runtime > > > > > flag for each operation, but unfortunately this defeats the purpose > > > > > of the ifdef protection everywhere else. > > > > > > > > > > Factor out the fallocate related test calls into a new helper and > > > > > add the appropriate ifdefs. > > > > > > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > > > --- > > > > > ltp/fsx.c | 59 ++++++++++++++++++++++++++++++++++++++++++------------- > > > > > 1 file changed, 45 insertions(+), 14 deletions(-) > > > > > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c > > > > > index 677f8c9f..417743c5 100644 > > > > > --- a/ltp/fsx.c > > > > > +++ b/ltp/fsx.c > > > > > @@ -2833,6 +2833,50 @@ __test_fallocate(int mode, const char *mode_str) > > > > > #endif > > > > > } > > > > > > > > > > +void > > > > > +test_fallocate_calls(void) > > > > > +{ > > > > > + if (fallocate_calls) > > > > > + fallocate_calls = test_fallocate(0); > > > > > + if (keep_size_calls) > > > > > + keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > > > > > + > > > > > +#ifdef FALLOC_FL_UNSHARE_RANGE > > > > > + if (unshare_range_calls) > > > > > + unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > > > > > +#else > > > > > + unshare_range_calls = 0; > > > > > +#endif > > > > > + > > > > > +#ifdef FALLOC_FL_PUNCH_HOLE > > > > > + if (punch_hole_calls) > > > > > + punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > > > > > +#else > > > > > + punch_hole_calls = 0; > > > > > +#endif > > > > > + > > > > > +#ifdef FALLOC_FL_ZERO_RANGE > > > > > + if (zero_range_calls) > > > > > + zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > > > > > +#else > > > > > + zero_range_calls = 0; > > > > > +#endif > > > > > + > > > > > +#ifdef FALLOC_FL_COLLAPSE_RANGE > > > > > + if (collapse_range_calls) > > > > > + collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > > > > > +#else > > > > > + collapse_range_calls = 0; > > > > > +#endif > > > > > > > > The concept looks fine, but collapse and zero range have been in the > > > > kernel for a decade now, do we really need to have ifdef tests for them? > > > > > > > > > > Probably not.. but why even bother worrying about individual flags? The > > > insert and unshare flags have been around for 9 and 8 years > > > respectively, none of these were fully ifdef'd from the beginning, and > > > I'm not aware of anyone that has actually complained. > > > > > > I'm not convinced that this patch matters for anybody in practice. I > > > included it just because it was simple enough to include the minimum > > > mechanical fix and I was slightly curious if somebody could come up with > > > a more elegant solution. In the spirit of being practical, maybe the > > > better approach here is to just remove the (at least the falloc flag > > > related) ifdefs entirely? We can always add them back if somebody > > > complains... > > > > As this patch is still controversial, I'll merge the other one at first, to > > catch up the release of this week. We can talk this one later, if you still > > hope to have it :) > > > > Thanks. In thinking more about it.. my reasoning above was that it seems > like the value of these ifdefs is to avoid disruption when new > functionality is introduced, but at the same time the fstests user base > may not be necessarily all that interested in eternal backwards > compatibility for ancient runtimes, etc. Therefore, I wonder if it's > reasonable to have an (informal) expiration date for when we can clear > out some of this cruft to keep the code cleaner and more maintainable > going forward. > > So I largely agree with Darrick's point, it's just that personally I'm > less interested in discussion over which fallocate flags to include or > not because to my mind that suggests we might as well just drop the > ifdefs entirely. That said, I'm not all that invested beyond just trying > to be proactive since I happened to be hacking in this area, so if you > guys want to leave things as is, or agree on a subset of flags to ifdef, > just let me know and I'll drop it or send a v2. Usually I just let Christoph complain and remove the ifdefs, but if I have to use my own rule, it would be that ifdefs and ./configure trickery isn't necessary for any symbol that is at least 5 years old. Recent complaints on the mailing list have caused me to revise that to 10 years old though (see recent memfd_create fixes). :) I also remember that a lot of the old crufty ifdef stuff (iirc) was kept around so that fstests would continue to run on old RHELs. Once in a while our QA folks rebase fstests to latest, but they also tend to patch back in whatever ./configure magic they need for 2.6 era kernels. --D > Brian > > > > Thanks, > > Zorro > > > > > > > > Brian > > > > > > > --D > > > > > > > > > + > > > > > +#ifdef FALLOC_FL_INSERT_RANGE > > > > > + if (insert_range_calls) > > > > > + insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > > > > > +#else > > > > > + insert_range_calls = 0; > > > > > +#endif > > > > > +} > > > > > + > > > > > bool > > > > > keep_running(void) > > > > > { > > > > > @@ -3271,20 +3315,7 @@ main(int argc, char **argv) > > > > > check_trunc_hack(); > > > > > } > > > > > > > > > > - if (fallocate_calls) > > > > > - fallocate_calls = test_fallocate(0); > > > > > - if (keep_size_calls) > > > > > - keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > > > > > - if (unshare_range_calls) > > > > > - unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > > > > > - if (punch_hole_calls) > > > > > - punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > > > > > - if (zero_range_calls) > > > > > - zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > > > > > - if (collapse_range_calls) > > > > > - collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > > > > > - if (insert_range_calls) > > > > > - insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > > > > > + test_fallocate_calls(); > > > > > if (clone_range_calls) > > > > > clone_range_calls = test_clone_range(); > > > > > if (dedupe_range_calls) > > > > > -- > > > > > 2.46.1 > > > > > > > > > > > > > > > > > > > > > > >
On Fri, Sep 27, 2024 at 08:25:37AM -0700, Darrick J. Wong wrote: > On Fri, Sep 27, 2024 at 08:07:49AM -0400, Brian Foster wrote: > > On Fri, Sep 27, 2024 at 01:42:31PM +0800, Zorro Lang wrote: > > > On Thu, Sep 26, 2024 at 11:55:21AM -0400, Brian Foster wrote: > > > > On Thu, Sep 26, 2024 at 07:50:28AM -0700, Darrick J. Wong wrote: > > > > > On Thu, Sep 26, 2024 at 10:41:47AM -0400, Brian Foster wrote: > > > > > > The various fallocate flags are mostly ifdef'd for backward > > > > > > compatibility with the exception of the associated test_fallocate() > > > > > > calls to verify functionality at runtime. I suspect the reason for > > > > > > this was to avoid ifdef ugliness around having to clear the runtime > > > > > > flag for each operation, but unfortunately this defeats the purpose > > > > > > of the ifdef protection everywhere else. > > > > > > > > > > > > Factor out the fallocate related test calls into a new helper and > > > > > > add the appropriate ifdefs. > > > > > > > > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > > > > --- > > > > > > ltp/fsx.c | 59 ++++++++++++++++++++++++++++++++++++++++++------------- > > > > > > 1 file changed, 45 insertions(+), 14 deletions(-) > > > > > > > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c > > > > > > index 677f8c9f..417743c5 100644 > > > > > > --- a/ltp/fsx.c > > > > > > +++ b/ltp/fsx.c > > > > > > @@ -2833,6 +2833,50 @@ __test_fallocate(int mode, const char *mode_str) > > > > > > #endif > > > > > > } > > > > > > > > > > > > +void > > > > > > +test_fallocate_calls(void) > > > > > > +{ > > > > > > + if (fallocate_calls) > > > > > > + fallocate_calls = test_fallocate(0); > > > > > > + if (keep_size_calls) > > > > > > + keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > > > > > > + > > > > > > +#ifdef FALLOC_FL_UNSHARE_RANGE > > > > > > + if (unshare_range_calls) > > > > > > + unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > > > > > > +#else > > > > > > + unshare_range_calls = 0; > > > > > > +#endif > > > > > > + > > > > > > +#ifdef FALLOC_FL_PUNCH_HOLE > > > > > > + if (punch_hole_calls) > > > > > > + punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > > > > > > +#else > > > > > > + punch_hole_calls = 0; > > > > > > +#endif > > > > > > + > > > > > > +#ifdef FALLOC_FL_ZERO_RANGE > > > > > > + if (zero_range_calls) > > > > > > + zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > > > > > > +#else > > > > > > + zero_range_calls = 0; > > > > > > +#endif > > > > > > + > > > > > > +#ifdef FALLOC_FL_COLLAPSE_RANGE > > > > > > + if (collapse_range_calls) > > > > > > + collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > > > > > > +#else > > > > > > + collapse_range_calls = 0; > > > > > > +#endif > > > > > > > > > > The concept looks fine, but collapse and zero range have been in the > > > > > kernel for a decade now, do we really need to have ifdef tests for them? > > > > > > > > > > > > > Probably not.. but why even bother worrying about individual flags? The > > > > insert and unshare flags have been around for 9 and 8 years > > > > respectively, none of these were fully ifdef'd from the beginning, and > > > > I'm not aware of anyone that has actually complained. > > > > > > > > I'm not convinced that this patch matters for anybody in practice. I > > > > included it just because it was simple enough to include the minimum > > > > mechanical fix and I was slightly curious if somebody could come up with > > > > a more elegant solution. In the spirit of being practical, maybe the > > > > better approach here is to just remove the (at least the falloc flag > > > > related) ifdefs entirely? We can always add them back if somebody > > > > complains... > > > > > > As this patch is still controversial, I'll merge the other one at first, to > > > catch up the release of this week. We can talk this one later, if you still > > > hope to have it :) > > > > > > > Thanks. In thinking more about it.. my reasoning above was that it seems > > like the value of these ifdefs is to avoid disruption when new > > functionality is introduced, but at the same time the fstests user base > > may not be necessarily all that interested in eternal backwards > > compatibility for ancient runtimes, etc. Therefore, I wonder if it's > > reasonable to have an (informal) expiration date for when we can clear > > out some of this cruft to keep the code cleaner and more maintainable > > going forward. > > > > So I largely agree with Darrick's point, it's just that personally I'm > > less interested in discussion over which fallocate flags to include or > > not because to my mind that suggests we might as well just drop the > > ifdefs entirely. That said, I'm not all that invested beyond just trying > > to be proactive since I happened to be hacking in this area, so if you > > guys want to leave things as is, or agree on a subset of flags to ifdef, > > just let me know and I'll drop it or send a v2. > > Usually I just let Christoph complain and remove the ifdefs, but if I > have to use my own rule, it would be that ifdefs and ./configure > trickery isn't necessary for any symbol that is at least 5 years old. > > Recent complaints on the mailing list have caused me to revise that to > 10 years old though (see recent memfd_create fixes). :) > > I also remember that a lot of the old crufty ifdef stuff (iirc) was kept > around so that fstests would continue to run on old RHELs. Once in a > while our QA folks rebase fstests to latest, but they also tend to patch > back in whatever ./configure magic they need for 2.6 era kernels. > That's kind of what I figured.. even if folks were running on old envs, maybe there's enough incompatibility to work through in extreme enough cases that customization is required anyways and we just don't hear about it upstream. Anyways, that all seems reasonable to me. Given that it looks like this has been broken for the last 8+ years or so, attached is an alternative proposal to slightly bend your 10 year rule in this case... ;). As before, this is not much of a big deal to me either way. Zorro, this kind of goes in the opposite direction of your comments to patch 1 where I added the missing ifdefs, so I'd defer to you as to whether you want to go this route or just leave things as is for the time being. The attached patch also removes the ifdefs added in patch 1, so there should be no need to edit patch 1 either way. Thanks. Brian > --D > > > Brian > > > > > > > Thanks, > > > Zorro > > > > > > > > > > > Brian > > > > > > > > > --D > > > > > > > > > > > + > > > > > > +#ifdef FALLOC_FL_INSERT_RANGE > > > > > > + if (insert_range_calls) > > > > > > + insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > > > > > > +#else > > > > > > + insert_range_calls = 0; > > > > > > +#endif > > > > > > +} > > > > > > + > > > > > > bool > > > > > > keep_running(void) > > > > > > { > > > > > > @@ -3271,20 +3315,7 @@ main(int argc, char **argv) > > > > > > check_trunc_hack(); > > > > > > } > > > > > > > > > > > > - if (fallocate_calls) > > > > > > - fallocate_calls = test_fallocate(0); > > > > > > - if (keep_size_calls) > > > > > > - keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > > > > > > - if (unshare_range_calls) > > > > > > - unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > > > > > > - if (punch_hole_calls) > > > > > > - punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > > > > > > - if (zero_range_calls) > > > > > > - zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > > > > > > - if (collapse_range_calls) > > > > > > - collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > > > > > > - if (insert_range_calls) > > > > > > - insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > > > > > > + test_fallocate_calls(); > > > > > > if (clone_range_calls) > > > > > > clone_range_calls = test_clone_range(); > > > > > > if (dedupe_range_calls) > > > > > > -- > > > > > > 2.46.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > From 710f88a08a978c2c6ae2e9f92a35da08aa45db72 Mon Sep 17 00:00:00 2001 From: Brian Foster <bfoster@redhat.com> Date: Fri, 27 Sep 2024 14:08:50 -0400 Subject: [PATCH] fsx: remove unnecessary fallocate flag ifdefs Content-Type: text/plain fallocate mode flags are typically ifdef'd when introduced to provide some backwards compatibility and minimize disruption for users who might not have picked up the latest supporting bits in their dev and test environments. As it is, most of these flags have been used outside of ifdef protection for runtime functionality detection for quite some time, so this apparently hasn't been providing much practical benefit. fallocate runtime functionality detection goes back 8-10 years now in some cases, without any known complaints upstream. Therefore, clean out some of the older ifdef cruft as a housekeeping exercise. This can be undone if it turns out to be problematic for any current fstests users. Signed-off-by: Brian Foster <bfoster@redhat.com> --- ltp/fsx.c | 77 +++++-------------------------------------------------- 1 file changed, 7 insertions(+), 70 deletions(-) diff --git a/ltp/fsx.c b/ltp/fsx.c index 677f8c9f..db8ee914 100644 --- a/ltp/fsx.c +++ b/ltp/fsx.c @@ -1231,7 +1231,6 @@ dotruncate(unsigned size) } } -#ifdef FALLOC_FL_PUNCH_HOLE void do_punch_hole(unsigned offset, unsigned length) { @@ -1280,15 +1279,6 @@ do_punch_hole(unsigned offset, unsigned length) memset(good_buf + max_offset, '\0', max_len); } -#else -void -do_punch_hole(unsigned offset, unsigned length) -{ - return; -} -#endif - -#ifdef FALLOC_FL_ZERO_RANGE void do_zero_range(unsigned offset, unsigned length, int keep_size) { @@ -1344,15 +1334,6 @@ do_zero_range(unsigned offset, unsigned length, int keep_size) memset(good_buf + offset, '\0', length); } -#else -void -do_zero_range(unsigned offset, unsigned length, int keep_size) -{ - return; -} -#endif - -#ifdef FALLOC_FL_COLLAPSE_RANGE void do_collapse_range(unsigned offset, unsigned length) { @@ -1399,15 +1380,6 @@ do_collapse_range(unsigned offset, unsigned length) file_size -= length; } -#else -void -do_collapse_range(unsigned offset, unsigned length) -{ - return; -} -#endif - -#ifdef FALLOC_FL_INSERT_RANGE void do_insert_range(unsigned offset, unsigned length) { @@ -1455,15 +1427,6 @@ do_insert_range(unsigned offset, unsigned length) file_size += length; } -#else -void -do_insert_range(unsigned offset, unsigned length) -{ - return; -} -#endif - -#ifdef XFS_IOC_EXCHANGE_RANGE int test_exchange_range(void) { @@ -1546,20 +1509,6 @@ out_free: free(p); } -#else -int -test_exchange_range(void) -{ - return 0; -} - -void -do_exchange_range(unsigned offset, unsigned length, unsigned dest) -{ - return; -} -#endif - #ifdef FICLONERANGE int test_clone_range(void) @@ -1897,12 +1846,10 @@ do_preallocate(unsigned offset, unsigned length, int keep_size, int unshare) opflags |= FL_KEEP_SIZE; mode |= FALLOC_FL_KEEP_SIZE; } -#ifdef FALLOC_FL_UNSHARE_RANGE if (unshare) { opflags |= FL_UNSHARE; mode |= FALLOC_FL_UNSHARE_RANGE; } -#endif if (length == 0) { if (!quiet && testcalls > simulatedopcount) @@ -2495,11 +2442,9 @@ usage(void) -q: quieter operation\n\ -r readbdy: 4096 would make reads page aligned (default 1)\n\ -s style: 1 gives smaller truncates (default 0)\n\ - -t truncbdy: 4096 would make truncates page aligned (default 1)\n" -#ifdef FALLOC_FL_UNSHARE_RANGE -" -u Do not use unshare range\n" -#endif -" -w writebdy: 4096 would make writes page aligned (default 1)\n\ + -t truncbdy: 4096 would make truncates page aligned (default 1)\n\ + -u Do not use unshare range\n\ + -w writebdy: 4096 would make writes page aligned (default 1)\n\ -x: preallocate file space before starting, XFS only\n\ -y: synchronize changes to a file\n" @@ -2513,18 +2458,10 @@ usage(void) #ifdef HAVE_LINUX_FALLOC_H " -F: Do not use fallocate (preallocation) calls\n" #endif -#ifdef FALLOC_FL_PUNCH_HOLE -" -H: Do not use punch hole calls\n" -#endif -#ifdef FALLOC_FL_ZERO_RANGE -" -z: Do not use zero range calls\n" -#endif -#ifdef FALLOC_FL_COLLAPSE_RANGE -" -C: Do not use collapse range calls\n" -#endif -#ifdef FALLOC_FL_INSERT_RANGE -" -I: Do not use insert range calls\n" -#endif +" -H: Do not use punch hole calls\n\ + -z: Do not use zero range calls\n\ + -C: Do not use collapse range calls\n\ + -I: Do not use insert range calls\n" #ifdef FICLONERANGE " -J: Do not use clone range calls\n" #endif
On Fri, Sep 27, 2024 at 08:25:37AM -0700, Darrick J. Wong wrote: > On Fri, Sep 27, 2024 at 08:07:49AM -0400, Brian Foster wrote: > > On Fri, Sep 27, 2024 at 01:42:31PM +0800, Zorro Lang wrote: > > > On Thu, Sep 26, 2024 at 11:55:21AM -0400, Brian Foster wrote: > > > > On Thu, Sep 26, 2024 at 07:50:28AM -0700, Darrick J. Wong wrote: > > > > > On Thu, Sep 26, 2024 at 10:41:47AM -0400, Brian Foster wrote: > > > > > > The various fallocate flags are mostly ifdef'd for backward > > > > > > compatibility with the exception of the associated test_fallocate() > > > > > > calls to verify functionality at runtime. I suspect the reason for > > > > > > this was to avoid ifdef ugliness around having to clear the runtime > > > > > > flag for each operation, but unfortunately this defeats the purpose > > > > > > of the ifdef protection everywhere else. > > > > > > > > > > > > Factor out the fallocate related test calls into a new helper and > > > > > > add the appropriate ifdefs. > > > > > > > > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > > > > --- > > > > > > ltp/fsx.c | 59 ++++++++++++++++++++++++++++++++++++++++++------------- > > > > > > 1 file changed, 45 insertions(+), 14 deletions(-) > > > > > > > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c > > > > > > index 677f8c9f..417743c5 100644 > > > > > > --- a/ltp/fsx.c > > > > > > +++ b/ltp/fsx.c > > > > > > @@ -2833,6 +2833,50 @@ __test_fallocate(int mode, const char *mode_str) > > > > > > #endif > > > > > > } > > > > > > > > > > > > +void > > > > > > +test_fallocate_calls(void) > > > > > > +{ > > > > > > + if (fallocate_calls) > > > > > > + fallocate_calls = test_fallocate(0); > > > > > > + if (keep_size_calls) > > > > > > + keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > > > > > > + > > > > > > +#ifdef FALLOC_FL_UNSHARE_RANGE > > > > > > + if (unshare_range_calls) > > > > > > + unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > > > > > > +#else > > > > > > + unshare_range_calls = 0; > > > > > > +#endif > > > > > > + > > > > > > +#ifdef FALLOC_FL_PUNCH_HOLE > > > > > > + if (punch_hole_calls) > > > > > > + punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > > > > > > +#else > > > > > > + punch_hole_calls = 0; > > > > > > +#endif > > > > > > + > > > > > > +#ifdef FALLOC_FL_ZERO_RANGE > > > > > > + if (zero_range_calls) > > > > > > + zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > > > > > > +#else > > > > > > + zero_range_calls = 0; > > > > > > +#endif > > > > > > + > > > > > > +#ifdef FALLOC_FL_COLLAPSE_RANGE > > > > > > + if (collapse_range_calls) > > > > > > + collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > > > > > > +#else > > > > > > + collapse_range_calls = 0; > > > > > > +#endif > > > > > > > > > > The concept looks fine, but collapse and zero range have been in the > > > > > kernel for a decade now, do we really need to have ifdef tests for them? > > > > > > > > > > > > > Probably not.. but why even bother worrying about individual flags? The > > > > insert and unshare flags have been around for 9 and 8 years > > > > respectively, none of these were fully ifdef'd from the beginning, and > > > > I'm not aware of anyone that has actually complained. > > > > > > > > I'm not convinced that this patch matters for anybody in practice. I > > > > included it just because it was simple enough to include the minimum > > > > mechanical fix and I was slightly curious if somebody could come up with > > > > a more elegant solution. In the spirit of being practical, maybe the > > > > better approach here is to just remove the (at least the falloc flag > > > > related) ifdefs entirely? We can always add them back if somebody > > > > complains... > > > > > > As this patch is still controversial, I'll merge the other one at first, to > > > catch up the release of this week. We can talk this one later, if you still > > > hope to have it :) > > > > > > > Thanks. In thinking more about it.. my reasoning above was that it seems > > like the value of these ifdefs is to avoid disruption when new > > functionality is introduced, but at the same time the fstests user base > > may not be necessarily all that interested in eternal backwards > > compatibility for ancient runtimes, etc. Therefore, I wonder if it's > > reasonable to have an (informal) expiration date for when we can clear > > out some of this cruft to keep the code cleaner and more maintainable > > going forward. > > > > So I largely agree with Darrick's point, it's just that personally I'm > > less interested in discussion over which fallocate flags to include or > > not because to my mind that suggests we might as well just drop the > > ifdefs entirely. That said, I'm not all that invested beyond just trying > > to be proactive since I happened to be hacking in this area, so if you > > guys want to leave things as is, or agree on a subset of flags to ifdef, > > just let me know and I'll drop it or send a v2. > > Usually I just let Christoph complain and remove the ifdefs, but if I > have to use my own rule, it would be that ifdefs and ./configure > trickery isn't necessary for any symbol that is at least 5 years old. > > Recent complaints on the mailing list have caused me to revise that to > 10 years old though (see recent memfd_create fixes). :) I prefer "10 years", due to most of RHELs support 10 years (or a bit longer:) I think some other Enterprice systems are similar. Thanks, Zorro > > I also remember that a lot of the old crufty ifdef stuff (iirc) was kept > around so that fstests would continue to run on old RHELs. Once in a > while our QA folks rebase fstests to latest, but they also tend to patch > back in whatever ./configure magic they need for 2.6 era kernels. > > --D > > > Brian > > > > > > > Thanks, > > > Zorro > > > > > > > > > > > Brian > > > > > > > > > --D > > > > > > > > > > > + > > > > > > +#ifdef FALLOC_FL_INSERT_RANGE > > > > > > + if (insert_range_calls) > > > > > > + insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > > > > > > +#else > > > > > > + insert_range_calls = 0; > > > > > > +#endif > > > > > > +} > > > > > > + > > > > > > bool > > > > > > keep_running(void) > > > > > > { > > > > > > @@ -3271,20 +3315,7 @@ main(int argc, char **argv) > > > > > > check_trunc_hack(); > > > > > > } > > > > > > > > > > > > - if (fallocate_calls) > > > > > > - fallocate_calls = test_fallocate(0); > > > > > > - if (keep_size_calls) > > > > > > - keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); > > > > > > - if (unshare_range_calls) > > > > > > - unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); > > > > > > - if (punch_hole_calls) > > > > > > - punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > > > > > > - if (zero_range_calls) > > > > > > - zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); > > > > > > - if (collapse_range_calls) > > > > > > - collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); > > > > > > - if (insert_range_calls) > > > > > > - insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); > > > > > > + test_fallocate_calls(); > > > > > > if (clone_range_calls) > > > > > > clone_range_calls = test_clone_range(); > > > > > > if (dedupe_range_calls) > > > > > > -- > > > > > > 2.46.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
diff --git a/ltp/fsx.c b/ltp/fsx.c index 677f8c9f..417743c5 100644 --- a/ltp/fsx.c +++ b/ltp/fsx.c @@ -2833,6 +2833,50 @@ __test_fallocate(int mode, const char *mode_str) #endif } +void +test_fallocate_calls(void) +{ + if (fallocate_calls) + fallocate_calls = test_fallocate(0); + if (keep_size_calls) + keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); + +#ifdef FALLOC_FL_UNSHARE_RANGE + if (unshare_range_calls) + unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); +#else + unshare_range_calls = 0; +#endif + +#ifdef FALLOC_FL_PUNCH_HOLE + if (punch_hole_calls) + punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); +#else + punch_hole_calls = 0; +#endif + +#ifdef FALLOC_FL_ZERO_RANGE + if (zero_range_calls) + zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); +#else + zero_range_calls = 0; +#endif + +#ifdef FALLOC_FL_COLLAPSE_RANGE + if (collapse_range_calls) + collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); +#else + collapse_range_calls = 0; +#endif + +#ifdef FALLOC_FL_INSERT_RANGE + if (insert_range_calls) + insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); +#else + insert_range_calls = 0; +#endif +} + bool keep_running(void) { @@ -3271,20 +3315,7 @@ main(int argc, char **argv) check_trunc_hack(); } - if (fallocate_calls) - fallocate_calls = test_fallocate(0); - if (keep_size_calls) - keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE); - if (unshare_range_calls) - unshare_range_calls = test_fallocate(FALLOC_FL_UNSHARE_RANGE); - if (punch_hole_calls) - punch_hole_calls = test_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); - if (zero_range_calls) - zero_range_calls = test_fallocate(FALLOC_FL_ZERO_RANGE); - if (collapse_range_calls) - collapse_range_calls = test_fallocate(FALLOC_FL_COLLAPSE_RANGE); - if (insert_range_calls) - insert_range_calls = test_fallocate(FALLOC_FL_INSERT_RANGE); + test_fallocate_calls(); if (clone_range_calls) clone_range_calls = test_clone_range(); if (dedupe_range_calls)
The various fallocate flags are mostly ifdef'd for backward compatibility with the exception of the associated test_fallocate() calls to verify functionality at runtime. I suspect the reason for this was to avoid ifdef ugliness around having to clear the runtime flag for each operation, but unfortunately this defeats the purpose of the ifdef protection everywhere else. Factor out the fallocate related test calls into a new helper and add the appropriate ifdefs. Signed-off-by: Brian Foster <bfoster@redhat.com> --- ltp/fsx.c | 59 ++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 14 deletions(-)