[dm-devel] Re: [PATCH 2/3] dm-mpath: add queue-length oriented dynamic load balancer

[Kiyoshi Ueda]

Hi Alasdair,

Thank you for the review and comments.
I'm totally agree with your comments.

I found that you have already updated the patch in your editing tree, thanks.
I attached patches for remaining parts of your comments.

Please review and apply.


On 05/09/2009 09:31 AM +0900, Alasdair G Kergon wrote:
> On Fri, Apr 24, 2009 at 05:07:26PM +0900, Kiyoshi Ueda wrote:
>> +	unsigned int		repeat_count;
> 
> Just 'unsigned repeat_count'

OK. (You have already fixed this.)
 

>> +	struct selector *s = (struct selector *) ps->context;
> 
> Drop the cast when it's from void.
> 
> 	struct selector *s = ps->context;

OK. (You have already fixed this.)

 
>> +static int ql_status(struct path_selector *ps, struct dm_path *path,
>> +		     status_type_t type, char *result, unsigned int maxlen)
> 
> unsigned maxlen

OK, fixed in rqdm-dlb-02-queue-length-dlb-type-fixes.patch


> (We're doing things like this in all new patches, but only changing existing
> code when it's touched for some other reason.)

OK, I followed the style of dm-round-robin and I understand your
preference now.
I'll also check the request-based dm patch-set, too, when I update
and repost it.


>> +	int sz = 0;
> 
> signed or unsigned sz?
> (It's already used inconsistently, I know - but is unsigned better here?)

Yes, I think unsigned is better. (You have already fixed this.)

 
>> +		case STATUSTYPE_INFO:
>> +			DMEMIT("%u ", atomic_read(&pi->qlen));
> 
> Signed or unsigned?

qlen should be always >= 0, but atomic_t is defined as signed.
So I use signed here.
Fixed in rqdm-dlb-02-queue-length-dlb-type-fixes.patch


>> +	/* Parse the arguments */
> 
> Please document parameters and selection algorithm in Documentation dir and
> maybe in inline comments too - it's not immediately obvious exactly how it
> behaves.
> 
>> +static struct dm_path *ql_select_path(struct path_selector *ps,
>> +				      unsigned *repeat_count, size_t nr_bytes)

OK, added comments/documentaion in
rqdm-dlb-02-queue-length-dlb-document.patch.

Thanks,
Kiyoshi Ueda
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: rqdm-dlb-02-queue-length-dlb-type-fixes.patch
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20090514/097acb28/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: rqdm-dlb-02-queue-length-dlb-document.patch
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20090514/097acb28/attachment-0001.ksh>