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