Re: [PATCH] ATA over Ethernet driver for 2.6.9

[Ed L Cashin <ecashin@coraid.com>]

Arjan van de Ven <arjan@fenrus.demon.nl> writes:

[helpful suggestions]
...
>> +struct Aoedev {
>> +	Aoedev *next;
>> +	uchar addr[6];		/* remote mac addr */
>> +	ushort flags;
>> +	ulong sysminor;
>> +	ulong aoemajor;
>> +	ulong aoeminor;
>
> sounds like the wrong type, why not use dev_t ?

These are ATA over Ethernet major and minor addresses, not device node
major and minor numbers.

>> +	ulong nopen;		/* user count */
>
> why do you need this ?
>
>> +static int
>> +aoeblk_release(struct inode *inode, struct file *filp)
>> +{
>> +	Aoedev *d;
>> +	ulong flags;
>> +
>> +	d = (Aoedev *) inode->i_bdev->bd_disk->private_data;
>> +
>> +	spin_lock_irqsave(&d->lock, flags);
>> +	if (--d->nopen == 0)
>
> eh why not just a ->release function instead that uses the blocklayer
> refcounting instead of doing your own ?

Do you just mean we should use inode->i_bdev->bd_openers instead of
having d->nopen?

>> +int
>> +aoeblk_make_request(request_queue_t *q, struct bio *bio)
>> +{
>> +	Aoedev *d;
>> +	Buf *buf;
>> +	struct sk_buff *sl;
>> +	ulong flags;
>> +
>> +	blk_queue_bounce(q, &bio);
>> +
>> +	buf = kallocz(sizeof *buf, GFP_KERNEL);
>
> this is deadlocky; you HAVE to use a mempool for allocations here!

OK.  Thanks for pointing that out.

-- 
  Ed L Cashin <ecashin@coraid.com>