On Tue, 2017-10-03 at 09:19 +0900, Damien Le Moal wrote:
> On 10/3/17 08:44, Bart Van Assche wrote:
> > On Mon, 2017-10-02 at 16:15 +0900, Damien Le Moal wrote:
> > > static void deadline_wunlock_zone(struct deadline_data *dd,
> > > struct request *rq)
> > > {
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&dd->zone_lock, flags);
> > > +
> > > WARN_ON_ONCE(!test_and_clear_bit(blk_rq_zone_no(rq), dd->zones_wlock));
> > > deadline_clear_request_zone_wlock(rq);
> > > +
> > > + spin_unlock_irqrestore(&dd->zone_lock, flags);
> > > }
> >
> > Is a request removed from the sort and fifo lists before being dispatched? If so,
> > does that mean that obtaining zone_lock in the above function is not necessary?
>
> Yes, a request is removed from the sort tree and fifo list before
> dispatching. But the dd->zone_lock spinlock is not there to protect
> that, dd->lock protects the sort tree and fifo list. dd->zone_lock was
> added to prevent the completed_request() method from changing a zone
> lock state while deadline_fifo_requests() or deadline_next_request() are
> running. Ex:
>
> Imagine this case: write request A for a zone Z is being executed (it
> was dispatched) so Z is locked. Dispatch runs and inspects the next
> requests in sort order. Let say we have the sequential writes B, C, D, E
> queued for the same zone Z. First B is inspected and cannot be
> dispatched (zone locked). Inspection move on to C, but before the that,
> A completes and Z is unlocked. Then C will be OK to go as the zone is
> now unlocked. But it is the wrong choice as it will result in out of
> order write. B must be the next request dispatched after A.
>
> dd->zone_lock prevents this from happening. Without this spinlock, the
> bad example case above happens very easily.
Hello Damien,
Thanks for the detailed and really clear reply. I hope you do not mind that I
have a few further questions about this patch?
- Does the zone_lock spinlock have to be acquired by both deadline_wunlock_zone()
callers or only by the call from the request completion path?
- Why do both the mq-deadline and the sd driver each have their own instance of
the zones_wlock bitmap? Has it been considered to convert both bitmaps into a
single bitmap that is shared by both kernel components and that exists e.g. at
the request queue level?
Thanks,
Bart.
