lkml.org 
[lkml]   [2013]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 6/6] Squashfs: Directly decompress into the page cache for file data (V2)
    On Thu, Nov 07, 2013 at 08:24:25PM +0000, Phillip Lougher wrote:
    > This introduces an implementation of squashfs_readpage_block()
    > that directly decompresses into the page cache.
    >
    > This uses the previously added page handler abstraction to push
    > down the necessary kmap_atomic/kunmap_atomic operations on the
    > page cache buffers into the decompressors. This enables
    > direct copying into the page cache without using the slow
    > kmap/kunmap calls.
    >
    > The code detects when multiple threads are racing in
    > squashfs_readpage() to decompress the same block, and avoids
    > this regression by falling back to using an intermediate
    > buffer.
    >
    > This patch enhances the performance of Squashfs significantly
    > when multiple processes are accessing the filesystem simultaneously
    > because it not only reduces memcopying, but it more importantly
    > eliminates the lock contention on the intermediate buffer.
    >
    > Using single-thread decompression.
    >
    > dd if=file1 of=/dev/null bs=4096 &
    > dd if=file2 of=/dev/null bs=4096 &
    > dd if=file3 of=/dev/null bs=4096 &
    > dd if=file4 of=/dev/null bs=4096
    >
    > Before:
    >
    > 629145600 bytes (629 MB) copied, 45.8046 s, 13.7 MB/s
    >
    > After:
    >
    > 629145600 bytes (629 MB) copied, 9.29414 s, 67.7 MB/s
    >
    > V2:
    > * update comment adding failure to grab pages could be
    > because we've been VM reclaimed, but the other pages are
    > still in the page cache and uptodate.
    > * Make Kconfig option a choice, making the either-other nature of
    > the option more explicit, and also tidying up the ifdef in the
    > Makefile
    >
    > Signed-off-by: Phillip Lougher <phillip@squashfs.org.uk>
    > ---
    > fs/squashfs/Kconfig | 28 +++++++
    > fs/squashfs/Makefile | 4 +-
    > fs/squashfs/file_direct.c | 178 +++++++++++++++++++++++++++++++++++++++++++++
    > fs/squashfs/page_actor.c | 104 ++++++++++++++++++++++++++
    > fs/squashfs/page_actor.h | 32 ++++++++
    > 5 files changed, 345 insertions(+), 1 deletion(-)
    > create mode 100644 fs/squashfs/file_direct.c
    > create mode 100644 fs/squashfs/page_actor.c
    >
    > diff --git a/fs/squashfs/Kconfig b/fs/squashfs/Kconfig
    > index c92c75f..3a21adf 100644
    > --- a/fs/squashfs/Kconfig
    > +++ b/fs/squashfs/Kconfig
    > @@ -26,6 +26,34 @@ 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 mempcpy and it also removes the lock contention

    memcpy

    > + on the single buffer.
    > +
    > +endchoice
    > +
    > +choice
    > prompt "Decompressor parallelisation options"
    > depends on SQUASHFS
    > help
    > diff --git a/fs/squashfs/Makefile b/fs/squashfs/Makefile
    > index 908c0d9..4132520 100644
    > --- a/fs/squashfs/Makefile
    > +++ b/fs/squashfs/Makefile
    > @@ -4,7 +4,9 @@
    >
    > 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 file_cache.c
    > +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-$(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_direct.c b/fs/squashfs/file_direct.c
    > new file mode 100644
    > index 0000000..d020d94
    > --- /dev/null
    > +++ b/fs/squashfs/file_direct.c
    > @@ -0,0 +1,178 @@
    > +/*
    > + * Copyright (c) 2013
    > + * Phillip Lougher <phillip@squashfs.org.uk>
    > + *
    > + * This work is licensed under the terms of the GNU GPL, version 2. See
    > + * the COPYING file in the top-level directory.
    > + */
    > +
    > +#include <linux/fs.h>
    > +#include <linux/vfs.h>
    > +#include <linux/kernel.h>
    > +#include <linux/slab.h>
    > +#include <linux/string.h>
    > +#include <linux/pagemap.h>
    > +#include <linux/mutex.h>
    > +
    > +#include "squashfs_fs.h"
    > +#include "squashfs_fs_sb.h"
    > +#include "squashfs_fs_i.h"
    > +#include "squashfs.h"
    > +#include "page_actor.h"
    > +
    > +static int squashfs_read_cache(struct page *target_page, u64 block, int bsize,
    > + int pages, struct page **page);
    > +
    > +/* Read separately compressed datablock directly into page cache */
    > +int squashfs_readpage_block(struct page *target_page, u64 block, int bsize)
    > +
    > +{
    > + struct inode *inode = target_page->mapping->host;
    > + struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
    > +
    > + int file_end = (i_size_read(inode) - 1) >> PAGE_CACHE_SHIFT;
    > + int mask = (1 << (msblk->block_log - PAGE_CACHE_SHIFT)) - 1;
    > + int start_index = target_page->index & ~mask;
    > + int end_index = start_index | mask;
    > + int i, n, pages, missing_pages, bytes, res = -ENOMEM;
    > + struct page **page;
    > + struct squashfs_page_actor *actor;
    > + void *pageaddr;
    > +
    > + if (end_index > file_end)
    > + end_index = file_end;
    > +
    > + pages = end_index - start_index + 1;
    > +
    > + page = kmalloc(sizeof(void *) * pages, GFP_KERNEL);
    > + if (page == NULL)
    > + goto error_out;
    > +
    > + /*
    > + * Create a "page actor" which will kmap and kunmap the
    > + * page cache pages appropriately within the decompressor
    > + */
    > + actor = squashfs_page_actor_init_special(page, pages, 0);
    > + if (actor == NULL)
    > + goto error_out2;
    > +
    > + /* Try to grab all the pages covered by the Squashfs block */
    > + for (missing_pages = 0, i = 0, n = start_index; i < pages; i++, n++) {
    > + page[i] = (n == target_page->index) ? target_page :
    > + grab_cache_page_nowait(target_page->mapping, n);
    > +
    > + if (page[i] == NULL) {
    > + missing_pages++;
    > + continue;
    > + }
    > +
    > + if (PageUptodate(page[i])) {
    > + unlock_page(page[i]);
    > + page_cache_release(page[i]);
    > + page[i] = NULL;
    > + missing_pages++;
    > + }
    > + }
    > +
    > + if (missing_pages) {
    > + /*
    > + * Couldn't get one or more pages, this page has either
    > + * been VM reclaimed, but others are still in the page cache
    > + * and uptodate, or we're racing with another thread in
    > + * squashfs_readpage also trying to grab them. Fall back to
    > + * using an intermediate buffer.
    > + */
    > + kfree(actor);
    > + return squashfs_read_cache(target_page, block, bsize, pages,
    > + page);
    > + }
    > +
    > + /* Decompress directly into the page cache buffers */
    > + res = squashfs_read_data(inode->i_sb, block, bsize, NULL, actor);
    > + if (res < 0)
    > + goto mark_errored;
    > +
    > + /* Last page may have trailing bytes not filled */
    > + bytes = res % PAGE_CACHE_SIZE;
    > + if (bytes) {
    > + pageaddr = kmap_atomic(page[pages - 1]);
    > + memset(pageaddr + bytes, 0, PAGE_CACHE_SIZE - bytes);
    > + kunmap_atomic(pageaddr);
    > + }
    > +
    > + /* Mark pages as uptodate, unlock and release */
    > + for (i = 0; i < pages; i++) {
    > + flush_dcache_page(page[i]);
    > + SetPageUptodate(page[i]);
    > + unlock_page(page[i]);
    > + if (page[i] != target_page)
    > + page_cache_release(page[i]);
    > + }
    > +
    > + kfree(actor);
    > + kfree(page);
    > +
    > + return 0;
    > +
    > +mark_errored:
    > + /* Decompression failed, mark pages as errored. Target_page is
    > + * dealt with by the caller
    > + */
    > + for (i = 0; i < pages; i++) {
    > + if (page[i] == target_page)
    > + continue;
    > + pageaddr = kmap_atomic(page[i]);
    > + memset(pageaddr, 0, PAGE_CACHE_SIZE);
    Do we need page zeroing?
    If others see !PG_uptodate, it will retry to read so I guess we don't need it.

    > + kunmap_atomic(pageaddr);
    > + flush_dcache_page(page[i]);
    > + SetPageError(page[i]);
    > + unlock_page(page[i]);
    > + page_cache_release(page[i]);
    > + }
    > +
    > + kfree(actor);
    > +error_out2:
    > + kfree(page);
    > +error_out:
    > + return res;
    > +}
    > +
    > +
    > +static int squashfs_read_cache(struct page *target_page, u64 block, int bsize,
    > + int pages, struct page **page)
    > +{
    > + struct inode *i = target_page->mapping->host;
    > + struct squashfs_cache_entry *buffer = squashfs_get_datablock(i->i_sb,
    > + block, bsize);
    > + int bytes = buffer->length, res = buffer->error, n, offset = 0;
    > + void *pageaddr;
    > +
    > + if (res) {
    > + ERROR("Unable to read page, block %llx, size %x\n", block,
    > + bsize);
    > + goto out;
    > + }
    > +
    > + for (n = 0; n < pages && bytes > 0; n++,
    > + bytes -= PAGE_CACHE_SIZE, offset += PAGE_CACHE_SIZE) {
    > + int avail = min_t(int, bytes, PAGE_CACHE_SIZE);
    > +
    > + if (page[n] == NULL)
    > + continue;
    > +
    > + pageaddr = kmap_atomic(page[n]);
    > + squashfs_copy_data(pageaddr, buffer, offset, avail);
    > + memset(pageaddr + avail, 0, PAGE_CACHE_SIZE - avail);
    > + kunmap_atomic(pageaddr);
    > + flush_dcache_page(page[n]);
    > + SetPageUptodate(page[n]);
    > + unlock_page(page[n]);
    > + if (page[n] != target_page)
    > + page_cache_release(page[n]);
    > + }
    > +
    > +out:
    > + squashfs_cache_put(buffer);

    Nitpick:

    It would be better to free page in caller rather than caller if the function
    return error?

    > + kfree(page);
    > + return res;
    > +}
    > diff --git a/fs/squashfs/page_actor.c b/fs/squashfs/page_actor.c
    > new file mode 100644
    > index 0000000..8e754ff
    > --- /dev/null
    > +++ b/fs/squashfs/page_actor.c
    > @@ -0,0 +1,104 @@
    > +/*
    > + * Copyright (c) 2013
    > + * Phillip Lougher <phillip@squashfs.org.uk>
    > + *
    > + * This work is licensed under the terms of the GNU GPL, version 2. See
    > + * the COPYING file in the top-level directory.
    > + */
    > +
    > +#include <linux/kernel.h>
    > +#include <linux/slab.h>
    > +#include <linux/pagemap.h>
    > +#include "page_actor.h"
    > +
    > +/* Implementation of page_actor for decompressing into intermediate buffer */
    > +static void *cache_first_page(struct squashfs_page_actor *actor)
    > +{
    > + actor->next_page = 1;
    > + return actor->buffer[0];
    > +}
    > +
    > +static void *cache_next_page(struct squashfs_page_actor *actor)
    > +{
    > + if (actor->next_page == actor->pages)
    > + return NULL;
    > +
    > + return actor->buffer[actor->next_page++];
    > +}
    > +
    > +static void cache_finish_page(struct squashfs_page_actor *actor)
    > +{
    > + /* empty */
    > +}
    > +
    > +struct squashfs_page_actor *squashfs_page_actor_init(void **buffer,
    > + int pages, int length)
    > +{
    > + struct squashfs_page_actor *actor = kmalloc(sizeof(*actor), GFP_KERNEL);
    > +
    > + if (actor == NULL)
    > + return NULL;
    > +
    > + if (length)
    > + actor->length = length;
    > + else
    > + actor->length = pages * PAGE_CACHE_SIZE;
    > + actor->buffer = buffer;
    > + actor->pages = pages;
    > + actor->next_page = 0;
    > +
    > + actor->squashfs_first_page = cache_first_page;
    > + actor->squashfs_next_page = cache_next_page;
    > + actor->squashfs_finish_page = cache_finish_page;
    > + return actor;
    > +}
    > +
    > +/* Implementation of page_actor for decompressing directly into page cache */
    > +static void *direct_first_page(struct squashfs_page_actor *actor)
    > +{
    > + actor->next_page = 1;
    > + return actor->pageaddr = kmap_atomic(actor->page[0]);
    > +}


    Just my two cents

    It makes new rule that we shouldn't call blocking function during page
    enumerating with page_actor. Somewhere comment about that will be helpful.

    > +
    > +static void *direct_next_page(struct squashfs_page_actor *actor)
    > +{
    > + if (actor->pageaddr)
    > + kunmap_atomic(actor->pageaddr);
    > +
    > + if (actor->next_page == actor->pages) {
    > + actor->pageaddr = NULL;
    > + return NULL;
    > + }
    > +
    > + return actor->pageaddr = kmap_atomic(actor->page[actor->next_page++]);
    > +}
    > +
    > +static void direct_finish_page(struct squashfs_page_actor *actor)
    > +{
    > + if (actor->pageaddr)
    > + kunmap_atomic(actor->pageaddr);
    > +}
    > +
    > +
    > +struct squashfs_page_actor *squashfs_page_actor_init_special(struct page **page,
    > + int pages, int length)
    > +{
    > + struct squashfs_page_actor *actor = kmalloc(sizeof(*actor), GFP_KERNEL);
    > +
    > + if (actor == NULL)
    > + return NULL;
    > +
    > + if (length)
    > + actor->length = length;
    > + else
    > + actor->length = pages * PAGE_CACHE_SIZE;
    > + actor->page = page;
    > + actor->pages = pages;
    > + actor->next_page = 0;
    > + actor->pageaddr = NULL;
    > +
    > + actor->squashfs_first_page = direct_first_page;
    > + actor->squashfs_next_page = direct_next_page;
    > + actor->squashfs_finish_page = direct_finish_page;
    > + return actor;
    > +}
    > diff --git a/fs/squashfs/page_actor.h b/fs/squashfs/page_actor.h
    > index 19a66a3..22731c7 100644
    > --- a/fs/squashfs/page_actor.h
    > +++ b/fs/squashfs/page_actor.h
    > @@ -8,6 +8,7 @@
    > * the COPYING file in the top-level directory.
    > */
    >
    > +#ifndef CONFIG_SQUASHFS_FILE_DIRECT
    > struct squashfs_page_actor {
    > void **page;
    > int pages;
    > @@ -51,4 +52,35 @@ static inline void squashfs_finish_page(struct squashfs_page_actor *actor)
    > {
    > /* empty */
    > }
    > +#else
    > +struct squashfs_page_actor {
    > + union {
    > + void **buffer;
    > + struct page **page;
    > + };
    > + void *pageaddr;
    > + void *(*squashfs_first_page)(struct squashfs_page_actor *);
    > + void *(*squashfs_next_page)(struct squashfs_page_actor *);
    > + void (*squashfs_finish_page)(struct squashfs_page_actor *);
    > + int pages;
    > + int length;
    > + int next_page;
    > +};
    > +
    > +extern struct squashfs_page_actor *squashfs_page_actor_init(void **, int, int);
    > +extern struct squashfs_page_actor *squashfs_page_actor_init_special(struct page
    > + **, int, int);
    > +static inline void *squashfs_first_page(struct squashfs_page_actor *actor)
    > +{
    > + return actor->squashfs_first_page(actor);
    > +}
    > +static inline void *squashfs_next_page(struct squashfs_page_actor *actor)
    > +{
    > + return actor->squashfs_next_page(actor);
    > +}
    > +static inline void squashfs_finish_page(struct squashfs_page_actor *actor)
    > +{
    > + actor->squashfs_finish_page(actor);
    > +}
    > +#endif
    > #endif

    Most of thing from me are just nitpicks.
    Looks great to me.

    Thanks, Phillip.

    Reviewed-by: Minchan Kim <minchan@kernel.org>

    --
    Kind regards,
    Minchan Kim


    \
     
     \ /
      Last update: 2013-11-08 09:41    [W:4.363 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site