On Wed, 2018-05-16 at 17:16 +0200, Dmitry Vyukov wrote:
> On Wed, May 16, 2018 at 4:56 PM, Bart Van Assche <Bart.VanAssche@xxxxxxx> wrote:
> > On Wed, 2018-05-16 at 22:05 +0900, Tetsuo Handa wrote:
> > > diff --git a/block/blk-core.c b/block/blk-core.c
> > > index 85909b4..59e2496 100644
> > > --- a/block/blk-core.c
> > > +++ b/block/blk-core.c
> > > @@ -951,10 +951,10 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
> > > smp_rmb();
> > >
> > > wait_event(q->mq_freeze_wq,
> > > - (atomic_read(&q->mq_freeze_depth) == 0 &&
> > > - (preempt || !blk_queue_preempt_only(q))) ||
> > > + atomic_read(&q->mq_freeze_depth) ||
> > > + (preempt || !blk_queue_preempt_only(q)) ||
> > > blk_queue_dying(q));
> > > - if (blk_queue_dying(q))
> > > + if (atomic_read(&q->mq_freeze_depth) || blk_queue_dying(q))
> > > return -ENODEV;
> > > }
> > > }
> >
> > That change looks wrong to me.
>
> Hi Bart,
>
> Why does it look wrong to you?
Because that change conflicts with the purpose of queue freezing and also because
that change would inject I/O errors in code paths that shouldn't inject I/O errors.
Please have a look at e.g. generic_make_request(). From the start of that function:
if (blk_queue_enter(q, flags) < 0) {
if (!blk_queue_dying(q) && (bio->bi_opf & REQ_NOWAIT))
bio_wouldblock_error(bio);
else
bio_io_error(bio);
return ret;
}
The above patch changes the behavior of blk_queue_enter() code from waiting while
q->mq_freeze_depth != 0 into returning -ENODEV while the request queue is frozen.
That will cause generic_make_request() to call bio_io_error(bio) while a request
queue is frozen if REQ_NOWAIT has not been set, which is the default behavior. So
any operation that freezes the queue temporarily, e.g. changing the queue depth,
concurrently with I/O processing can cause I/O to fail with -ENODEV. As you
probably know failure of write requests has very annoying consequences. It e.g.
causes filesystems to go into read-only mode. That's why I think that the above
change is completely wrong.
Bart.
