Re: [PATCH 1/2] dm-kcopyd: introduce per-module throttle structure

[Joe Thornber <ejt@redhat.com>]

On Tue, May 31, 2011 at 06:03:39PM -0400, Mikulas Patocka wrote:
> Hi
> 
> Here I'm sending new kcopyd throttling patches that allow the throttle to 
> be set per module (i.e. one throttle for mirror and the other for 
> snapshots).

I'm sorry Mikulas, but these patches still really worry me.

i) Your throttling is time based, rather than throughput.

ii) No account is taken of the source and destination devices.  If I
have an idle mirror, I would like the resyncing of a new leg to go at
100% speed.  Other mirrors in the system may be under load so should
resync in a more sympathetic manner.  Should we be using the
congestion functions to see if the source or destination are too busy?
 
iii) calling msleep in kernel code really makes me shudder, especially
when you call it with a fixed, small duration and then loop.  Can't
you work out how long you should sleep for?  Why not defer the worker
thread until this time? (i.e. use queue_delayed_work()).

iv) you haven't explained how the sys admin works out the correct
throttle value.  If we write this down then maybe we can think about
automating it.

- Joe