| Message ID | 1406829213-4759-1-git-send-email-xerofoify@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 -- 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
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.
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. -- 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
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 -- 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
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)
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(-)