* [PATCH][PROGS] xfs_io: Allow setting multiple mode flags for fallocate()
@ 2017-03-18 1:26 Calvin Owens
2017-03-18 6:55 ` Dave Chinner
0 siblings, 1 reply; 6+ messages in thread
From: Calvin Owens @ 2017-03-18 1:26 UTC (permalink / raw)
To: linux-xfs; +Cc: kernel-team, calvinowens
This allows testing FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE.
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;
default:
command_usage(&falloc_cmd);
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH][PROGS] xfs_io: Allow setting multiple mode flags for fallocate()
2017-03-18 1:26 [PATCH][PROGS] xfs_io: Allow setting multiple mode flags for fallocate() Calvin Owens
@ 2017-03-18 6:55 ` Dave Chinner
2017-03-20 4:33 ` Calvin Owens
0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2017-03-18 6:55 UTC (permalink / raw)
To: Calvin Owens; +Cc: linux-xfs, kernel-team
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.
> 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.
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;
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][PROGS] xfs_io: Allow setting multiple mode flags for fallocate()
2017-03-18 6:55 ` Dave Chinner
@ 2017-03-20 4:33 ` Calvin Owens
2017-03-21 21:54 ` Dave Chinner
0 siblings, 1 reply; 6+ messages in thread
From: Calvin Owens @ 2017-03-20 4:33 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, kernel-team, calvinowens
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?
> 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 :)
It might be worth cleaning up 'falloc' though: '-k' doesn't really make
sense on its own right? Maybe remove "-k" and make "-p" OR in KEEP_SIZE
like you suggest?
Thanks,
Calvin
> Cheers,
>
> Dave.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][PROGS] xfs_io: Allow setting multiple mode flags for fallocate()
2017-03-20 4:33 ` Calvin Owens
@ 2017-03-21 21:54 ` Dave Chinner
2017-03-31 4:14 ` Calvin Owens
0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2017-03-21 21:54 UTC (permalink / raw)
To: Calvin Owens; +Cc: linux-xfs, kernel-team
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....
> Maybe remove "-k" and make "-p" OR in KEEP_SIZE
> like you suggest?
See above, and the answer is obvious.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][PROGS] xfs_io: Allow setting multiple mode flags for fallocate()
2017-03-21 21:54 ` Dave Chinner
@ 2017-03-31 4:14 ` Calvin Owens
2017-04-04 19:26 ` Eric Sandeen
0 siblings, 1 reply; 6+ messages in thread
From: Calvin Owens @ 2017-03-31 4:14 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, kernel-team
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH][PROGS] xfs_io: Allow setting multiple mode flags for fallocate()
2017-03-31 4:14 ` Calvin Owens
@ 2017-04-04 19:26 ` Eric Sandeen
0 siblings, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2017-04-04 19:26 UTC (permalink / raw)
To: Calvin Owens, Dave Chinner; +Cc: linux-xfs, kernel-team
On 3/30/17 11:14 PM, Calvin Owens wrote:
> ---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>
Reviewed-by: Eric Sandeen <sandeen@redhat.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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-04 19:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-18 1:26 [PATCH][PROGS] xfs_io: Allow setting multiple mode flags for fallocate() Calvin Owens
2017-03-18 6:55 ` Dave Chinner
2017-03-20 4:33 ` Calvin Owens
2017-03-21 21:54 ` Dave Chinner
2017-03-31 4:14 ` Calvin Owens
2017-04-04 19:26 ` Eric Sandeen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.