Re: [PATCH 2/2] fsx: add missing fallocate flag ifdefs

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

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