On Fri, Oct 04, 2013 at 07:13:31AM +0100, Phillip Lougher wrote: > Minchan Kim <minc...@kernel.org> wrote: > >Sqsuashfs have used cache for normal data pages but it's pointless > >because MM already has cache layer and squashfs adds extra pages > >into MM's page cache when it reads a page from compressed block. > > > >This patch removes cache usage for normal data pages so it could > >remove unnecessary one copy > > 1. As I mentioned last week, the use of the "unnecessary" cache is there > to prevent two or more processes simultaneously trying to read the same > pages. Without this, such racing processes will decompress the same > blocks repeatedly. > > It is easy to dismiss this as an rare event, but, when it happens it > has a major impact on performance, because the racing processes > can get stuck in a lock-step arrangement, repeatedly trying to access > the same blocks until the eof. If the file is many megabytes or > gigabytes in size (such as when Squashfs is used as a container fs for > cetain liveCDs, or virtual machine disk images) this will lead to > a significant reduction in performance. > > So I consider this a major regression. > > 2. You patch also adds another regression, which is to reintroduce > kmap() rather than kmap_atomic(). > > I was asked to remove this at my first submission attempt > in 2005 > > http://lkml.indiana.edu/hypermail/linux/kernel/0503.2/0809.html > > So I'm not particularly willing to reintroduce it now. > > 3. Your patch potentially reintroduces this bug > > http://www.spinics.net/lists/linux-fsdevel/msg02555.html > http://zaitcev.livejournal.com/86954.html > > 4. You patch is unconditional. With such code changes as this > it is always essential to make this a "buy in option", with the > original behaviour retained as default. Otherwise, lots of users > potentially find their embedded/enterprise/mission critical > system unexpectedly breaks when upgrading the kernel, and I get > a lot of angry email. > > Phillip
I will handle all your points in next patchset. Thanks for the review! -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
