Re: [PATCH 26/28] stale-handle.c: use syncfs() rather than sync()

[Dave Chinner <david@fromorbit.com>]

On Wed, Apr 30, 2025 at 02:04:08PM +0530, Nirjhar Roy (IBM) wrote:
> On Thu, 2025-04-17 at 13:01 +1000, Dave Chinner wrote:
> > xfs/238 runs stale_handle to create 1000 inodes, grab their file
> > handles, unlink them and then try to open them from the stored file
> > handles.  It takes a ridiculously long time to run under
> > check-parallel because it runs sync() multiple times:
> > 
> > xfs/238        144s
> > 
> > When running check-parallel, sync() can take a -long- time to
> > run as there can be dozens of filesystems that need to be synced,
> > not to mention sync getting hung up behind all the mount and
> > unmounts that are also being run.
> > 
> > Convert the sync() calls to syncfs() so that they only try to sync
> > the filesystem under test and not the entire system. This avoids
> > interactions and delays with other tests and mount/unmount
> > operations, hence allowing both the test and the overall
> > check-parallel operation to run faster:
> Yes, filesystem wise sync is makes more sense. I remember similar
> changes with your previous check-parralel patch series as well.
> > 
> > xfs/238        5s
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  src/stale_handle.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/stale_handle.c b/src/stale_handle.c
> > index 2acd4968c..fd8b3ec61 100644
> > --- a/src/stale_handle.c
> > +++ b/src/stale_handle.c
> > @@ -21,7 +21,7 @@
> >  int main(int argc, char **argv)
> >  {
> >  	int	i;
> > -	int	fd;
> > +	int	fd, test_dir_fd;
> >  	int	ret;
> >  	int	failed = 0;
> >  	char	fname[MAXPATHLEN];
> Not related to this change: 
> A lot of local variables are being used in this test:
> char	fname[MAXPATHLEN];
> void	*handle[NUMFILES];
> size_t	hlen[NUMFILES];
> char fshandle[256];
> Is this fine? Shouldn't we limit the usage of so much local stack?

It's userspace and it's not a big/complex program. Using stack like
this instead of allocating memory is simple, fast, and easy to
maintain. There's no reason to complicate the code unnecessarily.

> > @@ -44,6 +44,12 @@ int main(int argc, char **argv)
> >  		return EXIT_FAILURE;
> >  	}
> >  
> > +	test_dir_fd = open(test_dir, O_RDONLY|O_DIRECTORY);
> > +	if (test_dir_fd < 0) {
> > +		perror(test_dir);
> > +		return EXIT_FAILURE;
> > +	}
> > +
> Now that we are already opening the test_dir to get an fd of the
> filessytem being tested, should we remove the stat call(line 42) just
> before this open call and instead use this open call to check the
> presence/absence of test_dir?

We can, it doesn't matter either way...

> Other than this, the rest of the changes look good to me (some minor
> comments below but not related to this change).
> 
> Feel free to add 
> Reviewed-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>

Thanks!

-Dave.
-- 
Dave Chinner
david@fromorbit.com