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

[Calvin Owens <calvinowens@fb.com>]

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