Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
From: Dennis Zhou <dennis@kernel.org>
To: dsterba@suse.cz, Dennis Zhou <dennis@kernel.org>,
	David Sterba <dsterba@suse.com>,
	Josef Bacik <josef@toxicpanda.com>, Chris Mason <clm@fb.com>,
	Omar Sandoval <osandov@osandov.com>,
	Nick Terrell <terrelln@fb.com>,
	kernel-team@fb.com, linux-btrfs@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 00/11] btrfs: add zstd compression level support
Date: Thu, 31 Jan 2019 10:56:02 -0500
Message-ID: <20190131155602.GA90850@dennisz-mbp> (raw)
In-Reply-To: <20190131140436.GD2900@twin.jikos.cz>

On Thu, Jan 31, 2019 at 03:04:36PM +0100, David Sterba wrote:
> On Wed, Jan 30, 2019 at 12:40:59PM -0500, Dennis Zhou wrote:
> > Hi David,
> > 
> > On Tue, Jan 29, 2019 at 06:18:30PM +0100, David Sterba wrote:
> > > On Mon, Jan 28, 2019 at 04:24:26PM -0500, Dennis Zhou wrote:
> > > > As mentioned above, a requirement that differs zstd from zlib is that
> > > > higher levels of compression require more memory. To manage this, each
> > > > compression level has its own queue of workspaces. A global LRU is used
> > > > to help with reclaim. To guarantee forward progress, a max level
> > > > workspace is preallocated and hidden from the LRU.
> > > 
> > > Here I'd like to bring up what was mentioned in previous iteration, the
> > > workspace sizes.
> > > 
> > > Level   Compression Memory
> > > 1       0.8 MB
> > > 2       1.0 MB
> > > 3       1.3 MB
> > > 4       0.9 MB
> > > 5       1.4 MB
> > > 6       1.5 MB
> > > 7       1.4 MB
> > > 8       1.8 MB
> > > 9       1.8 MB
> > > 10      1.8 MB
> > > 11      1.8 MB
> > > 12      1.8 MB
> > > 13      2.3 MB
> > > 14      2.6 MB
> > > 15      2.6 MB
> > > 
> > > and decompression needs memory of level 1. The sizes can be grouped
> > > together to say 3 sizes, I'm not sure that we'd really need 15 distinct
> > > workspaces. The reclaim mechanism helps, but I'd rather keep a smaller
> > > number of workspaces that covers average use.
> > > 
> > > Default level is 3, that's 1.3 MiB, that also covers level 1, 2 and 4.
> > > For 5 to 12 it's 1.8 and the rest is 2.6 MiB.
> > > 
> > 
> > I realize the current implementation doesn't have a monotonic memory
> > requirement guarantee. I've added that, and below is updated memory
> > requirements per level. I've updated the commit to include this too.
> > 
> > Level     Memory (KB)
> > 1            780 
> > 2           1004
> > 3           1260
> > 4           1260
> > 5           1388
> > 6           1516
> > 7           1516
> > 8           1772
> > 9           1772
> > 10          1772
> > 11          1772
> > 12          1772
> > 13          2284
> > 14          2547
> > 15          2547
> > 
> > > > btrfs filesystem 10 times and then read back after dropping the caches.
> > > > The btrfs filesystem was on an SSD.
> > > > 
> > > > Level   Ratio   Compression (MB/s)  Decompression (MB/s)
> > > > 1       2.658        438.47                910.51
> > > > 2       2.744        364.86                886.55
> > > > 3       2.801        336.33                828.41
> > > > 4       2.858        286.71                886.55
> > > > 5       2.916        212.77                556.84
> > > > 6       2.363        119.82                990.85
> > > > 7       3.000        154.06                849.30
> > > > 8       3.011        159.54                875.03
> > > > 9       3.025        100.51                940.15
> > > > 10      3.033        118.97                616.26
> > > > 11      3.036         94.19                802.11
> > > > 12      3.037         73.45                931.49
> > > > 13      3.041         55.17                835.26
> > > > 14      3.087         44.70                716.78
> > > > 15      3.126         37.30                878.84
> > > 
> > > From my casual user's perspective, I'd use the level 1 for speed, 7 for
> > > better ratio and 15 for the best compression. Anything else does not
> > > look good, though the results would vary based on the data set. I
> > > assume that the silesia corpus serves as a good approximation of the
> > > worst case average.
> > > 
> > > The levels 7-14 strike particularly obvious pattern: same ratio but the
> > > speed gets worse with each level. Taking the default level into account,
> > > (my) recommended levels would be 1, 3, 7, 15.
> > > 
> > 
> > I do see why we want to limit the number of levels as the memory
> > requirements do kind of bucket themselves. But, this means when zstd
> > gets updated, we'd have to reevaluate the compression levels btrfs
> > supports. I'm not sure it's a great idea to have that dependency.
> > I imagine we could offer some level of guidance, but it really would be
> > up to the user to figure out what works best for them.
> 
> If it was not clear, I did not mean to have only 4 levels, keep all 15
> same as there are 9 for zlib. The guildelines would be desirable and I
> don't want to make decision for the user which level to pick. So we
> don't disagree.
> 

I see, that was my misunderstanding.

> > The reclaim mechanism only keeps workspaces around if they are being
> > used by the appropriate level. So, the memory overhead is actively used
> > memory and if not, it is reclaimed after at most ~2 minutes later. I
> > also scan up before allocating a workspace, so that should help limit
> > the number of workspaces in circulation.
> 
> We'd need to observe that in practice before doing refinements, simpler
> logic is better for the start.
> 
> There's some penalty caused by the allocation if there are no workspaces
> at all, as the amount of memory is quite large for kernel.
> This could stress the memory subsystem also because the memory has to be
> either contiguous or vmalloced. As the memory is released soon, all the
> work might need to be done again and again. So, more than one
> preallocated workspace could be good but the number of levels does not
> make it easy to choose which one.

That makes sense. I don't have an answer for how to balance the number
of workspaces, but am happy to iterate on this as we get more data.

If no one has any other comments on the series after another day or so I
can send v2 with the handful of things people have mentioned and the
monotonic memory requirement patch.

Thanks,
Dennis

      reply index

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-28 21:24 Dennis Zhou
2019-01-28 21:24 ` [PATCH 01/11] btrfs: add macros for compression type and level Dennis Zhou
2019-01-29  7:26   ` Nikolay Borisov
2019-01-29 17:57   ` Josef Bacik
2019-01-29 22:30   ` Omar Sandoval
2019-01-31 16:00   ` David Sterba
2019-01-31 16:17     ` Dennis Zhou
2019-01-28 21:24 ` [PATCH 02/11] btrfs: rename workspaces_list to workspace_manager Dennis Zhou
2019-01-29  7:27   ` Nikolay Borisov
2019-01-29 17:58   ` Josef Bacik
2019-01-28 21:24 ` [PATCH 03/11] btrfs: manage heuristic workspace as index 0 Dennis Zhou
2019-01-29  7:53   ` Nikolay Borisov
2019-01-29 22:43     ` Dennis Zhou
2019-01-29 18:02   ` Josef Bacik
2019-01-31 16:10   ` David Sterba
2019-01-28 21:24 ` [PATCH 04/11] btrfs: unify compression ops with workspace_manager Dennis Zhou
2019-01-29  7:54   ` Nikolay Borisov
2019-01-29 18:03   ` Josef Bacik
2019-01-28 21:24 ` [PATCH 05/11] btrfs: add helper methods for workspace manager init and cleanup Dennis Zhou
2019-01-29  7:58   ` Nikolay Borisov
2019-01-29 18:04   ` Josef Bacik
2019-01-28 21:24 ` [PATCH 06/11] btrfs: add compression interface in (get/put)_workspace() Dennis Zhou
2019-01-29 18:06   ` Josef Bacik
2019-01-28 21:24 ` [PATCH 07/11] btrfs: move to fn pointers for get/put workspaces Dennis Zhou
2019-01-29  8:22   ` Nikolay Borisov
2019-01-29 23:35     ` Dennis Zhou
2019-01-29 18:17   ` Josef Bacik
2019-01-29 18:44     ` Josef Bacik
2019-01-28 21:24 ` [PATCH 08/11] btrfs: plumb level through the compression interface Dennis Zhou
2019-01-29  8:08   ` Nikolay Borisov
2019-01-29 18:17   ` Josef Bacik
2019-01-28 21:24 ` [PATCH 09/11] btrfs: change set_level() to bound the level passed in Dennis Zhou
2019-01-29  8:14   ` Nikolay Borisov
2019-01-30 22:06     ` Dennis Zhou
2019-01-28 21:24 ` [PATCH 10/11] btrfs: zstd use the passed through level instead of default Dennis Zhou
2019-01-29  8:15   ` Nikolay Borisov
2019-01-28 21:24 ` [PATCH 11/11] btrfs: add zstd compression level support Dennis Zhou
2019-01-29  7:25   ` Nikolay Borisov
2019-01-29 22:50     ` Dennis Zhou
2019-01-31 18:10   ` David Sterba
2019-01-31 18:13   ` David Sterba
2019-01-29 17:18 ` [PATCH 00/11] " David Sterba
2019-01-29 21:12   ` Nick Terrell
2019-01-30 17:40   ` Dennis Zhou
2019-01-31 14:04     ` David Sterba
2019-01-31 15:56       ` Dennis Zhou [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190131155602.GA90850@dennisz-mbp \
    --to=dennis@kernel.org \
    --cc=clm@fb.com \
    --cc=dsterba@suse.com \
    --cc=dsterba@suse.cz \
    --cc=josef@toxicpanda.com \
    --cc=kernel-team@fb.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=osandov@osandov.com \
    --cc=terrelln@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox