All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alasdair G Kergon <agk@redhat.com>
To: Ryo Tsuruta <ryov@valinux.co.jp>
Cc: dm-devel@redhat.com
Subject: Re: [PATCH 1/2] dm-ioband: I/O bandwidth controller v1.10.0: Source code and patch
Date: Tue, 20 Jan 2009 15:53:34 +0000	[thread overview]
Message-ID: <20090120155334.GH9859@agk.fab.redhat.com> (raw)
In-Reply-To: <20090120.141114.226778416.ryov@valinux.co.jp>

So, what needs to be reviewed?


1. General style/layout/naming cleanup.
- It's pretty good compared to a lot of patches that get sent, but there are
still a few things we can improve.

Lindent is throwing up some stuff (but remember it doesn't get things
perfect so don't make all the changes it recommends).
Remove double blank lines, plenty of unnecessary braces, "unsigned int" ->
"unsigned".  
Review the names - should a few more things get a DM_ or dm_ prefix?
Are all the names consistent and as self-explanatory as they can reasonably be?
(e.g. a variable called 'new' - new what?)


2. A high-level review.
Review the documentation and what the code does and how it does it.
- Does it make sense to add this to the kernel in this way?


3. A device-mapper review.
- Does the code fit into device-mapper correctly, using all the dm interfaces properly?


4. A detailed code review.
- Checking for correctness, fixing typos, improving messages, looking for ways
  to improve the code structure etc.


So, it needs volunteers on this list to take on aspects of that and lend their
support to the patch.

I consider items 1, 2 and 3 as blockers that must be completed satisfactorily before
I can push it upstream.  (Items 1 and 3 should be quick; 2 may take a bit
longer.)  Item 4 is of course open-ended so does not have to be finished before
it can be included.

Alasdair
-- 
agk@redhat.com

  parent reply	other threads:[~2009-01-20 15:53 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-01-20  5:10 [PATCH 0/2] dm-ioband: I/O bandwidth controller v1.10.0: Introduction Ryo Tsuruta
2009-01-20  5:11 ` [PATCH 1/2] dm-ioband: I/O bandwidth controller v1.10.0: Source code and patch Ryo Tsuruta
2009-01-20  5:12   ` [PATCH 2/2] dm-ioband: I/O bandwidth controller v1.10.0: Document Ryo Tsuruta
2009-01-20 14:52   ` [PATCH 1/2] dm-ioband: I/O bandwidth controller v1.10.0: Source code and patch Alasdair G Kergon
2009-01-21 13:03     ` Ryo Tsuruta
2009-01-21 17:18       ` Alasdair G Kergon
2009-01-22 12:05         ` Ryo Tsuruta
2009-02-04  5:07           ` Ryo Tsuruta
2009-01-20 15:19   ` Alasdair G Kergon
2009-01-20 15:53   ` Alasdair G Kergon [this message]
     [not found]     ` <20090120155334.GH9859-swAlYijrCMMf7BdofF/totBPR1lH4CV8@public.gmane.org>
2009-01-22 16:12       ` [dm-devel] " Vivek Goyal
2009-01-22 16:12         ` Vivek Goyal
     [not found]         ` <20090122161218.GA28795-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2009-01-23 10:14           ` Ryo Tsuruta
2009-01-23 10:14         ` Ryo Tsuruta
2009-01-26 16:29           ` Vivek Goyal
     [not found]             ` <20090126162951.GI31802-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2009-01-29  3:36               ` 2-Level IO scheduling (Re: [dm-devel] [PATCH 1/2] dm-ioband: I/O bandwidth controller v1.10.0: Source code and patch) Ryo Tsuruta
2009-01-29  3:39               ` Hierarchical grouping facility for IO controller " Ryo Tsuruta
2009-01-29  3:41               ` Implementation of dm-ioband as a dm-driver " Ryo Tsuruta
2009-01-29  3:36             ` 2-Level IO scheduling " Ryo Tsuruta
2009-01-29  3:39             ` Hierarchical grouping facility for IO controller " Ryo Tsuruta
2009-01-29  3:41             ` Implementation of dm-ioband as a dm-driver " Ryo Tsuruta
     [not found]           ` <20090123.191404.39168431.ryov-jCdQPDEk3idL9jVzuh4AOg@public.gmane.org>
2009-01-26 16:29             ` [dm-devel] [PATCH 1/2] dm-ioband: I/O bandwidth controller v1.10.0: Source code and patch Vivek Goyal
2009-01-20 15:04 ` [PATCH 0/2] dm-ioband: I/O bandwidth controller v1.10.0: Introduction Alasdair G Kergon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20090120155334.GH9859@agk.fab.redhat.com \
    --to=agk@redhat.com \
    --cc=dm-devel@redhat.com \
    --cc=ryov@valinux.co.jp \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.