| Message ID | 20180614145510.8072-1-khalid.elmously@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [B,v2,1/1] block: do not use interruptible wait anywhere | expand |
Replying and removing the CC addresses that I spammed. Hopefully if someone were to reply to this thread they'd reply to this message to avoid spamming them again. On 2018-06-14 10:55:10 , Khalid Elmously wrote: > From: Alan Jenkins <alan.christopher.jenkins@gmail.com> > > BugLink: http://bugs.launchpad.net/bugs/1776887 > > When blk_queue_enter() waits for a queue to unfreeze, or unset the > PREEMPT_ONLY flag, do not allow it to be interrupted by a signal. > > The PREEMPT_ONLY flag was introduced later in commit 3a0a529971ec > ("block, scsi: Make SCSI quiesce and resume work reliably"). Note the SCSI > device is resumed asynchronously, i.e. after un-freezing userspace tasks. > > So that commit exposed the bug as a regression in v4.15. A mysterious > SIGBUS (or -EIO) sometimes happened during the time the device was being > resumed. Most frequently, there was no kernel log message, and we saw Xorg > or Xwayland killed by SIGBUS.[1] > > [1] E.g. https://bugzilla.redhat.com/show_bug.cgi?id=1553979 > > Without this fix, I get an IO error in this test: > > # dd if=/dev/sda of=/dev/null iflag=direct & \ > while killall -SIGUSR1 dd; do sleep 0.1; done & \ > echo mem > /sys/power/state ; \ > sleep 5; killall dd # stop after 5 seconds > > The interruptible wait was added to blk_queue_enter in > commit 3ef28e83ab15 ("block: generic request_queue reference counting"). > Before then, the interruptible wait was only in blk-mq, but I don't think > it could ever have been correct. > > Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: stable@vger.kernel.org > Signed-off-by: Alan Jenkins <alan.christopher.jenkins@gmail.com> > Signed-off-by: Jens Axboe <axboe@kernel.dk> > (cherry-picked from 1dc3039bc87ae7d19a990c3ee71cfd8a9068f428) > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> > > --- > block/blk-core.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index fc0666354af3..59c91e345eea 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -821,7 +821,6 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) > > while (true) { > bool success = false; > - int ret; > > rcu_read_lock(); > if (percpu_ref_tryget_live(&q->q_usage_counter)) { > @@ -853,14 +852,12 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) > */ > smp_rmb(); > > - ret = wait_event_interruptible(q->mq_freeze_wq, > - (atomic_read(&q->mq_freeze_depth) == 0 && > - (preempt || !blk_queue_preempt_only(q))) || > - blk_queue_dying(q)); > + wait_event(q->mq_freeze_wq, > + (atomic_read(&q->mq_freeze_depth) == 0 && > + (preempt || !blk_queue_preempt_only(q))) || > + blk_queue_dying(q)); > if (blk_queue_dying(q)) > return -ENODEV; > - if (ret) > - return ret; > } > } > > -- > 2.17.1 >
On 14.06.2018 16:55, Khalid Elmously wrote: > From: Alan Jenkins <alan.christopher.jenkins@gmail.com> > > BugLink: http://bugs.launchpad.net/bugs/1776887 > > When blk_queue_enter() waits for a queue to unfreeze, or unset the > PREEMPT_ONLY flag, do not allow it to be interrupted by a signal. > > The PREEMPT_ONLY flag was introduced later in commit 3a0a529971ec > ("block, scsi: Make SCSI quiesce and resume work reliably"). Note the SCSI > device is resumed asynchronously, i.e. after un-freezing userspace tasks. > > So that commit exposed the bug as a regression in v4.15. A mysterious > SIGBUS (or -EIO) sometimes happened during the time the device was being > resumed. Most frequently, there was no kernel log message, and we saw Xorg > or Xwayland killed by SIGBUS.[1] > > [1] E.g. https://bugzilla.redhat.com/show_bug.cgi?id=1553979 > > Without this fix, I get an IO error in this test: > > # dd if=/dev/sda of=/dev/null iflag=direct & \ > while killall -SIGUSR1 dd; do sleep 0.1; done & \ > echo mem > /sys/power/state ; \ > sleep 5; killall dd # stop after 5 seconds > > The interruptible wait was added to blk_queue_enter in > commit 3ef28e83ab15 ("block: generic request_queue reference counting"). > Before then, the interruptible wait was only in blk-mq, but I don't think > it could ever have been correct. > > Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: stable@vger.kernel.org > Signed-off-by: Alan Jenkins <alan.christopher.jenkins@gmail.com> > Signed-off-by: Jens Axboe <axboe@kernel.dk> > (cherry-picked from 1dc3039bc87ae7d19a990c3ee71cfd8a9068f428) > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > > --- Needs to have the SRU template text added to the description. > block/blk-core.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index fc0666354af3..59c91e345eea 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -821,7 +821,6 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) > > while (true) { > bool success = false; > - int ret; > > rcu_read_lock(); > if (percpu_ref_tryget_live(&q->q_usage_counter)) { > @@ -853,14 +852,12 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) > */ > smp_rmb(); > > - ret = wait_event_interruptible(q->mq_freeze_wq, > - (atomic_read(&q->mq_freeze_depth) == 0 && > - (preempt || !blk_queue_preempt_only(q))) || > - blk_queue_dying(q)); > + wait_event(q->mq_freeze_wq, > + (atomic_read(&q->mq_freeze_depth) == 0 && > + (preempt || !blk_queue_preempt_only(q))) || > + blk_queue_dying(q)); > if (blk_queue_dying(q)) > return -ENODEV; > - if (ret) > - return ret; > } > } > >
On 06/14/18 16:55, Khalid Elmously wrote: > From: Alan Jenkins <alan.christopher.jenkins@gmail.com> > > BugLink: http://bugs.launchpad.net/bugs/1776887 > > When blk_queue_enter() waits for a queue to unfreeze, or unset the > PREEMPT_ONLY flag, do not allow it to be interrupted by a signal. > > The PREEMPT_ONLY flag was introduced later in commit 3a0a529971ec > ("block, scsi: Make SCSI quiesce and resume work reliably"). Note the SCSI > device is resumed asynchronously, i.e. after un-freezing userspace tasks. > > So that commit exposed the bug as a regression in v4.15. A mysterious > SIGBUS (or -EIO) sometimes happened during the time the device was being > resumed. Most frequently, there was no kernel log message, and we saw Xorg > or Xwayland killed by SIGBUS.[1] > > [1] E.g. https://bugzilla.redhat.com/show_bug.cgi?id=1553979 > > Without this fix, I get an IO error in this test: > > # dd if=/dev/sda of=/dev/null iflag=direct & \ > while killall -SIGUSR1 dd; do sleep 0.1; done & \ > echo mem > /sys/power/state ; \ > sleep 5; killall dd # stop after 5 seconds > > The interruptible wait was added to blk_queue_enter in > commit 3ef28e83ab15 ("block: generic request_queue reference counting"). > Before then, the interruptible wait was only in blk-mq, but I don't think > it could ever have been correct. > > Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: stable@vger.kernel.org > Signed-off-by: Alan Jenkins <alan.christopher.jenkins@gmail.com> > Signed-off-by: Jens Axboe <axboe@kernel.dk> > (cherry-picked from 1dc3039bc87ae7d19a990c3ee71cfd8a9068f428) > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> > > --- > block/blk-core.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index fc0666354af3..59c91e345eea 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -821,7 +821,6 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) > > while (true) { > bool success = false; > - int ret; > > rcu_read_lock(); > if (percpu_ref_tryget_live(&q->q_usage_counter)) { > @@ -853,14 +852,12 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) > */ > smp_rmb(); > > - ret = wait_event_interruptible(q->mq_freeze_wq, > - (atomic_read(&q->mq_freeze_depth) == 0 && > - (preempt || !blk_queue_preempt_only(q))) || > - blk_queue_dying(q)); > + wait_event(q->mq_freeze_wq, > + (atomic_read(&q->mq_freeze_depth) == 0 && > + (preempt || !blk_queue_preempt_only(q))) || > + blk_queue_dying(q)); > if (blk_queue_dying(q)) > return -ENODEV; > - if (ret) > - return ret; > } > } > > This patch has been sent again by Joseph on Jun-29 [1]. In general I would NAK the second submission, but the other SRU request thread is already referenced on the bug report so it's easier to follow the discussion from the LP bug keeping that one alive. Thanks anyway, Khaled! [1] https://lists.ubuntu.com/archives/kernel-team/2018-June/093606.html
diff --git a/block/blk-core.c b/block/blk-core.c index fc0666354af3..59c91e345eea 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -821,7 +821,6 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) while (true) { bool success = false; - int ret; rcu_read_lock(); if (percpu_ref_tryget_live(&q->q_usage_counter)) { @@ -853,14 +852,12 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) */ smp_rmb(); - ret = wait_event_interruptible(q->mq_freeze_wq, - (atomic_read(&q->mq_freeze_depth) == 0 && - (preempt || !blk_queue_preempt_only(q))) || - blk_queue_dying(q)); + wait_event(q->mq_freeze_wq, + (atomic_read(&q->mq_freeze_depth) == 0 && + (preempt || !blk_queue_preempt_only(q))) || + blk_queue_dying(q)); if (blk_queue_dying(q)) return -ENODEV; - if (ret) - return ret; } }