Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
From: Dennis Zhou <dennis@kernel.org>
To: Nikolay Borisov <nborisov@suse.com>
Cc: 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 07/11] btrfs: move to fn pointers for get/put workspaces
Date: Tue, 29 Jan 2019 18:35:09 -0500
Message-ID: <20190129233509.GC87266@dennisz-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <6f9b4be7-0ce8-bd62-c90d-f3f537528b7a@suse.com>

Hi Nikolay,

On Tue, Jan 29, 2019 at 10:22:34AM +0200, Nikolay Borisov wrote:
> 
> 
> On 28.01.19 г. 23:24 ч., Dennis Zhou wrote:
> > The previous patch added generic helpers for get_workspace() and
> > put_workspace(). Now, we can migrate ownership of the workspace_manager
> > to be in the compression type code as the compression code itself
> > doesn't care beyond being able to get a workspace. The init/cleanup
> > and get/put methods are abstracted so each compression algorithm can
> > decide how they want to manage their workspaces.
> > 
> > Signed-off-by: Dennis Zhou <dennis@kernel.org>

Thanks for reviewing my patches so promptly!

> 
> TBH I can't really see the value in this patch. IMO it doesn't make the
> code more readable, on the contrary, you create algorithm-specific
> wrappers over the generic function, where the sole specialization is in
> the arguments passed to the generic functions. You introduce 4 more
> function pointers and this affects performance negatively (albeit can't
> say to what extent) due to spectre mitigations (retpolines).
> 

I'll try and address the need for function pointers below, but of the 4,
only 2 are called often as 2 are for init and clenaup.

For the cost of function pointers, I tested with retpoline on, with zlib
both via function pointers and hard coded using the Silesia corpus test
in the cover letter. It didn't show up in perf and the runtimes were
very close to each other with the function pointers actually performing
marginally better. I imagine the actual compression is heavy enough that
it greatly outweighs the cost of a function pointer.

> I also read the follow up patches with the hopes of seeing how the code
> becomes cleaner to no avail. At this point I'm really not in favor of
> this particular patch.

So I guess to recap the whole idea. The original implementation forced
everyone to use workspaces_list. This is a generic implementation that
only allows the management of workspaces by a lru list.

Zstd compression levels introduces the requirement of being able to
distinguish workspaces. This is not possible with the original
workspaces_list implementation.  If I modify this, it becomes rather
cumbersome adding variables to the generic workspaces_list that will
only be used by a particular compression types. It also fractures the
code having find_workspace() and free_workspace() deal with changing
code paths based on the compression type.

I think a big strength of this series is that we no longer rely on using
paired arrays and passing around compression type. Once we are working
within a compression type, it is not possible to accidentally cross
boundaries due to an off by one error. Each compression type is siloed
with workspace management code being up to the compression type code and
not the core compression code.

This series works to migrate to a two-level API with get_workspace() and
put_workspace() being the interface that the core compression code
interacts with each compression type. The core compression code does not
and should not care beyond the fact that it is getting a workspace. So,
get_workspace() and put_workspace() deal with managing the workspaces
themselves. Underneath this is alloc_workspace() and free_workspace()
which deal only with creating and deleting a workspace.  I think the
division of labor here is neat and allows for code sharing.

Prior to this, no one really needed to do anything smarter with
workspace management and therefore could share a single implementation.
Now, zstd has to do something a little smarter based on the level
requested and has the ability to use higher level workspaces should they
be available. The division of the API let's zstd do just that. It hides
all of the manager complexity away from the core compression code.

I apologize if I'm only restating what I've already said. If the
reasoning is still unclear, I can try and be more specific to those
unaddressed areas.

Thanks,
Dennis

  reply index

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-28 21:24 [PATCH 00/11] btrfs: add zstd compression level support 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 [this message]
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

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=20190129233509.GC87266@dennisz-mbp.dhcp.thefacebook.com \
    --to=dennis@kernel.org \
    --cc=clm@fb.com \
    --cc=dsterba@suse.com \
    --cc=josef@toxicpanda.com \
    --cc=kernel-team@fb.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nborisov@suse.com \
    --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