Messages in this thread | | | Date | Mon, 17 Mar 2014 16:00:40 -0400 | From | Mike Snitzer <> | Subject | Re: [patch v3]DM: dm-insitu-comp: a compressed DM target for SSD |
| |
On Mon, Mar 17 2014 at 5:56am -0400, Shaohua Li <shli@kernel.org> wrote:
> On Fri, Mar 14, 2014 at 06:44:45PM -0400, Mike Snitzer wrote: > > On Fri, Mar 14 2014 at 5:40am -0400, > > Shaohua Li <shli@kernel.org> wrote: > > > > > On Mon, Mar 10, 2014 at 09:52:56AM -0400, Mike Snitzer wrote: > > > > On Fri, Mar 07 2014 at 2:57am -0500, > > > > Shaohua Li <shli@kernel.org> wrote: > > > > > > > > > ping! > > > > > > > > Hi, > > > > > > > > I intend to get dm-insitu-comp reviewed for 3.15. Sorry I haven't > > > > gotten back with you before now, been busy tending to 3.14-rc issues. > > > > > > > > I took a quick first pass over your code a couple weeks ago. Looks to > > > > be in great shape relative to coding conventions and the more DM > > > > specific conventions. Clearly demonstrates you have a good command of > > > > DM concepts and quirks. > > > > Think I need to eat my words from above at least partially. Given you > > haven't implemented any of the target suspend or resume hooks this > > target will _not_ work properly across suspend + resume cycles that all > > DM targets must support. > > > > But we can obviously work through it with urgency for 3.15. > > > > I've pulled your v3 patch into git and have overlayed edits from my > > first pass. Lots of funky wrapping to conform to 80 columns. But > > whitespace aside, I've added FIXME:s in the relevant files. If you work > > on any of these FIXMEs please send follow-up patches so that we don't > > step on each others' toes. > > > > Please see the 'for-3.15-insitu-comp' branch of this git repo: > > git://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git > > Thanks for your to look at it. I fixed them against your tree. Please check below patch.
I folded your changes in, and then committed a patch ontop that cleans some code up. But added 2 FIXMEs that still speak to pretty fundamental problems with the architecture of the dm-insitu-comp target, see: https://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=for-3.15-insitu-comp&id=8565ab6b04837591d03c94851c2f9f9162ce12f4
Unfortunately the single insitu_comp_wq workqueue that all insitu-comp targets are to share isn't a workable solution. Each target needs to have resource isolation from other targets (imagine insitu-comp used for multiple SSDs). This is important for suspend too because you'll need to flush/stop the workqueue.
You introduced a state machine for tracking suspending, suspended, resumed. This really isn't necessary. During suspend you need to flush_workqueue(). On resume you shouldn't need to do anything special.
As I noted in the commit, the thin and cache targets can serve as references for how you can manage the workqueue across suspend/resume and the lifetime of these workqueues relative to .ctr and .dtr.
| |