[PATCH] Remove certain calls for releasing page cache

[PATCH] Remove certain calls for releasing page cache

[Nicholas Krause]

This patch removes the lines for releasing the page cache in certain
files as this may aid in perfomance with writes in the compression
rountines of btrfs. Please note that this patch has not been tested
on my own hardware due to no compression based btrfs volumes of my
own.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 fs/btrfs/compression.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 1daea0b..b55b0e1 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -1,4 +1,5 @@
 /*
+	
  * Copyright (C) 2008 Oracle.  All rights reserved.
  *
  * This program is free software; you can redistribute it and/or
@@ -246,7 +247,6 @@ static noinline void end_compressed_writeback(struct inode *inode, u64 start,
 		}
 		for (i = 0; i < ret; i++) {
 			end_page_writeback(pages[i]);
-			page_cache_release(pages[i]);
 		}
 		nr_pages -= ret;
 		index += ret;
@@ -293,21 +293,6 @@ static void end_compressed_bio_write(struct bio *bio, int err)
 
 	end_compressed_writeback(inode, cb->start, cb->len);
 	/* note, our inode could be gone now */
-
-	/*
-	 * release the compressed pages, these came from alloc_page and
-	 * are not attached to the inode at all
-	 */
-	index = 0;
-	for (index = 0; index < cb->nr_pages; index++) {
-		page = cb->compressed_pages[index];
-		page->mapping = NULL;
-		page_cache_release(page);
-	}
-
-	/* finally free the cb struct */
-	kfree(cb->compressed_pages);
-	kfree(cb);
 out:
 	bio_put(bio);
 }
-- 
1.9.1

Re: [PATCH] Remove certain calls for releasing page cache

[Josef Bacik]

On 07/30/2014 04:42 PM, Nicholas Krause wrote:
> This patch removes the lines for releasing the page cache in certain
> files as this may aid in perfomance with writes in the compression
> rountines of btrfs. Please note that this patch has not been tested
> on my own hardware due to no compression based btrfs volumes of my
> own.
>

http://37.media.tumblr.com/tumblr_m44v95k9Y11qjhgo6o1_250.gif

Josef

Re: [PATCH] Remove certain calls for releasing page cache

[Zach Brown]

On Wed, Jul 30, 2014 at 04:47:12PM -0400, Josef Bacik wrote:
> On 07/30/2014 04:42 PM, Nicholas Krause wrote:
> >This patch removes the lines for releasing the page cache in certain
> >files as this may aid in perfomance with writes in the compression
> >rountines of btrfs. Please note that this patch has not been tested
> >on my own hardware due to no compression based btrfs volumes of my
> >own.
> >
> 
> http://37.media.tumblr.com/tumblr_m44v95k9Y11qjhgo6o1_250.gif

https://www.youtube.com/watch?v=jVfkYZmXHAg

- z

Re: [PATCH] Remove certain calls for releasing page cache

[Nick Krause]

On Wed, Jul 30, 2014 at 4:51 PM, Zach Brown <zab@zabbo.net> wrote:
> On Wed, Jul 30, 2014 at 04:47:12PM -0400, Josef Bacik wrote:
>> On 07/30/2014 04:42 PM, Nicholas Krause wrote:
>> >This patch removes the lines for releasing the page cache in certain
>> >files as this may aid in perfomance with writes in the compression
>> >rountines of btrfs. Please note that this patch has not been tested
>> >on my own hardware due to no compression based btrfs volumes of my
>> >own.
>> >
>>
>> http://37.media.tumblr.com/tumblr_m44v95k9Y11qjhgo6o1_250.gif
>
> https://www.youtube.com/watch?v=jVfkYZmXHAg
>
> - z


Zach,
Is there a reason for freeing the page cache as I seem to be missing why?
Regards Nick

Re: [PATCH] Remove certain calls for releasing page cache

[Josef Bacik]

So I'm going to list all the things that are wrong with this in the off chance that you will actually improve and then go back to ignoring your emails.

1) You test your patches before you send them.  Xfstests is how you test.

2) You seem to think page_cache_release releases the page cache.  It doesn't, it releases a ref on the page, we take a ref while we are doing IO and then drop it when we are done.  It is not evicted from cache until the VM decides to do it at some point in the future, so all your patch does is make us leak page cache.

3) You don't free the compressed pages.  This leaks memory.  These pages are only used for writing out, there is absolutely no reason to keep them around after the IO is complete, we have the actual page with the real data still in cache.

4) You also made it so we don't free the the struct we use to track the compressed IO, which is just as useless as the compressed pages now, causing another memory leak.

Had you tested this patch it would have killed the box pretty quickly and you would have known all of this.  But you didn't, and wasted a lot of much smarter peoples time.  This is not OK, this gets you ignored.  Do not do it again.  Thanks,

Josef

Nick Krause <xerofoify@gmail.com> wrote:


On Wed, Jul 30, 2014 at 4:51 PM, Zach Brown <zab@zabbo.net> wrote:
> On Wed, Jul 30, 2014 at 04:47:12PM -0400, Josef Bacik wrote:
>> On 07/30/2014 04:42 PM, Nicholas Krause wrote:
>> >This patch removes the lines for releasing the page cache in certain
>> >files as this may aid in perfomance with writes in the compression
>> >rountines of btrfs. Please note that this patch has not been tested
>> >on my own hardware due to no compression based btrfs volumes of my
>> >own.
>> >
>>
>> https://urldefense.proofpoint.com/v1/url?u=http://37.media.tumblr.com/tumblr_m44v95k9Y11qjhgo6o1_250.gif&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=epKcfiwbd4AA3VbfORegZjsqgl8dEZjg%2B3XKikkWVBM%3D%0A&s=5dd9a127c17ca09d584d86694d71f37cd336eab3f17658f8c0dccbfe2a53e2d8
>
> https://urldefense.proofpoint.com/v1/url?u=https://www.youtube.com/watch?v%3DjVfkYZmXHAg&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=epKcfiwbd4AA3VbfORegZjsqgl8dEZjg%2B3XKikkWVBM%3D%0A&s=5402110ffa4beccf257af0fc266e8cab20264119fe9bdb2e4734c8ba3e49a211
>
> - z


Zach,
Is there a reason for freeing the page cache as I seem to be missing why?
Regards Nick

Re: [PATCH] Remove certain calls for releasing page cache

[Dave Airlie]

> This patch removes the lines for releasing the page cache in certain
> files as this may aid in perfomance with writes in the compression
> rountines of btrfs. Please note that this patch has not been tested
> on my own hardware due to no compression based btrfs volumes of my
> own.
>

For all that is sacred, STOP.

Go and do something else, you are wasting people's valuable time,

Don't send any patches you haven't tested ever. If you aren't capable
of setting up a VM to run compressed btrfs volumes in, what makes you
think you can patch the code.

This isn't how you learn to be a kernel developer by wasting other
kernel developers time, if you can't work out why releasing the page cache
is necessary then don't send the patch until you have spent the time
understanding what the page cache is.

I know you'll just ignore this, and keep on trucking just like you ignored
the other messages from Stephen before.

But if you want to work on the kernel, this isn't the way to do it, and
nobody will ever take a patch from you seriously if you continue in this
fashion.

Dave.

Re: [PATCH] Remove certain calls for releasing page cache

[Nick Krause]

On Wed, Jul 30, 2014 at 7:30 PM, Dave Airlie <airlied@gmail.com> wrote:
>> This patch removes the lines for releasing the page cache in certain
>> files as this may aid in perfomance with writes in the compression
>> rountines of btrfs. Please note that this patch has not been tested
>> on my own hardware due to no compression based btrfs volumes of my
>> own.
>>
>
> For all that is sacred, STOP.
>
> Go and do something else, you are wasting people's valuable time,
>
> Don't send any patches you haven't tested ever. If you aren't capable
> of setting up a VM to run compressed btrfs volumes in, what makes you
> think you can patch the code.
>
> This isn't how you learn to be a kernel developer by wasting other
> kernel developers time, if you can't work out why releasing the page cache
> is necessary then don't send the patch until you have spent the time
> understanding what the page cache is.
>
> I know you'll just ignore this, and keep on trucking just like you ignored
> the other messages from Stephen before.
>
> But if you want to work on the kernel, this isn't the way to do it, and
> nobody will ever take a patch from you seriously if you continue in this
> fashion.
>
> Dave.
Dave ,
Seems I need to have tested this code first.
Regards Nick

Re: [PATCH] Remove certain calls for releasing page cache

[Dave Airlie]

On 31 July 2014 12:05, Nick Krause <xerofoify@gmail.com> wrote:
> On Wed, Jul 30, 2014 at 7:30 PM, Dave Airlie <airlied@gmail.com> wrote:
>>> This patch removes the lines for releasing the page cache in certain
>>> files as this may aid in perfomance with writes in the compression
>>> rountines of btrfs. Please note that this patch has not been tested
>>> on my own hardware due to no compression based btrfs volumes of my
>>> own.
>>>
>>
>> For all that is sacred, STOP.
>>
>> Go and do something else, you are wasting people's valuable time,
>>
>> Don't send any patches you haven't tested ever. If you aren't capable
>> of setting up a VM to run compressed btrfs volumes in, what makes you
>> think you can patch the code.
>>
>> This isn't how you learn to be a kernel developer by wasting other
>> kernel developers time, if you can't work out why releasing the page cache
>> is necessary then don't send the patch until you have spent the time
>> understanding what the page cache is.
>>
>> I know you'll just ignore this, and keep on trucking just like you ignored
>> the other messages from Stephen before.
>>
>> But if you want to work on the kernel, this isn't the way to do it, and
>> nobody will ever take a patch from you seriously if you continue in this
>> fashion.
>>
>> Dave.
> Dave ,
> Seems I need to have tested this code first.
> Regards Nick


No you needed to do a lot more, these one line replies from you are
quite stupid,

You are quite deliberately missing the point of people trying to help you,

Dave.

Re: [PATCH] Remove certain calls for releasing page cache

[Nick Krause]

On Wed, Jul 30, 2014 at 11:57 PM, Dave Airlie <airlied@gmail.com> wrote:
> On 31 July 2014 12:05, Nick Krause <xerofoify@gmail.com> wrote:
>> On Wed, Jul 30, 2014 at 7:30 PM, Dave Airlie <airlied@gmail.com> wrote:
>>>> This patch removes the lines for releasing the page cache in certain
>>>> files as this may aid in perfomance with writes in the compression
>>>> rountines of btrfs. Please note that this patch has not been tested
>>>> on my own hardware due to no compression based btrfs volumes of my
>>>> own.
>>>>
>>>
>>> For all that is sacred, STOP.
>>>
>>> Go and do something else, you are wasting people's valuable time,
>>>
>>> Don't send any patches you haven't tested ever. If you aren't capable
>>> of setting up a VM to run compressed btrfs volumes in, what makes you
>>> think you can patch the code.
>>>
>>> This isn't how you learn to be a kernel developer by wasting other
>>> kernel developers time, if you can't work out why releasing the page cache
>>> is necessary then don't send the patch until you have spent the time
>>> understanding what the page cache is.
>>>
>>> I know you'll just ignore this, and keep on trucking just like you ignored
>>> the other messages from Stephen before.
>>>
>>> But if you want to work on the kernel, this isn't the way to do it, and
>>> nobody will ever take a patch from you seriously if you continue in this
>>> fashion.
>>>
>>> Dave.
>> Dave ,
>> Seems I need to have tested this code first.
>> Regards Nick
>
>
> No you needed to do a lot more, these one line replies from you are
> quite stupid,
>
> You are quite deliberately missing the point of people trying to help you,
>
> Dave.

Honestly ,
I understand and thanks for the help a lot. I am wondering if some one
can give me
a pointer on where to download the git repos for the btrfs user spaces
tools as it
seems on the btrfs tools on the wiki.
Regards Nick

Re: [PATCH] Remove certain calls for releasing page cache

[Duncan]

Nick Krause posted on Thu, 31 Jul 2014 00:14:50 -0400 as excerpted:

>  I am wondering if some one can
> give me a pointer on where to download the git repos for the btrfs user
> spaces tools as it seems on the btrfs tools on the wiki.

https://btrfs.wiki.kernel.org/index.php/Btrfs_source_repositories#btrfs-progs_git_repository

You mean the URLs listed there?  The official repo listed there is the same one
found in the gentoo btrfs-progs live-ebuild, for those (like me) that prefer
that to the versioned ebuild.  I've no reason to believe that the integration
-branch URLs listed there are invalid, either, tho I've personally stuck to the
offical repo, master branch, which as of the last time I did an update was still
at v3.14.2, tho I've seen enough patches on the list I'm expecting a v3.16
release shortly after the v3.16 kernel release.

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

Re: [PATCH] Remove certain calls for releasing page cache

[Hugo Mills]

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

On Wed, Jul 30, 2014 at 10:05:16PM -0400, Nick Krause wrote:
> On Wed, Jul 30, 2014 at 7:30 PM, Dave Airlie <airlied@gmail.com> wrote:
> >> This patch removes the lines for releasing the page cache in certain
> >> files as this may aid in perfomance with writes in the compression
> >> rountines of btrfs. Please note that this patch has not been tested
> >> on my own hardware due to no compression based btrfs volumes of my
> >> own.
> >>
> >
> > For all that is sacred, STOP.
[snip]
> > But if you want to work on the kernel, this isn't the way to do it, and
> > nobody will ever take a patch from you seriously if you continue in this
> > fashion.
> >
> > Dave.
> Dave ,
> Seems I need to have tested this code first.

   You've said this before, having made exactly the same error (not
testing a patch). Yet you do it again. You seem to be ignoring all the
advice you've been given -- or at least not learning from it, and not
learning from your experiences. Could you please, for half an hour or
so, stop thinking about the immediate goal of getting a patch into the
kernel, and take a short while to think about your process of
learning. Look at all the advice you've had (from me, from Ted, from
others), actually understand it, and consider all the things you need
to do which *aren't* hacking up a lump of C. Actually learn these
things -- have them in your mind all the time.

   I would appreciate it if you could actually engage with someone
(doesn't have to be me) about this -- why are you ignoring the advice?
Is it because you don't understand it? Is it because you think you can
cut corners? Is it because you're concetrating on the code so much that
you're forgetting it?

   The main thing you're doing which is making people angry is not
because you're submitting bad patches (although you are). It's because
you're not listening to advice, and you're not apparently learning
anything from the feedback you're given. Your behaviour is not
changing over time, which makes you look like a waste of time to all
those people trying to help you.

   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
         --- That's not rain, that's a lake with slots in it ---         

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

Re: [PATCH] Remove certain calls for releasing page cache

[Nick Krause]

On Thu, Jul 31, 2014 at 6:11 AM, Hugo Mills <hugo@carfax.org.uk> wrote:
> On Wed, Jul 30, 2014 at 10:05:16PM -0400, Nick Krause wrote:
>> On Wed, Jul 30, 2014 at 7:30 PM, Dave Airlie <airlied@gmail.com> wrote:
>> >> This patch removes the lines for releasing the page cache in certain
>> >> files as this may aid in perfomance with writes in the compression
>> >> rountines of btrfs. Please note that this patch has not been tested
>> >> on my own hardware due to no compression based btrfs volumes of my
>> >> own.
>> >>
>> >
>> > For all that is sacred, STOP.
> [snip]
>> > But if you want to work on the kernel, this isn't the way to do it, and
>> > nobody will ever take a patch from you seriously if you continue in this
>> > fashion.
>> >
>> > Dave.
>> Dave ,
>> Seems I need to have tested this code first.
>
>    You've said this before, having made exactly the same error (not
> testing a patch). Yet you do it again. You seem to be ignoring all the
> advice you've been given -- or at least not learning from it, and not
> learning from your experiences. Could you please, for half an hour or
> so, stop thinking about the immediate goal of getting a patch into the
> kernel, and take a short while to think about your process of
> learning. Look at all the advice you've had (from me, from Ted, from
> others), actually understand it, and consider all the things you need
> to do which *aren't* hacking up a lump of C. Actually learn these
> things -- have them in your mind all the time.
>
>    I would appreciate it if you could actually engage with someone
> (doesn't have to be me) about this -- why are you ignoring the advice?
> Is it because you don't understand it? Is it because you think you can
> cut corners? Is it because you're concetrating on the code so much that
> you're forgetting it?
>
>    The main thing you're doing which is making people angry is not
> because you're submitting bad patches (although you are). It's because
> you're not listening to advice, and you're not apparently learning
> anything from the feedback you're given. Your behaviour is not
> changing over time, which makes you look like a waste of time to all
> those people trying to help you.
>
>    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
>          --- That's not rain, that's a lake with slots in it ---

Hugo,
That makes sense seems I need to listen better and test my code first, which is
bad in any environment.
Regards Nick