Re: [PATCH v2] staging: writeboost: Add dm-writeboost

[Mike Snitzer <snitzer@redhat.com>]

On Wed, Dec 10 2014 at  6:42am -0500,
Akira Hayakawa <ruby.wktk@gmail.com> wrote:

> This patch adds dm-writeboost to staging tree.
> 
> dm-writeboost is a log-structured SSD-caching driver.
> It caches data in log-structured way on the cache device
> so that the performance is maximized.
> 
> The merit of putting this driver in staging tree is
> to make it possible to get more feedback from users
> and polish the codes.
> 
> Signed-off-by: Akira Hayakawa <ruby.wktk@gmail.com>

Joe Thornber Nacked this from going into staging based on his further
review of the code.  On the basis that you should _not_ be copying all
pages associated with each bio into an in-core buffer.  Have you
addressed that fundamental problem?

But taking a step back: Why do you think going into staging actually
helps you?  Anyone who is willing to test this code should be able to
apply a patch to their kernel.  Having to feed changes to Greg to
deliver updates to any early consumers of this _EXPERIMENTAL_ target
seems misplaced when you consider that Joe has detailed important
fixes, capabilties and tools that need addressing before anyone should
trust their production data to dm-writeboost.

I think you're completely missing that you are pushing _hard_ for this
target to go upstream before it is actually ready.  In doing so you're
so hung up on that "upstream" milestone that you cannot appreciate the
reluctance that Joe and I have given the quantity of code you're pushing
-- especially when coupled with the limited utility of dm-writeboost in
comparison with full featured upstream drivers like dm-cache and bcache.

As for this v2, you didn't detail what you changed (I can obviously
apply v1 and then v2 to see the difference but a brief summary would be
helpful in general when you revise a patch)

But one inline comment:

> diff --git a/drivers/staging/writeboost/TODO b/drivers/staging/writeboost/TODO
> new file mode 100644
> index 0000000..761a9fe
> --- /dev/null
> +++ b/drivers/staging/writeboost/TODO
> @@ -0,0 +1,52 @@
> +TODO:
> +
> +- Get the GitExtract test so it's performance is similar to raw spindle.

No, the whole point of a write cache is to improve performance.  You
should be trying to surpass the performance of the raw spindle.