re: [PATCH] squashfs: enhance parallel I/O
From: Phillip Lougher
Date: Wed Oct 23 2013 - 02:22:09 EST
Hi Minchan,
Apologies for the lateness of this review, I had forgotten I'd not
send it.
Minchan Kim <minchan@xxxxxxxxxx> wrote:
Now squashfs have used for only one stream buffer for decompression
so it hurts concurrent read performance so this patch supprts
multiple decompressor to enhance performance concurrent I/O.
Four 1G file dd read on KVM machine which has 2 CPU and 4G memory.
dd if=test/test1.dat of=/dev/null &
dd if=test/test2.dat of=/dev/null &
dd if=test/test3.dat of=/dev/null &
dd if=test/test4.dat of=/dev/null &
old : 1m39s -> new : 9s
This patch is based on [1].
[1] Squashfs: Refactor decompressor interface and code
Cc: Phillip Lougher <phillip@xxxxxxxxxxxxxxx>
Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
---
fs/squashfs/Kconfig | 12 +++
fs/squashfs/Makefile | 9 +-
fs/squashfs/decompressor_multi.c | 194 ++++++++++++++++++++++++++++++++++++++
3 files changed, 214 insertions(+), 1 deletion(-)
create mode 100644 fs/squashfs/decompressor_multi.c
diff --git a/fs/squashfs/Kconfig b/fs/squashfs/Kconfig
index c70111e..7a580d0 100644
--- a/fs/squashfs/Kconfig
+++ b/fs/squashfs/Kconfig
@@ -63,6 +63,18 @@ config SQUASHFS_LZO
If unsure, say N.
+config SQUASHFS_MULTI_DECOMPRESSOR
Small alterations to the English, I don't like doing this because
English is probably a foreign language to you, and my Korean is non-existent!
but, this is user visible and you did say you wanted review !
+ bool "Use multiple decompressor for handling concurrent I/O"
bool "Use multiple decompressors for handling parallel I/O"
Concurrent is subtly different to parallel, and you use parallel in
the following description.
+ depends on SQUASHFS
+ help
+ By default Squashfs uses a compressor for data but it gives
+ poor performance on parallel I/O workload of multiple CPU
+ mahchine due to waitting a compressor available.
+
By default Squashfs uses a single decompressor but it gives
poor performance on parallel I/O workloads when using multiple CPU
machines due to waiting on decompressor availability.
+ If workload has parallel I/O and your system has enough memory,
+ this option may improve overall I/O performance.
+ If unsure, say N.
+
If you have a parallel I/O workload and your system has enough memory,
using this option may improve overall I/O performance.
If unsure, say N.
+
config SQUASHFS_XZ
bool "Include support for XZ compressed file systems"
depends on SQUASHFS
diff --git a/fs/squashfs/Makefile b/fs/squashfs/Makefile
index c223c84..dfebc3b 100644
--- a/fs/squashfs/Makefile
+++ b/fs/squashfs/Makefile
@@ -4,8 +4,15 @@
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 decompressor_single.o
+squashfs-y += namei.o super.o symlink.o decompressor.o
+
squashfs-$(CONFIG_SQUASHFS_XATTR) += xattr.o xattr_id.o
squashfs-$(CONFIG_SQUASHFS_LZO) += lzo_wrapper.o
squashfs-$(CONFIG_SQUASHFS_XZ) += xz_wrapper.o
squashfs-$(CONFIG_SQUASHFS_ZLIB) += zlib_wrapper.o
+
+ifdef CONFIG_SQUASHFS_MULTI_DECOMPRESSOR
+ squashfs-y += decompressor_multi.o
+else
+ squashfs-y += decompressor_single.o
+endif
diff --git a/fs/squashfs/decompressor_multi.c b/fs/squashfs/decompressor_multi.c
new file mode 100644
index 0000000..23f1e94
--- /dev/null
+++ b/fs/squashfs/decompressor_multi.c
@@ -0,0 +1,194 @@
+/*
+ * Copyright (c) 2013
+ * Minchan Kim <minchan@xxxxxxxxxx>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+#include <linux/types.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/buffer_head.h>
+#include <linux/sched.h>
+#include <linux/wait.h>
+#include <linux/cpumask.h>
+
+#include "squashfs_fs.h"
+#include "squashfs_fs_sb.h"
+#include "decompressor.h"
+#include "squashfs.h"
+
+/*
+ * This file implements multi-threaded decompression in the
+ * decompressor framework
+ */
+
+
+/*
+ * The reason that multiply two is that a CPU can request new I/O
+ * while it is waitting previous request.
s/waitting/waiting/
The English in the comments is understandable, and not user-visible, and
so I won't change it, except for spelling mistakes ...
+ */
+#define MAX_DECOMPRESSOR (num_online_cpus() * 2)
+
+
+int squashfs_max_decompressors(void)
+{
+ return MAX_DECOMPRESSOR;
+}
+
+
+struct squashfs_stream {
+ void *comp_opts;
+ struct list_head strm_list;
+ struct mutex mutex;
+ int avail_comp;
+ wait_queue_head_t wait;
+};
+
+
+struct comp_stream {
+ void *stream;
+ struct list_head list;
+};
+
One general small nitpick, you use "comp" to name things, comp_stream,
avail_comp etc. But, as this is a decompressor, it should more correctly
be decomp...
(note comp_opts is not decomp_opts because it is the compression options
selected by the user at compression time) ...
+
+static void put_comp_stream(struct comp_stream *comp_strm,
+ struct squashfs_stream *stream)
+{
+ mutex_lock(&stream->mutex);
+ list_add(&comp_strm->list, &stream->strm_list);
+ mutex_unlock(&stream->mutex);
+ wake_up(&stream->wait);
+}
+
+void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
+ void *comp_opts)
+{
+ struct squashfs_stream *stream;
+ struct comp_stream *comp_strm = NULL;
+ int err = -ENOMEM;
+
+ stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+ if (!stream)
+ goto out;
+
+ stream->comp_opts = comp_opts;
+ mutex_init(&stream->mutex);
+ INIT_LIST_HEAD(&stream->strm_list);
+ init_waitqueue_head(&stream->wait);
+
+ comp_strm = kmalloc(sizeof(*comp_strm), GFP_KERNEL);
+ if (!comp_strm)
+ goto out;
+
+ comp_strm->stream = msblk->decompressor->init(msblk,
+ stream->comp_opts);
+ if (IS_ERR(comp_strm->stream)) {
+ err = PTR_ERR(comp_strm->stream);
+ goto out;
+ }
+
+ list_add(&comp_strm->list, &stream->strm_list);
+ stream->avail_comp = 1;
I assume you're always creating a decompressor here (rather than
setting it to 0, and allocating the first one in get_comp_stream) to
ensure there's always at least one decompressor available going into
get_comp_stream()? So if decompressor creation fails in
get_comp_stream() we can always fall back to using the decompressors
already allocated, because we know there will always be at least one?
Maybe a comment to that effect? To show creating a decompressor here
and setting this to one is deliberate.... This will prevent others
from thinking they can "optimise" the code by having the first decompressor
created in get_comp_stream()!
/* ensure we have at least one decompressor in get_comp_stream() */ ?
+ return stream;
+
+out:
+ kfree(comp_strm);
+ kfree(stream);
+ return ERR_PTR(err);
+}
+
+
+void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
+{
+ struct squashfs_stream *stream = msblk->stream;
+ if (stream) {
+ struct comp_stream *comp_strm;
+
+ while (!list_empty(&stream->strm_list)) {
+ comp_strm = list_entry(stream->strm_list.prev,
+ struct comp_stream, list);
+ list_del(&comp_strm->list);
+ msblk->decompressor->free(comp_strm->stream);
+ kfree(comp_strm);
+ stream->avail_comp--;
+ }
+ }
+
+ WARN_ON(stream->avail_comp);
+ kfree(stream->comp_opts);
+ kfree(stream);
+}
+
+
+static struct comp_stream *get_comp_stream(struct squashfs_sb_info *msblk,
+ struct squashfs_stream *stream)
+{
+ struct comp_stream *comp_strm;
+
+ while (1) {
+ mutex_lock(&stream->mutex);
+
+ /* There is available comp_stream */
+ if (!list_empty(&stream->strm_list)) {
+ comp_strm = list_entry(stream->strm_list.prev,
+ struct comp_stream, list);
+ list_del(&comp_strm->list);
+ mutex_unlock(&stream->mutex);
+ break;
+ }
+
+ /*
+ * If there is no available comp and already full,
+ * let's wait for releasing comp from other users.
+ */
+ if (stream->avail_comp >= MAX_DECOMPRESSOR)
+ goto wait;
+
+ /* Let's allocate new comp */
+ comp_strm = kmalloc(sizeof(*comp_strm), GFP_KERNEL);
+ if (!comp_strm)
+ goto wait;
+
+ comp_strm->stream = msblk->decompressor->init(msblk,
+ stream->comp_opts);
+ if (IS_ERR(comp_strm->stream)) {
+ kfree(comp_strm);
+ goto wait;
+ }
+
+ stream->avail_comp++;
+ WARN_ON(stream->avail_comp > MAX_DECOMPRESSOR);
+
+ mutex_unlock(&stream->mutex);
+ break;
+wait:
+ /*
+ * If system memory is tough, let's for other's
+ * releasing instead of hurting VM because it could
+ * make page cache thrashing.
+ */
+ mutex_unlock(&stream->mutex);
+ wait_event(stream->wait,
+ !list_empty(&stream->strm_list));
+ }
+
+ return comp_strm;
+}
+
+
+int squashfs_decompress(struct squashfs_sb_info *msblk,
+ void **buffer, struct buffer_head **bh, int b, int offset, int length,
+ int srclength, int pages)
+{
The interface here is changed by the introduction of the page
handler abstraction...
I can apply your patch after the refactoring patch and before the
"Squashfs: Directly decompress into the page cache for file" patch-set
and fix up the code here.... Or you can fix up the code? I'm
happy to do either, whatever you prefer.
Thanks
Phillip
+ int res;
+ struct squashfs_stream *stream = msblk->stream;
+ struct comp_stream *comp_stream = get_comp_stream(msblk, stream);
+ res = msblk->decompressor->decompress(msblk, comp_stream->stream,
+ buffer, bh, b, offset, length, srclength, pages);
+ put_comp_stream(comp_stream, stream);
+ if (res < 0)
+ ERROR("%s decompression failed, data probably corrupt\n",
+ msblk->decompressor->name);
+ return res;
+}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/