[dm-devel] Re: [PATCH 1/3]: region based notifications for dm-io

[Alasdair G Kergon]

On Tue, Jun 05, 2007 at 03:05:30PM +0200, Stefan Bader wrote:
> This patch adds the new interface to dm-io.

And also undoes some formatting, comments, variable name changes etc.
that I made when preparing earlier patches for upstream.  Please revert
those changes and also look at what I changed last time, and make
similar changes to this code.

Please avoid embedding enum definitions inside structs, for example.

-       int bi_rw;                      /* READ|WRITE - not READA */
+       int                     bi_rw;  /* READ|WRITE + bio flags */

Do we now support READA?

-       io_notify_fn fn;        /* Callback for asynchronous requests */
-       void *context;          /* Passed to callback */
+       io_notify_fn                    fn;      /* Callback for async req */
+       void *                          context; /* Passed to callback */

Please don't make format changes like that.

And things like these need resolving before inclusion:

+                       //spin_lock(&sio->lock);
+       /* FIXME: This might me unnecessary. */
+       /* FIXME: really need this ? */

+       unsigned                flags;          /* Future use...  */

Why do we need that *today* rather than adding it in future when we need
it?

This first patch is large and it would be nice to see it broken down
further.   For example, get any renaming / function moving out of the
way before making substantive code changes.

But before that, let's examine and understand the proposed interface
changes in dm-io.h.  The patch is unfortunately not easy to read in its
current form.  Adding the new interface in parallel with the old was
appropriate previously (change to nature of interface, and we needed to
retain the original interface in RHEL) but I don't think it works very
well this time.

Alasdair
-- 
agk at redhat.com