[PATCH] Add support to check for FALLOC_FL_COLLAPSE_RANGE and FALLOC_FL_ZERO_RANGE crap modes

[PATCH] Add support to check for FALLOC_FL_COLLAPSE_RANGE and FALLOC_FL_ZERO_RANGE crap modes

[Nicholas Krause]

This adds checks for the stated modes as if they are crap we will return error
not supported.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 fs/btrfs/file.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 1f2b99c..599495a 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -2490,7 +2490,8 @@ static long btrfs_fallocate(struct file *file, int mode,
 	alloc_end = round_up(offset + len, blocksize);
 
 	/* Make sure we aren't being give some crap mode */
-	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
+	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE|
+	FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE))
 		return -EOPNOTSUPP;
 
 	if (mode & FALLOC_FL_PUNCH_HOLE)
-- 
1.7.10.4

Re: [PATCH] Add support to check for FALLOC_FL_COLLAPSE_RANGE and FALLOC_FL_ZERO_RANGE crap modes

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 2250 bytes --]

On Thu, Jul 31, 2014 at 01:53:33PM -0400, Nicholas Krause wrote:
> This adds checks for the stated modes as if they are crap we will return error
> not supported.

   You've just enabled two options, but you haven't actually
implemented the code behind it. I would tell you *NOT* to do anything
else on this work until you can answer the question: What happens if
you apply this patch, create a large file called "foo.txt", and then a
userspace program executes the following code?

int fd = open("foo.txt", O_RDWR);
fallocate(fd, FALLOCATE_FL_COLLAPSE_RANGE, 50, 50);

   Try it on a btrfs filesystem, both with and without your patch.
Also try it on an ext4 filesystem.

   Once you've done all of that, reply to this mail and tell me what
the problem is with this patch. You need to make two answers: what are
the technical problems with the patch? What errors have you made in
the development process?

   *Only* if you can answer those questions sensibly, should you write
any more patches, of any kind.

   Hugo.

> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  fs/btrfs/file.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 1f2b99c..599495a 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -2490,7 +2490,8 @@ static long btrfs_fallocate(struct file *file, int mode,
>  	alloc_end = round_up(offset + len, blocksize);
>  
>  	/* Make sure we aren't being give some crap mode */
> -	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
> +	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE|
> +	FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE))
>  		return -EOPNOTSUPP;
>  
>  	if (mode & FALLOC_FL_PUNCH_HOLE)
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 65E74AC0 from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
  --- The glass is neither half-full nor half-empty; it is twice as ---  
                        large as it needs to be.                         

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] Add support to check for FALLOC_FL_COLLAPSE_RANGE and FALLOC_FL_ZERO_RANGE crap modes

[Nick Krause]

On Thu, Jul 31, 2014 at 3:09 PM, Hugo Mills <hugo@carfax.org.uk> wrote:
> On Thu, Jul 31, 2014 at 01:53:33PM -0400, Nicholas Krause wrote:
>> This adds checks for the stated modes as if they are crap we will return error
>> not supported.
>
>    You've just enabled two options, but you haven't actually
> implemented the code behind it. I would tell you *NOT* to do anything
> else on this work until you can answer the question: What happens if
> you apply this patch, create a large file called "foo.txt", and then a
> userspace program executes the following code?
>
> int fd = open("foo.txt", O_RDWR);
> fallocate(fd, FALLOCATE_FL_COLLAPSE_RANGE, 50, 50);
>
>    Try it on a btrfs filesystem, both with and without your patch.
> Also try it on an ext4 filesystem.
>
>    Once you've done all of that, reply to this mail and tell me what
> the problem is with this patch. You need to make two answers: what are
> the technical problems with the patch? What errors have you made in
> the development process?
>
>    *Only* if you can answer those questions sensibly, should you write
> any more patches, of any kind.
>
>    Hugo.
>
>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>> ---
>>  fs/btrfs/file.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>> index 1f2b99c..599495a 100644
>> --- a/fs/btrfs/file.c
>> +++ b/fs/btrfs/file.c
>> @@ -2490,7 +2490,8 @@ static long btrfs_fallocate(struct file *file, int mode,
>>       alloc_end = round_up(offset + len, blocksize);
>>
>>       /* Make sure we aren't being give some crap mode */
>> -     if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
>> +     if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE|
>> +     FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE))
>>               return -EOPNOTSUPP;
>>
>>       if (mode & FALLOC_FL_PUNCH_HOLE)
>> --
>> 1.7.10.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> === Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
>   PGP key: 65E74AC0 from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
>   --- The glass is neither half-full nor half-empty; it is twice as ---
>                         large as it needs to be.
Calls are there in btrfs , therefore will either kernel panic or cause an oops.
Need to test this patch as this is very easy to catch bug.
Nick

Re: [PATCH] Add support to check for FALLOC_FL_COLLAPSE_RANGE and FALLOC_FL_ZERO_RANGE crap modes

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 2316 bytes --]

On Thu, Jul 31, 2014 at 09:53:15PM -0400, Nick Krause wrote:
> On Thu, Jul 31, 2014 at 3:09 PM, Hugo Mills <hugo@carfax.org.uk> wrote:
> > On Thu, Jul 31, 2014 at 01:53:33PM -0400, Nicholas Krause wrote:
> >> This adds checks for the stated modes as if they are crap we will return error
> >> not supported.
> >
> >    You've just enabled two options, but you haven't actually
> > implemented the code behind it. I would tell you *NOT* to do anything
> > else on this work until you can answer the question: What happens if
> > you apply this patch, create a large file called "foo.txt", and then a
> > userspace program executes the following code?
> >
> > int fd = open("foo.txt", O_RDWR);
> > fallocate(fd, FALLOCATE_FL_COLLAPSE_RANGE, 50, 50);
> >
> >    Try it on a btrfs filesystem, both with and without your patch.
> > Also try it on an ext4 filesystem.
> >
> >    Once you've done all of that, reply to this mail and tell me what
> > the problem is with this patch. You need to make two answers: what are
> > the technical problems with the patch? What errors have you made in
> > the development process?
> >
> >    *Only* if you can answer those questions sensibly, should you write
> > any more patches, of any kind.
[snip]

> Calls are there in btrfs , therefore will either kernel panic or
> cause an oops.

   That's a guess. I can tell it's a guess, because I've actually read
(some of) the rest of that function, so I've got a good idea of what I
think it will do -- and panic or oops is not the answer. Try again.
You can answer this question two ways: by test (see my suggestion
above), or by reading and understanding the code. Either will work in
this case, but doing neither is not an option for someone who wants to
change the function.

> Need to test this patch as this is very easy to catch bug.

   So why didn't you? It's your patch, testing it is your job --
*before* it gets out into the outside world.

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 65E74AC0 from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
  --- But people have always eaten people,  / what else is there to ---  
         eat?  / If the Juju had meant us not to eat people / he         
                     wouldn't have made us of meat.                      

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] Add support to check for FALLOC_FL_COLLAPSE_RANGE and FALLOC_FL_ZERO_RANGE crap modes

[Theodore Ts'o]

On Thu, Jul 31, 2014 at 08:09:10PM +0100, Hugo Mills wrote:
> On Thu, Jul 31, 2014 at 01:53:33PM -0400, Nicholas Krause wrote:
> > This adds checks for the stated modes as if they are crap we will return error
> > not supported.
> 
>    You've just enabled two options, but you haven't actually
> implemented the code behind it. I would tell you *NOT* to do anything
> else on this work until you can answer the question: What happens if
> you apply this patch, create a large file called "foo.txt", and then a
> userspace program executes the following code?
> 
> int fd = open("foo.txt", O_RDWR);
> fallocate(fd, FALLOCATE_FL_COLLAPSE_RANGE, 50, 50);
> 
>    Try it on a btrfs filesystem, both with and without your patch.
> Also try it on an ext4 filesystem.
> 
>    Once you've done all of that, reply to this mail and tell me what
> the problem is with this patch. You need to make two answers: what are
> the technical problems with the patch? What errors have you made in
> the development process?

There are also the conceptual failures.  Before you do anything else,
you need to be able to answer the question, "what do you think the
flags FALLOC_FL_COLLAPSE_RANGE and FALLOC_FL_ZERO_RANGE are supposed
to do?"  What are the possible appropriate things for btrfs to do if
it sees these flags?  (Hint: there is more than one correct answer,
and its current choice is one of them.  What is the other one?)

Nick, the fact that you call these modes "crap" is a hint that you
have a fundamental lack of understanding --- and before you waste more
of kernel developers' time, you need to get that understanding first,
for any bit of code that you propose to "improve".

This is why I suggested that you work on userspace testing scripts
first.  It's pretty clear you are (a) incredibly sloppy, and (b)
lacking conceptual understanding of a lot of technical details, and
(c) even worse, aren't letting this lack of understanding stop you
from posting patches.  As a result you are adding negative value to
whatever project or subsystem you try to attach yourself to --- you're
not helping.

						- Ted

P.S.   As a further hint, change the above code to read:

	int fd = open("foo.txt", O_RDWR);
	if (fallocate(fd, FALLOCATE_FL_COLLAPSE_RANGE, 4096, 8192) < 0)
		perror("fallocate");

And then run "filefrag -vs foo.txt" before and after running the above
code fragment and then try something like this:

     cp /usr/share/dict/words foo.txt
     filefrag -vs foo.txt
     ls -l foo.txt
     /tmp/fallocate-test-prog
     filefrag -vs foo.txt
     ls -l foo.txt
     diff /usr/share/dict/words foo.txt

Try doing this on an ext4 or xfs system and a btrfs file system.

Re: [PATCH] Add support to check for FALLOC_FL_COLLAPSE_RANGE and FALLOC_FL_ZERO_RANGE crap modes

[Nick Krause]

On Fri, Aug 1, 2014 at 8:21 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Thu, Jul 31, 2014 at 08:09:10PM +0100, Hugo Mills wrote:
>> On Thu, Jul 31, 2014 at 01:53:33PM -0400, Nicholas Krause wrote:
>> > This adds checks for the stated modes as if they are crap we will return error
>> > not supported.
>>
>>    You've just enabled two options, but you haven't actually
>> implemented the code behind it. I would tell you *NOT* to do anything
>> else on this work until you can answer the question: What happens if
>> you apply this patch, create a large file called "foo.txt", and then a
>> userspace program executes the following code?
>>
>> int fd = open("foo.txt", O_RDWR);
>> fallocate(fd, FALLOCATE_FL_COLLAPSE_RANGE, 50, 50);
>>
>>    Try it on a btrfs filesystem, both with and without your patch.
>> Also try it on an ext4 filesystem.
>>
>>    Once you've done all of that, reply to this mail and tell me what
>> the problem is with this patch. You need to make two answers: what are
>> the technical problems with the patch? What errors have you made in
>> the development process?
>
> There are also the conceptual failures.  Before you do anything else,
> you need to be able to answer the question, "what do you think the
> flags FALLOC_FL_COLLAPSE_RANGE and FALLOC_FL_ZERO_RANGE are supposed
> to do?"  What are the possible appropriate things for btrfs to do if
> it sees these flags?  (Hint: there is more than one correct answer,
> and its current choice is one of them.  What is the other one?)
>
> Nick, the fact that you call these modes "crap" is a hint that you
> have a fundamental lack of understanding --- and before you waste more
> of kernel developers' time, you need to get that understanding first,
> for any bit of code that you propose to "improve".
>
> This is why I suggested that you work on userspace testing scripts
> first.  It's pretty clear you are (a) incredibly sloppy, and (b)
> lacking conceptual understanding of a lot of technical details, and
> (c) even worse, aren't letting this lack of understanding stop you
> from posting patches.  As a result you are adding negative value to
> whatever project or subsystem you try to attach yourself to --- you're
> not helping.
>
>                                                 - Ted
>
> P.S.   As a further hint, change the above code to read:
>
>         int fd = open("foo.txt", O_RDWR);
>         if (fallocate(fd, FALLOCATE_FL_COLLAPSE_RANGE, 4096, 8192) < 0)
>                 perror("fallocate");
>
> And then run "filefrag -vs foo.txt" before and after running the above
> code fragment and then try something like this:
>
>      cp /usr/share/dict/words foo.txt
>      filefrag -vs foo.txt
>      ls -l foo.txt
>      /tmp/fallocate-test-prog
>      filefrag -vs foo.txt
>      ls -l foo.txt
>      diff /usr/share/dict/words foo.txt
>
> Try doing this on an ext4 or xfs system and a btrfs file system.

I miss send this patch, that's my there are issues.
Cheers Nick

Re: [PATCH] Add support to check for FALLOC_FL_COLLAPSE_RANGE and FALLOC_FL_ZERO_RANGE crap modes

[Duncan]

Nicholas Krause posted on Thu, 31 Jul 2014 13:53:33 -0400 as excerpted:

> This adds checks for the stated modes as if they are crap we will return
> error not supported.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  fs/btrfs/file.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index 1f2b99c..599495a
> 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -2490,7 +2490,8 @@
> static long btrfs_fallocate(struct file *file, int mode,
>  	alloc_end = round_up(offset + len, blocksize);
>  
>  	/* Make sure we aren't being give some crap mode */
> -	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
> +	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE| +
> FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE))
>  		return -EOPNOTSUPP;
>  
>  	if (mode & FALLOC_FL_PUNCH_HOLE)

Is the supporting code already there?

You're removing the EOPNOTSUPP errors, but the code doesn't add the 
support, just removes the errors in the check for it, yet your comment 
doesn't point out that the support is actually already there with a 
pointer to either the commit adding it or the functions supporting it, as 
it should if that's true and the implementing patch simply forgot to 
remove those checks.

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman