On Fri, Dec 08, 2017 at 01:08:37PM -0700, Jens Axboe wrote:
> On 12/08/2017 08:38 AM, Michele Ballabio wrote:
> > Hi,
> > kernels 4.13.*, 4.14.* 4.15-rc2 crash on occasion, especially
> > on x86-32 systems. To trigger the problem, run as root:
> >
> > while true
> > do
> > /sbin/udevadm trigger --type=subsystems --action=change
> > /sbin/udevadm trigger --type=devices --action=change
> > /sbin/udevadm settle --timeout=120
> > done
> >
> > (Thanks to Patrick Volkerding for the reproducer).
> >
> > Sometimes the kernel oopses immediately, sometimes a bit later (less than
> > five minutes).
> >
> > The bisection pointed to commit caa4b02476e31fc7933d2138062f7f355d3cd8f7
> > (blk-map: call blk_queue_bounce from blk_rq_append_bio). A revert
> > fixes the problem (tested on 4.13 and master).
>
> Thanks for your report - can you try the below patch? Totally
> untested...
>
>
> diff --git a/block/blk-map.c b/block/blk-map.c
> index b21f8e86f120..ad970719a1fc 100644
> --- a/block/blk-map.c
> +++ b/block/blk-map.c
> @@ -12,22 +12,22 @@
> #include "blk.h"
>
> /*
> - * Append a bio to a passthrough request. Only works can be merged into
> - * the request based on the driver constraints.
> + * Append a bio to a passthrough request. Only works if the bio can be merged
> + * into the request based on the driver constraints.
> */
> -int blk_rq_append_bio(struct request *rq, struct bio *bio)
> +int blk_rq_append_bio(struct request *rq, struct bio **bio)
> {
> - blk_queue_bounce(rq->q, &bio);
> + blk_queue_bounce(rq->q, bio);
>
> if (!rq->bio) {
> - blk_rq_bio_prep(rq->q, rq, bio);
> + blk_rq_bio_prep(rq->q, rq, *bio);
> } else {
> - if (!ll_back_merge_fn(rq->q, rq, bio))
> + if (!ll_back_merge_fn(rq->q, rq, *bio))
> return -EINVAL;
>
> - rq->biotail->bi_next = bio;
> - rq->biotail = bio;
> - rq->__data_len += bio->bi_iter.bi_size;
> + rq->biotail->bi_next = *bio;
> + rq->biotail = *bio;
> + rq->__data_len += (*bio)->bi_iter.bi_size;
> }
>
> return 0;
> @@ -73,8 +73,9 @@ static int __blk_rq_map_user_iov(struct request *rq,
> * We link the bounce buffer in and could have to traverse it
> * later so we have to get a ref to prevent it from being freed
> */
> - ret = blk_rq_append_bio(rq, bio);
> bio_get(bio);
> +
> + ret = blk_rq_append_bio(rq, &bio);
> if (ret) {
> bio_endio(bio);
> __blk_rq_unmap_user(orig_bio);
> @@ -236,7 +237,7 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
> if (do_copy)
> rq->rq_flags |= RQF_COPY_USER;
>
> - ret = blk_rq_append_bio(rq, bio);
> + ret = blk_rq_append_bio(rq, &bio);
> if (unlikely(ret)) {
> /* request is too big */
> bio_put(bio);
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index a4f28b7e4c65..e18877177f1b 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -1576,7 +1576,9 @@ static struct request *_make_request(struct request_queue *q, bool has_write,
> return req;
>
> for_each_bio(bio) {
> - ret = blk_rq_append_bio(req, bio);
> + struct bio *bounce_bio = bio;
> +
> + ret = blk_rq_append_bio(req, &bounce_bio);
> if (ret)
> return ERR_PTR(ret);
> }
> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
> index 7c69b4a9694d..0d99b242e82e 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -920,7 +920,7 @@ pscsi_map_sg(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
> " %d i: %d bio: %p, allocating another"
> " bio\n", bio->bi_vcnt, i, bio);
>
> - rc = blk_rq_append_bio(req, bio);
> + rc = blk_rq_append_bio(req, &bio);
> if (rc) {
> pr_err("pSCSI: failed to append bio\n");
> goto fail;
> @@ -938,7 +938,7 @@ pscsi_map_sg(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
> }
>
> if (bio) {
> - rc = blk_rq_append_bio(req, bio);
> + rc = blk_rq_append_bio(req, &bio);
> if (rc) {
> pr_err("pSCSI: failed to append bio\n");
> goto fail;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 8089ca17db9a..06b88d38f611 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -948,7 +948,7 @@ extern int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
> extern void blk_rq_unprep_clone(struct request *rq);
> extern blk_status_t blk_insert_cloned_request(struct request_queue *q,
> struct request *rq);
> -extern int blk_rq_append_bio(struct request *rq, struct bio *bio);
> +extern int blk_rq_append_bio(struct request *rq, struct bio **bio);
> extern void blk_delay_queue(struct request_queue *, unsigned long);
> extern void blk_queue_split(struct request_queue *, struct bio **);
> extern void blk_recount_segments(struct request_queue *, struct bio *);
Hi Jens,
I can reproduce this issue every time by forcing bounce on virtio-scsi
and enabling NEED_BOUNCE_POOL.
After applying your patch, there is still kernel oops[1].
I traced it a bit and found the following patch[2] makes a difference by
getting rid of copying iov_iter, but I guess this one is related with
the gcc(6.4.1 20170727).
Even though both your patch and the patch of 'bio_copy_to_iter: get rid
of copying iov_iter' are applied, there is still another oops[3].
[1] kernel oops after applying Jens's patch
https://pastebin.com/kn53fKY5
[2] patch of 'bio_copy_to_iter: get rid of copying iov_iter'
block/bio.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 76bb3dafffea..baa8e447eeb1 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1090,7 +1090,7 @@ static int bio_copy_from_iter(struct bio *bio, struct iov_iter *iter)
* Copy all pages from bio to iov_iter.
* Returns 0 on success, or error on failure.
*/
-static int bio_copy_to_iter(struct bio *bio, struct iov_iter iter)
+static int bio_copy_to_iter(struct bio *bio, struct iov_iter *iter)
{
int i;
struct bio_vec *bvec;
@@ -1101,9 +1101,9 @@ static int bio_copy_to_iter(struct bio *bio, struct iov_iter iter)
ret = copy_page_to_iter(bvec->bv_page,
bvec->bv_offset,
bvec->bv_len,
- &iter);
+ iter);
- if (!iov_iter_count(&iter))
+ if (!iov_iter_count(iter))
break;
if (ret < bvec->bv_len)
@@ -1144,7 +1144,7 @@ int bio_uncopy_user(struct bio *bio)
if (!current->mm)
ret = -EINTR;
else if (bio_data_dir(bio) == READ)
- ret = bio_copy_to_iter(bio, bmd->iter);
+ ret = bio_copy_to_iter(bio, &bmd->iter);
if (bmd->is_our_pages)
bio_free_pages(bio);
}
[3] kernel oops after applying Jens's patch and the attached patch of
'bio_copy_to_iter: get rid of copying iov_iter'
https://pastebin.com/3fMEhkWy
--
Ming
