| Message ID | 20190204202008.51652-1-dennis@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: add zstd compression level support | expand |
On Mon, Feb 04, 2019 at 03:19:56PM -0500, Dennis Zhou wrote: > Hi everyone, > > V2 had only a handful of changes outside of minor feedback. > 0001: > - use functions over macros > 0003: > - BTRFS_NR_WORKSPACE_MANAGERS is added instead of overriding > BTRFS_COMPRESS_TYPES > 0011 (new): > - address monotonic memory requirement for zstd workspaces > 0012: > - increase reclaim timer to 307s from 67s > - move from keeping track of time in ns to jiffies > - remove timer in cleanup code > - use min_t() instead of if statements in .set_level() > - add header text to describe how workspaces are managed > - nofs_flag type -> unsigned long to unsigned int Something is wrong, the patchset on top of 5.0-rc5 hangs in test btrfs/007, without a stacktrace. V1 was fine and I double checked that rc5 itself is fine.
On Tue, Feb 05, 2019 at 03:57:53PM +0100, David Sterba wrote: > On Mon, Feb 04, 2019 at 03:19:56PM -0500, Dennis Zhou wrote: > > Hi everyone, > > > > V2 had only a handful of changes outside of minor feedback. > > 0001: > > - use functions over macros > > 0003: > > - BTRFS_NR_WORKSPACE_MANAGERS is added instead of overriding > > BTRFS_COMPRESS_TYPES > > 0011 (new): > > - address monotonic memory requirement for zstd workspaces > > 0012: > > - increase reclaim timer to 307s from 67s > > - move from keeping track of time in ns to jiffies > > - remove timer in cleanup code > > - use min_t() instead of if statements in .set_level() > > - add header text to describe how workspaces are managed > > - nofs_flag type -> unsigned long to unsigned int > > Something is wrong, the patchset on top of 5.0-rc5 hangs in test > btrfs/007, without a stacktrace. V1 was fine and I double checked that > rc5 itself is fine. Hmmm, well that's awkward. I ran xfstests and that test passed on my machine. I'll figure out the delta and submit a v3. Thanks David! Thanks, Dennis
On Tue, Feb 05, 2019 at 11:03:02AM -0500, Dennis Zhou wrote: > On Tue, Feb 05, 2019 at 03:57:53PM +0100, David Sterba wrote: > > On Mon, Feb 04, 2019 at 03:19:56PM -0500, Dennis Zhou wrote: > > > Hi everyone, > > > > > > V2 had only a handful of changes outside of minor feedback. > > > 0001: > > > - use functions over macros > > > 0003: > > > - BTRFS_NR_WORKSPACE_MANAGERS is added instead of overriding > > > BTRFS_COMPRESS_TYPES > > > 0011 (new): > > > - address monotonic memory requirement for zstd workspaces > > > 0012: > > > - increase reclaim timer to 307s from 67s > > > - move from keeping track of time in ns to jiffies > > > - remove timer in cleanup code > > > - use min_t() instead of if statements in .set_level() > > > - add header text to describe how workspaces are managed > > > - nofs_flag type -> unsigned long to unsigned int > > > > Something is wrong, the patchset on top of 5.0-rc5 hangs in test > > btrfs/007, without a stacktrace. V1 was fine and I double checked that > > rc5 itself is fine. > > Hmmm, well that's awkward. I ran xfstests and that test passed on my > machine. I'll figure out the delta and submit a v3. Thanks David! It failed on a VM and 2 other physical machines, so it's not somethig related to the testing setup.
On Tue, Feb 05, 2019 at 05:27:34PM +0100, David Sterba wrote: > On Tue, Feb 05, 2019 at 11:03:02AM -0500, Dennis Zhou wrote: > > On Tue, Feb 05, 2019 at 03:57:53PM +0100, David Sterba wrote: > > > On Mon, Feb 04, 2019 at 03:19:56PM -0500, Dennis Zhou wrote: > > > > Hi everyone, > > > > > > > > V2 had only a handful of changes outside of minor feedback. > > > > 0001: > > > > - use functions over macros > > > > 0003: > > > > - BTRFS_NR_WORKSPACE_MANAGERS is added instead of overriding > > > > BTRFS_COMPRESS_TYPES > > > > 0011 (new): > > > > - address monotonic memory requirement for zstd workspaces > > > > 0012: > > > > - increase reclaim timer to 307s from 67s > > > > - move from keeping track of time in ns to jiffies > > > > - remove timer in cleanup code > > > > - use min_t() instead of if statements in .set_level() > > > > - add header text to describe how workspaces are managed > > > > - nofs_flag type -> unsigned long to unsigned int > > > > > > Something is wrong, the patchset on top of 5.0-rc5 hangs in test > > > btrfs/007, without a stacktrace. V1 was fine and I double checked that > > > rc5 itself is fine. > > > > Hmmm, well that's awkward. I ran xfstests and that test passed on my > > machine. I'll figure out the delta and submit a v3. Thanks David! > > It failed on a VM and 2 other physical machines, so it's not somethig > related to the testing setup. Oh, I wasn't alluding to that. I'm more that sure it's my fault somewhere along the line. Thanks, Dennis
On Tue, Feb 05, 2019 at 11:30:12AM -0500, Dennis Zhou wrote: > > > > Something is wrong, the patchset on top of 5.0-rc5 hangs in test > > > > btrfs/007, without a stacktrace. V1 was fine and I double checked that > > > > rc5 itself is fine. > > > > > > Hmmm, well that's awkward. I ran xfstests and that test passed on my > > > machine. I'll figure out the delta and submit a v3. Thanks David! > > > > It failed on a VM and 2 other physical machines, so it's not somethig > > related to the testing setup. > > Oh, I wasn't alluding to that. I'm more that sure it's my fault > somewhere along the line. Hang on, it's probably caused by fstests, as fssum is busy waiting somewhere, so it's not even kernel code. fstests a4235f29f780b15d9e87c0f2446a53b9e69c5e2d -- ok fstests 6d17c9076fe9d97d2411e5909e6b255e5e721fc3 (master) -- hangs
On Tue, Feb 05, 2019 at 05:51:13PM +0100, David Sterba wrote: > On Tue, Feb 05, 2019 at 11:30:12AM -0500, Dennis Zhou wrote: > > > > > Something is wrong, the patchset on top of 5.0-rc5 hangs in test > > > > > btrfs/007, without a stacktrace. V1 was fine and I double checked that > > > > > rc5 itself is fine. > > > > > > > > Hmmm, well that's awkward. I ran xfstests and that test passed on my > > > > machine. I'll figure out the delta and submit a v3. Thanks David! > > > > > > It failed on a VM and 2 other physical machines, so it's not somethig > > > related to the testing setup. > > > > Oh, I wasn't alluding to that. I'm more that sure it's my fault > > somewhere along the line. > > Hang on, it's probably caused by fstests, as fssum is busy waiting > somewhere, so it's not even kernel code. > > fstests a4235f29f780b15d9e87c0f2446a53b9e69c5e2d -- ok > fstests 6d17c9076fe9d97d2411e5909e6b255e5e721fc3 (master) -- hangs Git bisect points to commit 04b405fa1f8d0cacde489f3d1c8771dfe7a34dc0 Author: Zorro Lang <zlang@redhat.com> Date: Wed Jan 23 15:34:55 2019 +0800 common/dump: disable splice from FSSTRESS_AVOID
On Tue, Feb 05, 2019 at 06:07:49PM +0100, David Sterba wrote: > On Tue, Feb 05, 2019 at 05:51:13PM +0100, David Sterba wrote: > > On Tue, Feb 05, 2019 at 11:30:12AM -0500, Dennis Zhou wrote: > > > > > > Something is wrong, the patchset on top of 5.0-rc5 hangs in test > > > > > > btrfs/007, without a stacktrace. V1 was fine and I double checked that > > > > > > rc5 itself is fine. > > > > > > > > > > Hmmm, well that's awkward. I ran xfstests and that test passed on my > > > > > machine. I'll figure out the delta and submit a v3. Thanks David! > > > > > > > > It failed on a VM and 2 other physical machines, so it's not somethig > > > > related to the testing setup. > > > > > > Oh, I wasn't alluding to that. I'm more that sure it's my fault > > > somewhere along the line. > > > > Hang on, it's probably caused by fstests, as fssum is busy waiting > > somewhere, so it's not even kernel code. > > > > fstests a4235f29f780b15d9e87c0f2446a53b9e69c5e2d -- ok > > fstests 6d17c9076fe9d97d2411e5909e6b255e5e721fc3 (master) -- hangs > > Git bisect points to > > commit 04b405fa1f8d0cacde489f3d1c8771dfe7a34dc0 > Author: Zorro Lang <zlang@redhat.com> > Date: Wed Jan 23 15:34:55 2019 +0800 > > common/dump: disable splice from FSSTRESS_AVOID So with this commit reverted, test btrfs/007 does not hang anymore.
On Tue, Feb 05, 2019 at 07:27:49PM +0100, David Sterba wrote: > On Tue, Feb 05, 2019 at 06:07:49PM +0100, David Sterba wrote: > > On Tue, Feb 05, 2019 at 05:51:13PM +0100, David Sterba wrote: > > > On Tue, Feb 05, 2019 at 11:30:12AM -0500, Dennis Zhou wrote: > > > > > > > Something is wrong, the patchset on top of 5.0-rc5 hangs in test > > > > > > > btrfs/007, without a stacktrace. V1 was fine and I double checked that > > > > > > > rc5 itself is fine. > > > > > > > > > > > > Hmmm, well that's awkward. I ran xfstests and that test passed on my > > > > > > machine. I'll figure out the delta and submit a v3. Thanks David! > > > > > > > > > > It failed on a VM and 2 other physical machines, so it's not somethig > > > > > related to the testing setup. > > > > > > > > Oh, I wasn't alluding to that. I'm more that sure it's my fault > > > > somewhere along the line. > > > > > > Hang on, it's probably caused by fstests, as fssum is busy waiting > > > somewhere, so it's not even kernel code. > > > > > > fstests a4235f29f780b15d9e87c0f2446a53b9e69c5e2d -- ok > > > fstests 6d17c9076fe9d97d2411e5909e6b255e5e721fc3 (master) -- hangs > > > > Git bisect points to > > > > commit 04b405fa1f8d0cacde489f3d1c8771dfe7a34dc0 > > Author: Zorro Lang <zlang@redhat.com> > > Date: Wed Jan 23 15:34:55 2019 +0800 > > > > common/dump: disable splice from FSSTRESS_AVOID > > So with this commit reverted, test btrfs/007 does not hang anymore. Ok great! I'm going to add a v2 for 0012 to add taking the spin_lock just to be safe in cleanup. I should have that ready in a few minutes. Thanks, Dennis
On Tue, Feb 05, 2019 at 01:30:27PM -0500, Dennis Zhou wrote: > On Tue, Feb 05, 2019 at 07:27:49PM +0100, David Sterba wrote: > > On Tue, Feb 05, 2019 at 06:07:49PM +0100, David Sterba wrote: > > > On Tue, Feb 05, 2019 at 05:51:13PM +0100, David Sterba wrote: > > > > On Tue, Feb 05, 2019 at 11:30:12AM -0500, Dennis Zhou wrote: > > > > > > > > Something is wrong, the patchset on top of 5.0-rc5 hangs in test > > > > > > > > btrfs/007, without a stacktrace. V1 was fine and I double checked that > > > > > > > > rc5 itself is fine. > > > > > > > > > > > > > > Hmmm, well that's awkward. I ran xfstests and that test passed on my > > > > > > > machine. I'll figure out the delta and submit a v3. Thanks David! > > > > > > > > > > > > It failed on a VM and 2 other physical machines, so it's not somethig > > > > > > related to the testing setup. > > > > > > > > > > Oh, I wasn't alluding to that. I'm more that sure it's my fault > > > > > somewhere along the line. > > > > > > > > Hang on, it's probably caused by fstests, as fssum is busy waiting > > > > somewhere, so it's not even kernel code. > > > > > > > > fstests a4235f29f780b15d9e87c0f2446a53b9e69c5e2d -- ok > > > > fstests 6d17c9076fe9d97d2411e5909e6b255e5e721fc3 (master) -- hangs > > > > > > Git bisect points to > > > > > > commit 04b405fa1f8d0cacde489f3d1c8771dfe7a34dc0 > > > Author: Zorro Lang <zlang@redhat.com> > > > Date: Wed Jan 23 15:34:55 2019 +0800 > > > > > > common/dump: disable splice from FSSTRESS_AVOID > > > > So with this commit reverted, test btrfs/007 does not hang anymore. > > Ok great! I'm going to add a v2 for 0012 to add taking the spin_lock > just to be safe in cleanup. I should have that ready in a few minutes. > Before I run v3, are there any other concerns (pending the changes I made to address the mentioned make sense)? Thanks, Dennis
On Tue, Feb 05, 2019 at 03:48:48PM -0500, Dennis Zhou wrote: > > Ok great! I'm going to add a v2 for 0012 to add taking the spin_lock > > just to be safe in cleanup. I should have that ready in a few minutes. > > > > Before I run v3, are there any other concerns (pending the changes I > made to address the mentioned make sense)? You don't needs to do full v3 resend, either new version of the individual patches or incremental changes that can be squashed to the patch. I'm about to move the patchset from topic branch to misc-next so incrementals work better now as there aren't big updates expected.
On Wed, Feb 06, 2019 at 04:15:52PM +0100, David Sterba wrote: > On Tue, Feb 05, 2019 at 03:48:48PM -0500, Dennis Zhou wrote: > > > Ok great! I'm going to add a v2 for 0012 to add taking the spin_lock > > > just to be safe in cleanup. I should have that ready in a few minutes. > > > > > > > Before I run v3, are there any other concerns (pending the changes I > > made to address the mentioned make sense)? > > You don't needs to do full v3 resend, either new version of the > individual patches or incremental changes that can be squashed to the > patch. I'm about to move the patchset from topic branch to misc-next so > incrementals work better now as there aren't big updates expected. Ok great! I just sent a v2 of 0009 and v3 of 00012 which include the changes for warning on incorrect input and level out of bounds. I did forget to take out the "Re:" though.. whoops. Thanks, Dennis
On Mon, Feb 04, 2019 at 03:19:56PM -0500, Dennis Zhou wrote: > Dennis Zhou (12): > btrfs: add helpers for compression type and level > btrfs: rename workspaces_list to workspace_manager > btrfs: manage heuristic workspace as index 0 > btrfs: unify compression ops with workspace_manager > btrfs: add helper methods for workspace manager init and cleanup > btrfs: add compression interface in (get/put)_workspace() > btrfs: move to fn pointers for get/put workspaces > btrfs: plumb level through the compression interface > btrfs: change set_level() to bound the level passed in > btrfs: zstd use the passed through level instead of default > btrfs: make zstd memory requirements monotonic > btrfs: add zstd compression level support The patchset has been added to msic-next, scheduled for 5.1. I've left out the changes to warn on wrong level, so currently it falls back to the default. I don't want to make a last minute change of that sort, so this needs to go as a separate patch. I also did a last pass through all patches and made some trivial changes like reformatting comments or minor style updates, nothing functional. The inter-branch diff can be found at https://github.com/kdave/btrfs-devel/compare/ext/dzhou/compr-workspaces-v2..kdave:ext/dzhou/compr-workspaces The feature should be ready to use, though as mentioned before some fine tuning might be necessary to make the memory requirements more efficient. The code could be simplified or cleaned up now that there are 3 compressors, at this point stabilization fixes are preferred. Thanks.
On Thu, Feb 07, 2019 at 05:59:26PM +0100, David Sterba wrote: > On Mon, Feb 04, 2019 at 03:19:56PM -0500, Dennis Zhou wrote: > > Dennis Zhou (12): > > btrfs: add helpers for compression type and level > > btrfs: rename workspaces_list to workspace_manager > > btrfs: manage heuristic workspace as index 0 > > btrfs: unify compression ops with workspace_manager > > btrfs: add helper methods for workspace manager init and cleanup > > btrfs: add compression interface in (get/put)_workspace() > > btrfs: move to fn pointers for get/put workspaces > > btrfs: plumb level through the compression interface > > btrfs: change set_level() to bound the level passed in > > btrfs: zstd use the passed through level instead of default > > btrfs: make zstd memory requirements monotonic > > btrfs: add zstd compression level support > > The patchset has been added to msic-next, scheduled for 5.1. I've left > out the changes to warn on wrong level, so currently it falls back to > the default. I don't want to make a last minute change of that sort, so > this needs to go as a separate patch. Regarding the bug report from Dan, do you want me to send you a patch for it on top of the topic branch or do you mind fixing the missing: unsigned int level = 0; > > I also did a last pass through all patches and made some trivial changes > like reformatting comments or minor style updates, nothing functional. > The inter-branch diff can be found at > https://github.com/kdave/btrfs-devel/compare/ext/dzhou/compr-workspaces-v2..kdave:ext/dzhou/compr-workspaces > > The feature should be ready to use, though as mentioned before some fine > tuning might be necessary to make the memory requirements more > efficient. The code could be simplified or cleaned up now that there are > 3 compressors, at this point stabilization fixes are preferred. > > Thanks. Thanks David! Dennis
On Thu, Feb 07, 2019 at 12:36:54PM -0500, Dennis Zhou wrote: > On Thu, Feb 07, 2019 at 05:59:26PM +0100, David Sterba wrote: > > On Mon, Feb 04, 2019 at 03:19:56PM -0500, Dennis Zhou wrote: > > > Dennis Zhou (12): > > > btrfs: add helpers for compression type and level > > > btrfs: rename workspaces_list to workspace_manager > > > btrfs: manage heuristic workspace as index 0 > > > btrfs: unify compression ops with workspace_manager > > > btrfs: add helper methods for workspace manager init and cleanup > > > btrfs: add compression interface in (get/put)_workspace() > > > btrfs: move to fn pointers for get/put workspaces > > > btrfs: plumb level through the compression interface > > > btrfs: change set_level() to bound the level passed in > > > btrfs: zstd use the passed through level instead of default > > > btrfs: make zstd memory requirements monotonic > > > btrfs: add zstd compression level support > > > > The patchset has been added to msic-next, scheduled for 5.1. I've left > > out the changes to warn on wrong level, so currently it falls back to > > the default. I don't want to make a last minute change of that sort, so > > this needs to go as a separate patch. > > Regarding the bug report from Dan, do you want me to send you a patch > for it on top of the topic branch or do you mind fixing the missing: > > unsigned int level = 0; It's been fixed in the pushed branch.