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

[Brian Foster <bfoster@redhat.com>]

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