Re: [PATCH][PROGS] xfs_io: Allow setting multiple mode flags for fallocate()

[Calvin Owens <calvinowens@fb.com>]

On Wednesday 03/22 at 08:54 +1100, Dave Chinner wrote:
> On Sun, Mar 19, 2017 at 09:33:50PM -0700, Calvin Owens wrote:
> > On 03/17/2017 11:55 PM, Dave Chinner wrote:
> > >On Fri, Mar 17, 2017 at 06:26:09PM -0700, Calvin Owens wrote:
> > >>This allows testing FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE.
> > >
> > >Which is normally done through the "fpunch" command, which explains
> > >why nobody has noticed this.
> > 
> > Ah okay, I was assuming parity with syscalls in the commands and picked
> > the first one I saw ;)
> > 
> > >>Signed-off-by: Calvin Owens <calvinowens@fb.com>
> > >>---
> > >> io/prealloc.c | 10 +++++-----
> > >> 1 file changed, 5 insertions(+), 5 deletions(-)
> > >>
> > >>diff --git a/io/prealloc.c b/io/prealloc.c
> > >>index a9d66cc..2a4bcdc 100644
> > >>--- a/io/prealloc.c
> > >>+++ b/io/prealloc.c
> > >>@@ -201,19 +201,19 @@ fallocate_f(
> > >> 	while ((c = getopt(argc, argv, "cikpu")) != EOF) {
> > >> 		switch (c) {
> > >> 		case 'c':
> > >>-			mode = FALLOC_FL_COLLAPSE_RANGE;
> > >>+			mode |= FALLOC_FL_COLLAPSE_RANGE;
> > >> 			break;
> > >> 		case 'i':
> > >>-			mode = FALLOC_FL_INSERT_RANGE;
> > >>+			mode |= FALLOC_FL_INSERT_RANGE;
> > >> 			break;
> > >> 		case 'k':
> > >>-			mode = FALLOC_FL_KEEP_SIZE;
> > >>+			mode |= FALLOC_FL_KEEP_SIZE;
> > >> 			break;
> > >> 		case 'p':
> > >>-			mode = FALLOC_FL_PUNCH_HOLE;
> > >>+			mode |= FALLOC_FL_PUNCH_HOLE;
> > >> 			break;
> > >> 		case 'u':
> > >>-			mode = FALLOC_FL_UNSHARE_RANGE;
> > >>+			mode |= FALLOC_FL_UNSHARE_RANGE;
> > >> 			break;
> > >
> > >NACK. We should not allow users to set invalid combinations
> > >of commands such as 'falloc -cipu ...' - this would set the flags
> > >(FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_INSERT_RANGE |
> > >FALLOC_FL_PUNCH_HOLE | FALLOC_FL_UNSHARE_RANGE) and will always
> > >error out.
> > 
> > Isn't it potentially useful to test invalid behavior?
> 
> Yes, but exhaustive testing of syscall option validity is not what
> xfs_io is for - xfs_io is for giving test scripts access to the syscall
> functionality, and so if someone passes an invalid combination of
> options it should fail.
> 
> > >The use of FALLOC_FL_KEEP_SIZE for the FALLOC_FL_PUNCH_HOLE command
> > >is essentially for documentation purposes - it does not do
> > >truncation, hence the FALLOC_FL_KEEP_SIZE flag was added to it to
> > >ensure developers understand that it does not truncate the file and
> > >change EOF.
> > >
> > >IOWs, the fix needed to make the 'falloc -p' command work is really
> > >just this:
> > >
> > >-			mode = FALLOC_FL_PUNCH_HOLE;
> > >+			mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
> > 
> > My thinking was just "allow full exercising of the syscall", fpunch works
> > for my testcase so if you'd prefer not to do that we can just drop this :)
> 
> If you want to go test invalid combinations, write a syscall test
> for ltp.
> 
> > It might be worth cleaning up 'falloc' though: '-k' doesn't really make
> > sense on its own right?
> 
> As I said in my last response: "falloc -k" is a valid usage for
> preallocation because it means "allow preallocation beyond EOF".
> i.e. future writes that extend the file will not need to allocate
> blocks....

The point I'm trying to make is that it's fundamentally confusing to have
switches like "-k" and "-p" for ORable flags in a syscall argument, when
the switches don't themselves support being combined. ZERO_RANGE for
example is ORable with KEEP_SIZE or valid on its own; if "-z" were added
we couldn't blame the user for thinking that "-z -k" is a valid thing to
do, could we?

But this is pedantic and obviously relatively unimportant, patch below.

Thanks,
Calvin

---8<---
From: Calvin Owens <calvinowens@fb.com>
Date: Thu, 30 Mar 2017 20:50:55 -0700
Subject: [PATCH] io/prealloc: Fix "falloc -p" to pass KEEP_SIZE

Otherwise, the syscall just returns -EOPPNOTSUPP.

Signed-off-by: Calvin Owens <calvinowens@fb.com>
---
 io/prealloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io/prealloc.c b/io/prealloc.c
index a9d66cc..1a1c9ca 100644
--- a/io/prealloc.c
+++ b/io/prealloc.c
@@ -210,7 +210,7 @@ fallocate_f(
 			mode = FALLOC_FL_KEEP_SIZE;
 			break;
 		case 'p':
-			mode = FALLOC_FL_PUNCH_HOLE;
+			mode = FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE;
 			break;
 		case 'u':
 			mode = FALLOC_FL_UNSHARE_RANGE;
-- 
2.9.3