On Thu, Dec 27 2007, Adrian McMenamin wrote: > > On Thu, 2007-12-27 at 17:18 +0900, Paul Mundt wrote: > > On Thu, Dec 27, 2007 at 01:26:47AM +0000, Adrian McMenamin wrote: > > > > > > + /* now seek to take the request spinlock > > > + * before handling ending the request */ > > > + spin_lock(&gdrom_lock); > > > + list_del_init(&req->queuelist); > > > + blk_requeue_request(gd.gdrom_rq, req); > > > + if (err) > > > + end_request(req, 0); > > > + else > > > + end_request(req, 1); > > > + } > > > + spin_unlock(&gdrom_lock); > > > + kfree(read_command); > > > +} > > > + > > This locking is all over the place. What is this lock supposed to be > > accomplishing? > > - > > I have to hold the lock to access the request queue. As the linked list > of deferred requests is under the control of code also protected by the > lock, it is also held to ensure manipulation of that list is serialised. > > The first step of the loop manipulates that linked list - so it is held > as we re-iterate over the loop. > > This is pretty much the way Jens recommended I do it.
I didn't recommend the last requeue bit, it looks like a work-around due to the way that end_request() works. The kerneldoc comment for that function also tells you NOT to use this in new code. Use end_dequeued_request() and get rid of the requeue, and streamline 'err' so you can just pass it directly in. The locking otherwise looks fine to me. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
