| Message ID | 1406752954-26158-1-git-send-email-xerofoify@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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 -- 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 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 -- 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 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 -- 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
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 -- 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
> 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. -- 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 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 -- 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 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. -- 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 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 -- 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
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.
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.
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 -- 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/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); }
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(-)