Re: [PATCH RFC] fsx: support unshare range fallocate mode

[Brian Foster <bfoster@redhat.com>]

On Fri, Sep 20, 2024 at 10:58:39PM +0800, Zorro Lang wrote:
> On Fri, Sep 06, 2024 at 02:56:06PM -0400, Brian Foster wrote:
> > The fallocate unshare mode flag modifies traditional preallocate
> > mode to unshare any shared extents backing the target range. Without
> > the unshare flag, preallocate mode simply assures that blocks are
> > physically allocated, regardless of whether they might be shared.
> > Unshare mode behaves the same as preallocate mode outside of the
> > shared extent case.
> > 
> > Since unshare is fundamentally a modifier to preallocate mode,
> > enable it via an operation flag. Similar to keep size mode, select
> > it randomly for fallocate operations and track it via a flag and
> > string combination for operation logging and replay.
> > 
> > Unshare is mainly used for filesystems that support reflink, but the
> > operation is equivalent to preallocate mode for non-shared ranges,
> > so enable it by default. Filesystems that do not support the
> > fallocate flag (such as those that might not support reflink) will
> > fail the test operation and disable unshare calls at runtime. Also
> > provide a new command line option to explicitly disable unshare
> > calls.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> > 
> > Hi all,
> > 
> > A couple quick notes..
> > 
> > First, this applies on top of the currently pending EOF pollution
> > patches [1] to fsx.
> 
> The patchset [1] has been merged, thanks for doing that.
> 

Great, thanks.

> > 
> > Second, this triggers multiple new test failures in my initial tests
> > that I've not fully diagnosed. I'm not necessarily sure that we want to
> > hold off on added test coverage for that reason, but I'd like to at
> > least be sure this isn't due to this patch doing something blatantly
> > wrong.
> > 
> > That said, this has also uncovered one or two unshare related issues, so
> > I'm posting this slightly prematurely in order to share it and hopefully
> > get some review..
> > 
> > Brian
> > 
> > [1] https://lore.kernel.org/fstests/20240828181534.41054-1-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 1ba1bf65..003990db 100644
> > --- a/ltp/fsx.c
...
> > @@ -1879,15 +1887,25 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
> >  #ifdef HAVE_LINUX_FALLOC_H
> >  /* fallocate is basically a no-op unless extending, then a lot like a truncate */
> >  void
> > -do_preallocate(unsigned offset, unsigned length, int keep_size)
> > +do_preallocate(unsigned offset, unsigned length, int keep_size, int unshare)
> >  {
> >  	unsigned end_offset;
> > +	enum opflags opflags = FL_NONE;
> > +	int mode = 0;
> > +
> > +	if (keep_size) {
> > +		opflags |= FL_KEEP_SIZE;
> > +		mode |= FALLOC_FL_KEEP_SIZE;
> > +	}
> > +	if (unshare) {
> > +		opflags |= FL_UNSHARE;
> > +		mode |= FALLOC_FL_UNSHARE_RANGE;
> 
> The FALLOC_FL_UNSHARE_RANGE is not a new flag, but it might be not old enough.
> The first commit about the FALLOC_FL_UNSHARE_RANGE is:
> 
>   commit 409332b65d3ed8cfa7a8030f1e9d52f372219642
>   Author: Lukas Czerner <lczerner@redhat.com>
>   Date:   Thu Mar 13 19:07:42 2014 +1100
> 
>       fs: Introduce FALLOC_FL_ZERO_RANGE flag for fallocate
> 
> So "#ifdef FALLOC_FL_UNSHARE_RANGE" maybe needed.
> 

Indeed. Not that it matters, but note the above refers to zero range.
Unshare range came a couple years later in 71be6b4942dd6 ("vfs: add a
FALLOC_FL_UNSHARE mode to fallocate to unshare a range of blocks").

Also when looking at this, I noticed most of the test_fallocate() calls
seem to break backward compatibility. I.e., if an FALLOC_FL_* flag is
not defined, compilation is going to fail anyways because none of those
usages are ifdef'd.

This seems a bit confused, but I think the reason for the lack of ifdefs
for the test calls is that most of these are enabled by default. IOW
with a simple ifdef around the test_fallocate() call we wouldn't
actually set unshare_range_calls = 0 in the case where the flag doesn't
exist.

It would be nice to have a macro that just tested the flag as well and
otherwise returned 0, but a quick test suggests that's not allowed
(though my macro-fu is weak). For the purpose of this patch, I'm just
going to follow precedent and use an ifdef everywhere except for the
test_fallocate() call. Unless somebody has a better idea, maybe I'll
include a patch 2 that factors out a test_fallocate_calls() helper that
does all the relevant ifdefery.

Brian

> Thanks,
> Zorro
> 
> 
> > +	}
> >  
> >          if (length == 0) {
> >                  if (!quiet && testcalls > simulatedopcount)
> >                          prt("skipping zero length fallocate\n");
> > -                log4(OP_FALLOCATE, offset, length, FL_SKIPPED |
> > -		     (keep_size ? FL_KEEP_SIZE : FL_NONE));
> > +                log4(OP_FALLOCATE, offset, length, FL_SKIPPED | opflags);
> >                  return;
> >          }
> >  
> > @@ -1905,8 +1923,7 @@ do_preallocate(unsigned offset, unsigned length, int keep_size)
> >  	 * 	1: extending prealloc
> >  	 * 	2: interior prealloc
> >  	 */
> > -	log4(OP_FALLOCATE, offset, length,
> > -	     keep_size ? FL_KEEP_SIZE : FL_NONE);
> > +	log4(OP_FALLOCATE, offset, length, opflags);
> >  
> >  	if (end_offset > file_size) {
> >  		memset(good_buf + file_size, '\0', end_offset - file_size);
> > @@ -1921,7 +1938,7 @@ do_preallocate(unsigned offset, unsigned length, int keep_size)
> >  		      end_offset <= monitorend)))
> >  		prt("%lld falloc\tfrom 0x%x to 0x%x (0x%x bytes)\n", testcalls,
> >  				offset, offset + length, length);
> > -	if (fallocate(fd, keep_size ? FALLOC_FL_KEEP_SIZE : 0, (loff_t)offset, (loff_t)length) == -1) {
> > +	if (fallocate(fd, mode, (loff_t)offset, (loff_t)length) == -1) {
> >  	        prt("fallocate: 0x%x to 0x%x\n", offset, offset + length);
> >  		prterr("do_preallocate: fallocate");
> >  		report_failure(161);
> > @@ -1929,7 +1946,7 @@ do_preallocate(unsigned offset, unsigned length, int keep_size)
> >  }
> >  #else
> >  void
> > -do_preallocate(unsigned offset, unsigned length, int keep_size)
> > +do_preallocate(unsigned offset, unsigned length, int keep_size, int unshare)
> >  {
> >  	return;
> >  }
> > @@ -2095,6 +2112,8 @@ read_op(struct log_entry *log_entry)
> >  				log_entry->flags |= FL_KEEP_SIZE;
> >  			else if (strcmp(str, "close_open") == 0)
> >  				log_entry->flags |= FL_CLOSE_OPEN;
> > +			else if (strcmp(str, "unshare") == 0)
> > +				log_entry->flags |= FL_UNSHARE;
> >  			else if (strcmp(str, "*") == 0)
> >  				;  /* overlap marker; ignore */
> >  			else
> > @@ -2161,6 +2180,7 @@ test(void)
> >  	unsigned long	rv;
> >  	unsigned long	op;
> >  	int		keep_size = 0;
> > +	int		unshare = 0;
> >  
> >  	if (simulatedopcount > 0 && testcalls == simulatedopcount)
> >  		writefileimage();
> > @@ -2190,6 +2210,7 @@ test(void)
> >  			offset2 = log_entry.args[2];
> >  			closeopen = !!(log_entry.flags & FL_CLOSE_OPEN);
> >  			keep_size = !!(log_entry.flags & FL_KEEP_SIZE);
> > +			unshare = !!(log_entry.flags & FL_UNSHARE);
> >  			goto have_op;
> >  		}
> >  		return 0;
> > @@ -2219,8 +2240,12 @@ test(void)
> >  			size = random() % maxfilelen;
> >  		break;
> >  	case OP_FALLOCATE:
> > -		if (fallocate_calls && size && keep_size_calls)
> > -			keep_size = random() % 2;
> > +		if (fallocate_calls && size) {
> > +			if (keep_size_calls)
> > +				keep_size = random() % 2;
> > +			if (unshare_calls)
> > +				unshare = random() % 2;
> > +		}
> >  		break;
> >  	case OP_ZERO_RANGE:
> >  		if (zero_range_calls && size && keep_size_calls)
> > @@ -2334,7 +2359,7 @@ have_op:
> >  
> >  	case OP_FALLOCATE:
> >  		TRIM_OFF_LEN(offset, size, maxfilelen);
> > -		do_preallocate(offset, size, keep_size);
> > +		do_preallocate(offset, size, keep_size, unshare);
> >  		break;
> >  
> >  	case OP_PUNCH_HOLE:
> > @@ -2469,6 +2494,7 @@ usage(void)
> >  	-r readbdy: 4096 would make reads page aligned (default 1)\n\
> >  	-s style: 1 gives smaller truncates (default 0)\n\
> >  	-t truncbdy: 4096 would make truncates page aligned (default 1)\n\
> > +	-u Do not use unshare range\n\
> >  	-w writebdy: 4096 would make writes page aligned (default 1)\n\
> >  	-x: preallocate file space before starting, XFS only\n\
> >  	-y: synchronize changes to a file\n"
> > @@ -2853,7 +2879,7 @@ main(int argc, char **argv)
> >  	setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> >  
> >  	while ((ch = getopt_long(argc, argv,
> > -				 "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:w:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > +				 "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> >  				 longopts, NULL)) != EOF)
> >  		switch (ch) {
> >  		case 'b':
> > @@ -2952,6 +2978,9 @@ main(int argc, char **argv)
> >  			if (truncbdy <= 0)
> >  				usage();
> >  			break;
> > +		case 'u':
> > +			unshare_calls = 0;
> > +			break;
> >  		case 'w':
> >  			writebdy = getnum(optarg, &endp);
> >  			if (writebdy <= 0)
> > @@ -3242,6 +3271,8 @@ main(int argc, char **argv)
> >  		fallocate_calls = test_fallocate(0);
> >  	if (keep_size_calls)
> >  		keep_size_calls = test_fallocate(FALLOC_FL_KEEP_SIZE);
> > +	if (unshare_calls)
> > +		unshare_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)
> > -- 
> > 2.45.0
> > 
> > 
>