Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752542AbdIWCBd (ORCPT ); Fri, 22 Sep 2017 22:01:33 -0400 Received: from mail-ua0-f195.google.com ([209.85.217.195]:34290 "EHLO mail-ua0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751845AbdIWCBc (ORCPT ); Fri, 22 Sep 2017 22:01:32 -0400 X-Google-Smtp-Source: AOwi7QAZGUPsjamYABr+wAtHeRvGyeqPK2ukJSrCC3egDunknVueIeGUPAYiamOx5EIj4YfSH3B0a1yCSXWps2oiaEs= MIME-Version: 1.0 In-Reply-To: <20170922215508.73407-2-drosen@google.com> References: <20170922215508.73407-1-drosen@google.com> <20170922215508.73407-2-drosen@google.com> From: Phillip Lougher Date: Sat, 23 Sep 2017 03:01:30 +0100 Message-ID: Subject: Re: [PATCH 1/5] Squashfs: remove the FILE_CACHE option To: Daniel Rosenberg , LKML Cc: Phillip Lougher , adrien@schischi.me, Adrien Schildknecht Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7517 Lines: 210 On Fri, Sep 22, 2017 at 10:55 PM, Daniel Rosenberg wrote: > From: Adrien Schildknecht > > FILE_DIRECT is working fine and offers faster results and lower memory > footprint. > > Removing FILE_CACHE makes our life easier because we don't have to > maintain 2 differents function that does the same thing. When I added FILE_DIRECT, I deliberately retained the original FILE_CACHE behaviour and kept it as default, and I spent a lot of effort to do so. It basically required complete rewriting to enable FILE_CACHE and FILE_DIRECT to co-exist. FILE_CACHE wasn't simply some old cruft I left in there because I couldn't be bothered to remove it. There is a good reason for keeping the FILE_CACHE behaviour. While FILE_DIRECT improves the performance of Squashfs by eliminating a memcpy, and removing contention on a lock, it also increases the amount of I/O and CPU Squashfs can use at any one time (by enabling multiple Squashfs operations to run in parallel). This changes scheduling and can take resources away from other tasks. Who knows how many low-end embedded systems are out there that rely on the original slower behaviour of FILE_CACHE and will break if it is removed. I didn't want to remove the FILE_CACHE behaviour and get a lot of people complaining (and quite rightly too) that I'd removed a feature they were relying on, and I don't intend to now either. You quite simply don't remove a feature that people may be relying on. Let's be clear. Your reason for removing FILE_CACHE is simply because when adding your new whizzo feature, you needed to update both the FILE_CACHE and FILE_DIRECT code, and as *you* didn't use FILE_CACHE, you couldn't be bothered to update it, and removed it instead. This is not a valid reason for removing it. NACK Phillip > > Signed-off-by: Adrien Schildknecht > Signed-off-by: Daniel Rosenberg > --- > fs/squashfs/Kconfig | 28 ---------------------------- > fs/squashfs/Makefile | 3 +-- > fs/squashfs/file_cache.c | 38 -------------------------------------- > fs/squashfs/page_actor.h | 42 +----------------------------------------- > 4 files changed, 2 insertions(+), 109 deletions(-) > delete mode 100644 fs/squashfs/file_cache.c > > diff --git a/fs/squashfs/Kconfig b/fs/squashfs/Kconfig > index 1adb3346b9d6..6c81bf620067 100644 > --- a/fs/squashfs/Kconfig > +++ b/fs/squashfs/Kconfig > @@ -25,34 +25,6 @@ config SQUASHFS > > If unsure, say N. > > -choice > - prompt "File decompression options" > - depends on SQUASHFS > - help > - Squashfs now supports two options for decompressing file > - data. Traditionally Squashfs has decompressed into an > - intermediate buffer and then memcopied it into the page cache. > - Squashfs now supports the ability to decompress directly into > - the page cache. > - > - If unsure, select "Decompress file data into an intermediate buffer" > - > -config SQUASHFS_FILE_CACHE > - bool "Decompress file data into an intermediate buffer" > - help > - Decompress file data into an intermediate buffer and then > - memcopy it into the page cache. > - > -config SQUASHFS_FILE_DIRECT > - bool "Decompress files directly into the page cache" > - help > - Directly decompress file data into the page cache. > - Doing so can significantly improve performance because > - it eliminates a memcpy and it also removes the lock contention > - on the single buffer. > - > -endchoice > - > choice > prompt "Decompressor parallelisation options" > depends on SQUASHFS > diff --git a/fs/squashfs/Makefile b/fs/squashfs/Makefile > index 6655631c53ae..225330ab7723 100644 > --- a/fs/squashfs/Makefile > +++ b/fs/squashfs/Makefile > @@ -5,8 +5,7 @@ > obj-$(CONFIG_SQUASHFS) += squashfs.o > squashfs-y += block.o cache.o dir.o export.o file.o fragment.o id.o inode.o > squashfs-y += namei.o super.o symlink.o decompressor.o > -squashfs-$(CONFIG_SQUASHFS_FILE_CACHE) += file_cache.o > -squashfs-$(CONFIG_SQUASHFS_FILE_DIRECT) += file_direct.o page_actor.o > +squashfs-y += file_direct.o page_actor.o > squashfs-$(CONFIG_SQUASHFS_DECOMP_SINGLE) += decompressor_single.o > squashfs-$(CONFIG_SQUASHFS_DECOMP_MULTI) += decompressor_multi.o > squashfs-$(CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU) += decompressor_multi_percpu.o > diff --git a/fs/squashfs/file_cache.c b/fs/squashfs/file_cache.c > deleted file mode 100644 > index f2310d2a2019..000000000000 > --- a/fs/squashfs/file_cache.c > +++ /dev/null > @@ -1,38 +0,0 @@ > -/* > - * Copyright (c) 2013 > - * Phillip Lougher > - * > - * This work is licensed under the terms of the GNU GPL, version 2. See > - * the COPYING file in the top-level directory. > - */ > - > -#include > -#include > -#include > -#include > -#include > -#include > -#include > - > -#include "squashfs_fs.h" > -#include "squashfs_fs_sb.h" > -#include "squashfs_fs_i.h" > -#include "squashfs.h" > - > -/* Read separately compressed datablock and memcopy into page cache */ > -int squashfs_readpage_block(struct page *page, u64 block, int bsize) > -{ > - struct inode *i = page->mapping->host; > - struct squashfs_cache_entry *buffer = squashfs_get_datablock(i->i_sb, > - block, bsize); > - int res = buffer->error; > - > - if (res) > - ERROR("Unable to read page, block %llx, size %x\n", block, > - bsize); > - else > - squashfs_copy_cache(page, buffer, buffer->length, 0); > - > - squashfs_cache_put(buffer); > - return res; > -} > diff --git a/fs/squashfs/page_actor.h b/fs/squashfs/page_actor.h > index 98537eab27e2..d2df0544e0df 100644 > --- a/fs/squashfs/page_actor.h > +++ b/fs/squashfs/page_actor.h > @@ -8,46 +8,6 @@ > * the COPYING file in the top-level directory. > */ > > -#ifndef CONFIG_SQUASHFS_FILE_DIRECT > -struct squashfs_page_actor { > - void **page; > - int pages; > - int length; > - int next_page; > -}; > - > -static inline struct squashfs_page_actor *squashfs_page_actor_init(void **page, > - int pages, int length) > -{ > - struct squashfs_page_actor *actor = kmalloc(sizeof(*actor), GFP_KERNEL); > - > - if (actor == NULL) > - return NULL; > - > - actor->length = length ? : pages * PAGE_SIZE; > - actor->page = page; > - actor->pages = pages; > - actor->next_page = 0; > - return actor; > -} > - > -static inline void *squashfs_first_page(struct squashfs_page_actor *actor) > -{ > - actor->next_page = 1; > - return actor->page[0]; > -} > - > -static inline void *squashfs_next_page(struct squashfs_page_actor *actor) > -{ > - return actor->next_page == actor->pages ? NULL : > - actor->page[actor->next_page++]; > -} > - > -static inline void squashfs_finish_page(struct squashfs_page_actor *actor) > -{ > - /* empty */ > -} > -#else > struct squashfs_page_actor { > union { > void **buffer; > @@ -77,5 +37,5 @@ static inline void squashfs_finish_page(struct squashfs_page_actor *actor) > { > actor->squashfs_finish_page(actor); > } > -#endif > + > #endif > -- > 2.14.1.821.g8fa685d3b7-goog >