| Message ID | 20190222195348.2728-1-dennis@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: zstd ensure reclaim timer is properly cleaned up | expand |
On Fri, Feb 22, 2019 at 02:53:48PM -0500, Dennis Zhou wrote: > The timer function, zstd_reclaim_timer_fn(), reschedules itself under > certain conditions. When cleaning up, take the lock and remove all > workspaces. This prevents the timer from rearming itself. Lastly, switch > to del_timer_sync() to ensure that the timer function can't trigger as > we're unloading. > > Signed-off-by: Dennis Zhou <dennis@kernel.org> Reviewed-by: David Sterba <dsterba@suse.com> > --- > v2: > - cleanup workspaces and then disable the timer > > fs/btrfs/zstd.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c > index 3e418a3aeb11..6b9e29d050f3 100644 > --- a/fs/btrfs/zstd.c > +++ b/fs/btrfs/zstd.c > @@ -195,8 +195,7 @@ static void zstd_cleanup_workspace_manager(void) > struct workspace *workspace; > int i; > > - del_timer(&wsm.timer); > - > + spin_lock(&wsm.lock); > for (i = 0; i < ZSTD_BTRFS_MAX_LEVEL; i++) { > while (!list_empty(&wsm.idle_ws[i])) { > workspace = container_of(wsm.idle_ws[i].next, > @@ -206,6 +205,9 @@ static void zstd_cleanup_workspace_manager(void) > wsm.ops->free_workspace(&workspace->list); I've noticed while reading the code, why do you use the indirect call here? The wsm.ops points to btrfs_zstd_compress so free_workspace is always zstd_free_workspace. The compiler is usually smart to replace such things by direct call if the type has not escaped, but this is not true for btrfs_compress_op so the indirect function call must be preserved.
On Wed, Feb 27, 2019 at 05:44:41PM +0100, David Sterba wrote: > On Fri, Feb 22, 2019 at 02:53:48PM -0500, Dennis Zhou wrote: > > The timer function, zstd_reclaim_timer_fn(), reschedules itself under > > certain conditions. When cleaning up, take the lock and remove all > > workspaces. This prevents the timer from rearming itself. Lastly, switch > > to del_timer_sync() to ensure that the timer function can't trigger as > > we're unloading. > > > > Signed-off-by: Dennis Zhou <dennis@kernel.org> > > Reviewed-by: David Sterba <dsterba@suse.com> > Thanks! > > --- > > v2: > > - cleanup workspaces and then disable the timer > > > > fs/btrfs/zstd.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c > > index 3e418a3aeb11..6b9e29d050f3 100644 > > --- a/fs/btrfs/zstd.c > > +++ b/fs/btrfs/zstd.c > > @@ -195,8 +195,7 @@ static void zstd_cleanup_workspace_manager(void) > > struct workspace *workspace; > > int i; > > > > - del_timer(&wsm.timer); > > - > > + spin_lock(&wsm.lock); > > for (i = 0; i < ZSTD_BTRFS_MAX_LEVEL; i++) { > > while (!list_empty(&wsm.idle_ws[i])) { > > workspace = container_of(wsm.idle_ws[i].next, > > @@ -206,6 +205,9 @@ static void zstd_cleanup_workspace_manager(void) > > wsm.ops->free_workspace(&workspace->list); > > I've noticed while reading the code, why do you use the indirect call > here? The wsm.ops points to btrfs_zstd_compress so free_workspace is > always zstd_free_workspace. > > The compiler is usually smart to replace such things by direct call if > the type has not escaped, but this is not true for btrfs_compress_op so > the indirect function call must be preserved. I don't have a strong reason to use the indirect call here. It was just to make it consistent for everyone to use the indirection. This at least is in the cleanup path, so I don't think performance is that important? But I don't feel strongly for or against calling zstd_free_workspace() directly. Thanks, Dennis
On Wed, Feb 27, 2019 at 01:29:16PM -0500, Dennis Zhou wrote: > > I've noticed while reading the code, why do you use the indirect call > > here? The wsm.ops points to btrfs_zstd_compress so free_workspace is > > always zstd_free_workspace. > > > > The compiler is usually smart to replace such things by direct call if > > the type has not escaped, but this is not true for btrfs_compress_op so > > the indirect function call must be preserved. > > I don't have a strong reason to use the indirect call here. It was just > to make it consistent for everyone to use the indirection. This at least > is in the cleanup path, so I don't think performance is that important? It's not just that, the timer uses it too and there are indirect calls of the alloc_workspace callback. The indirection is not used by lzo nor zlib code, so I don't see what 'everyone' you mean. In the generic compression code it makes sense, I see that. > But I don't feel strongly for or against calling zstd_free_workspace() > directly. I feel strongly about not using the indirection when not necessary :)
On Wed, Feb 27, 2019 at 07:36:50PM +0100, David Sterba wrote: > On Wed, Feb 27, 2019 at 01:29:16PM -0500, Dennis Zhou wrote: > > > I've noticed while reading the code, why do you use the indirect call > > > here? The wsm.ops points to btrfs_zstd_compress so free_workspace is > > > always zstd_free_workspace. > > > > > > The compiler is usually smart to replace such things by direct call if > > > the type has not escaped, but this is not true for btrfs_compress_op so > > > the indirect function call must be preserved. > > > > I don't have a strong reason to use the indirect call here. It was just > > to make it consistent for everyone to use the indirection. This at least > > is in the cleanup path, so I don't think performance is that important? > > It's not just that, the timer uses it too and there are indirect calls > of the alloc_workspace callback. The indirection is not used by lzo nor > zlib code, so I don't see what 'everyone' you mean. In the generic > compression code it makes sense, I see that. > > > But I don't feel strongly for or against calling zstd_free_workspace() > > directly. > > I feel strongly about not using the indirection when not necessary :) Great :). I sent you a patch just now to remove the indirection [1]. [1] https://lore.kernel.org/linux-btrfs/20190227212128.38491-1-dennis@kernel.org Thanks, Dennis
diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c index 3e418a3aeb11..6b9e29d050f3 100644 --- a/fs/btrfs/zstd.c +++ b/fs/btrfs/zstd.c @@ -195,8 +195,7 @@ static void zstd_cleanup_workspace_manager(void) struct workspace *workspace; int i; - del_timer(&wsm.timer); - + spin_lock(&wsm.lock); for (i = 0; i < ZSTD_BTRFS_MAX_LEVEL; i++) { while (!list_empty(&wsm.idle_ws[i])) { workspace = container_of(wsm.idle_ws[i].next, @@ -206,6 +205,9 @@ static void zstd_cleanup_workspace_manager(void) wsm.ops->free_workspace(&workspace->list); } } + spin_unlock(&wsm.lock); + + del_timer_sync(&wsm.timer); } /*
The timer function, zstd_reclaim_timer_fn(), reschedules itself under certain conditions. When cleaning up, take the lock and remove all workspaces. This prevents the timer from rearming itself. Lastly, switch to del_timer_sync() to ensure that the timer function can't trigger as we're unloading. Signed-off-by: Dennis Zhou <dennis@kernel.org> --- v2: - cleanup workspaces and then disable the timer fs/btrfs/zstd.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)