lkml.org 
[lkml]   [2013]   [Sep]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: staging: Add dm-writeboost
On Sun, Sep 01 2013 at  7:10am -0400,
Akira Hayakawa <ruby.wktk@gmail.com> wrote:

> This patch introduces dm-writeboost to staging tree.
>
> dm-writeboost is a log-structured caching software.
> It batches in-coming random-writes to a big sequential write
> to a cache device.
>
> Unlike other block caching softwares like dm-cache and bcache,
> dm-writeboost focuses on bursty writes.
> Since the implementation is optimized on writes,
> the benchmark using fio indicates that
> it performs 259MB/s random writes with
> SSD with 266MB/s sequential write throughput
> which is only 3% loss.
>
> Furthermore,
> because it uses SSD cache device sequentially,
> the lifetime of the device is maximized.
>
> The merit of putting this software in staging tree is
> to make it more possible to get feedback from users
> and thus polish the code.
>
> Signed-off-by: Akira Hayakawa <ruby.wktk@gmail.com>
> ---
> MAINTAINERS | 7 +
> drivers/staging/Kconfig | 2 +
> drivers/staging/Makefile | 1 +
> drivers/staging/dm-writeboost/Kconfig | 8 +
> drivers/staging/dm-writeboost/Makefile | 1 +
> drivers/staging/dm-writeboost/TODO | 11 +
> drivers/staging/dm-writeboost/dm-writeboost.c | 3445 +++++++++++++++++++++++
> drivers/staging/dm-writeboost/dm-writeboost.txt | 133 +
> 8 files changed, 3608 insertions(+)
> create mode 100644 drivers/staging/dm-writeboost/Kconfig
> create mode 100644 drivers/staging/dm-writeboost/Makefile
> create mode 100644 drivers/staging/dm-writeboost/TODO
> create mode 100644 drivers/staging/dm-writeboost/dm-writeboost.c
> create mode 100644 drivers/staging/dm-writeboost/dm-writeboost.txt

Hi Akira,

Sorry for not getting back to you sooner. I'll make an effort to be
more responsive from now on.

Here is a list of things I noticed during the first _partial_ pass in
reviewing the code:

- the various typedefs aren't needed (e.g. device_id, cache_id,
cache_nr)

- variable names need to be a bit more verbose (arr => array)

- really not convinced we need WB{ERR,WARN,INFO} -- may have been useful
for early development but production code shouldn't be emitting
messages with line numbers

- all the code in one file is too cumbersome; would like to see it split
into multiple files.. not clear on what that split would look like yet

- any chance the log-structured IO could be abstracted to a new class in
drivers/md/persistent-data/ ? At least factor out a library with the
interface that drives the IO.

- really dislike the use of an intermediate "writeboost-mgr" target to
administer the writeboost devices. There is no need for this. Just
have a normal DM target whose .ctr takes care of validation and
determines whether a device needs formatting, etc. Otherwise I cannot
see how you can properly stack DM devices on writeboost devices
(suspend+resume become tediously different)

- shouldn't need to worry about managing your own sysfs hierarchy;
when a dm-writeboost .ctr takes a reference on a backing or cache
device it will establish a proper hierarchy (see: dm_get_device). What
advantages are you seeing from having/managing this sysfs tree?

- I haven't had time to review the migration daemon post you made today;
but it concerns me that dm-writeboost ever required this extra service
for normal function. I'll take a closer look at what you're asking
and respond tomorrow.

But in short this code really isn't even suitable for inclusion via
staging. There are far too many things, on a fundamental interface
level, that we need to sort out.

Probably best for you to publish the dm-writeboost code a git repo on
github.com or the like. I just don't see what benefit there is to
putting code like this in staging. Users already need considerable
userspace tools and infrastructure will also be changing in the
near-term (e.g. the migration daemon).

Regards,
Mike


\
 
 \ /
  Last update: 2013-09-17 00:21    [W:0.772 / U:0.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site