Re: [RFC][PATCH] nbd driver for 2.5.72

[Lou Langholtz]

[e-mail] wrote:

>On Fri, Jun 20, 2003 at 11:43:39PM -0600, Lou Langholtz wrote:
>
>In addition to comments from Andrew:
>. . .
>  
>
>>+static nbd_device_t nbd_devs[MAX_NBD];
>>+static struct request_queue nbd_queue[MAX_NBD];
>>+static spinlock_t nbd_lock[MAX_NBD];
>>    
>>
>
>Why not put these into nbd_device?
>
I'd considered that and I'm reconsidering it again now. Not convinced 
which way to go... Putting something as large as struct request_queue 
within the nbd_device seems unbalanced somehow. Then again, until 2.5 
the request_queue was typically shared by multiple devices of the same 
MAJOR so part of the way the code is has to do with the legacy code. 
Like the nbd_lock spinlock array and the struct request_queue queue_lock 
field. Along the lines you're pushing for, why not have struct 
requests_queue's queue_lock field then be the spinlock itself instead of 
just being a pointer to a spinlock???

>>+static uint32_t request_magic;
>>    
>>
>
>???  htonl(NBD_REQUEST_MAGIC) is perfectly OK in the place where you
>use it and more likely than not will give better code.
>
>  
>
>>+static uint32_t reply_magic;
>>    
>>
>
>Ditto.
>
What's wrong with having an explicit cache of this value that we can 
rest assured doesn't in the worst case get compiled into multiple calls 
to the htonl code?? Possible waste of one 4 byte memory location in the 
worst compiler case or is there another problem?

>. . .
>  
>
>>+static void __init init_nbd_dev(nbd_device_t *dev)
>>+{
>>+#ifdef PARANOIA
>>+	dev->magic = LO_MAGIC;
>>+#endif
>>+	atomic_set(&dev->refcnt, 0);
>>+	dev->flags = 0;
>>+	spin_lock_init(&dev->lock);
>>+	dev->harderror = 0;
>>+	nbd_qsys_init(&dev->tx_queue);
>>+	nbd_qsys_init(&dev->rx_queue);
>>+	dev->file = NULL;
>>+	dev->sock = NULL;
>>+	memset(&dev->sin, 0, sizeof(dev->sin));
>>+	dev->errcnt = 0;
>>+	dev->lasterr = ENOTCONN;
>>+	dev->closed = (RCV_SHUTDOWN|SEND_SHUTDOWN);
>>+	dev->ss_thread.task = NULL;
>>+	dev->tx_thread.task = NULL;
>>+	dev->rx_thread.task = NULL;
>>+	atomic_set(&dev->num_io_threads, 0);
>>    
>>
>
>*Ugh*.  These suckers are already zero-filled.
>  
>
Yeah. I'm a bit to attracted perhaps to zero filling things explicitly 
in my coding style. Sigh.

>>+	requests_out = 0;
>>+	qhandler_loops = 0;
>>    
>>
>
>Static variables that do not have explicit initializer are initialized with 0.
>  
>
Ditto.

>>+/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
>>+ *
>>+ * End of definitions shared with user space.
>>+ * From here on out, these definitions are only for kernel (driver).
>>+ */
>>+
>>+#ifdef MAJOR_NR
>>    
>>
>
>a) that's ifdef __KERNEL__
>b) use separate headers.
>  
>
Historically, some code used MAJOR_NR this way (like the original nbd 
driver unfortunately), and I didn't change that. Seperating the header 
was something I'd thought about but in the end felt that just changing 
two existing files was bad enough without going off and adding a third 
or more files. It's worth reconsidering though also. Thanks!