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