| Message ID | 20230104160938.62636-2-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Don't allow REQ_NOWAIT for bio splitting | expand |
On Wed, Jan 04, 2023 at 09:09:37AM -0700, Jens Axboe wrote: > This can't happen right now, but in preparation for allowing > bio_split_to_limits() returning NULL if it ended the bio, check for it > in all the callers. > > Signed-off-by: Jens Axboe <axboe@kernel.dk> > --- > block/blk-merge.c | 4 +++- > block/blk-mq.c | 5 ++++- > drivers/block/drbd/drbd_req.c | 2 ++ > drivers/block/ps3vram.c | 2 ++ > drivers/md/dm.c | 2 ++ > drivers/md/md.c | 2 ++ > drivers/nvme/host/multipath.c | 2 ++ > drivers/s390/block/dcssblk.c | 2 ++ > 8 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/block/blk-merge.c b/block/blk-merge.c > index 35a8f75cc45d..071c5f8cf0cf 100644 > --- a/block/blk-merge.c > +++ b/block/blk-merge.c > @@ -358,11 +358,13 @@ struct bio *__bio_split_to_limits(struct bio *bio, > default: > split = bio_split_rw(bio, lim, nr_segs, bs, > get_max_io_size(bio, lim) << SECTOR_SHIFT); > + if (IS_ERR(split)) > + return NULL; Can we decide on either passing an ERR_PTR or NULL and do it through the whole stack? > diff --git a/drivers/block/drbd/drbd_req.c b/drivers/block/drbd/drbd_req.c > index eb14ec8ec04c..e36216d50753 100644 > --- a/drivers/block/drbd/drbd_req.c > +++ b/drivers/block/drbd/drbd_req.c > @@ -1607,6 +1607,8 @@ void drbd_submit_bio(struct bio *bio) > struct drbd_device *device = bio->bi_bdev->bd_disk->private_data; > > bio = bio_split_to_limits(bio); > + if (!bio) > + return; So for the callers in drivers, do we need thee checks for drivers that don't even support REQ_NOWAIT?
Per the question in the other thread: these are my comments to it. On Tue, Jan 10, 2023 at 01:20:26AM -0800, Christoph Hellwig wrote: > > split = bio_split_rw(bio, lim, nr_segs, bs, > > get_max_io_size(bio, lim) << SECTOR_SHIFT); > > + if (IS_ERR(split)) > > + return NULL; > > Can we decide on either passing an ERR_PTR or NULL and do it through > the whole stack? > > > diff --git a/drivers/block/drbd/drbd_req.c b/drivers/block/drbd/drbd_req.c > > index eb14ec8ec04c..e36216d50753 100644 > > --- a/drivers/block/drbd/drbd_req.c > > +++ b/drivers/block/drbd/drbd_req.c > > @@ -1607,6 +1607,8 @@ void drbd_submit_bio(struct bio *bio) > > struct drbd_device *device = bio->bi_bdev->bd_disk->private_data; > > > > bio = bio_split_to_limits(bio); > > + if (!bio) > > + return; > > So for the callers in drivers, do we need thee checks for drivers > that don't even support REQ_NOWAIT? ---end quoted text---
On 1/10/23 2:20 AM, Christoph Hellwig wrote: > On Wed, Jan 04, 2023 at 09:09:37AM -0700, Jens Axboe wrote: >> This can't happen right now, but in preparation for allowing >> bio_split_to_limits() returning NULL if it ended the bio, check for it >> in all the callers. >> >> Signed-off-by: Jens Axboe <axboe@kernel.dk> >> --- >> block/blk-merge.c | 4 +++- >> block/blk-mq.c | 5 ++++- >> drivers/block/drbd/drbd_req.c | 2 ++ >> drivers/block/ps3vram.c | 2 ++ >> drivers/md/dm.c | 2 ++ >> drivers/md/md.c | 2 ++ >> drivers/nvme/host/multipath.c | 2 ++ >> drivers/s390/block/dcssblk.c | 2 ++ >> 8 files changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/block/blk-merge.c b/block/blk-merge.c >> index 35a8f75cc45d..071c5f8cf0cf 100644 >> --- a/block/blk-merge.c >> +++ b/block/blk-merge.c >> @@ -358,11 +358,13 @@ struct bio *__bio_split_to_limits(struct bio *bio, >> default: >> split = bio_split_rw(bio, lim, nr_segs, bs, >> get_max_io_size(bio, lim) << SECTOR_SHIFT); >> + if (IS_ERR(split)) >> + return NULL; > > Can we decide on either passing an ERR_PTR or NULL and do it through > the whole stack? We need the error return here as we could just return NULL and it not be an error, but for further down the stack I feel like returning an error that you can't do anything with (as it's already been dealt with) would be confusing. That's why I did it that way. >> diff --git a/drivers/block/drbd/drbd_req.c b/drivers/block/drbd/drbd_req.c >> index eb14ec8ec04c..e36216d50753 100644 >> --- a/drivers/block/drbd/drbd_req.c >> +++ b/drivers/block/drbd/drbd_req.c >> @@ -1607,6 +1607,8 @@ void drbd_submit_bio(struct bio *bio) >> struct drbd_device *device = bio->bi_bdev->bd_disk->private_data; >> >> bio = bio_split_to_limits(bio); >> + if (!bio) >> + return; > > So for the callers in drivers, do we need thee checks for drivers > that don't even support REQ_NOWAIT? Good point, probably not, we should be erroring these out before they reach the driver. Doesn't hurt though, it would not necessarily be obvious that you'd now need to start checking bio_split_to_limits() return values when you set NOWAIT on the queue.
diff --git a/block/blk-merge.c b/block/blk-merge.c index 35a8f75cc45d..071c5f8cf0cf 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -358,11 +358,13 @@ struct bio *__bio_split_to_limits(struct bio *bio, default: split = bio_split_rw(bio, lim, nr_segs, bs, get_max_io_size(bio, lim) << SECTOR_SHIFT); + if (IS_ERR(split)) + return NULL; break; } if (split) { - /* there isn't chance to merge the splitted bio */ + /* there isn't chance to merge the split bio */ split->bi_opf |= REQ_NOMERGE; blkcg_bio_issue_init(split); diff --git a/block/blk-mq.c b/block/blk-mq.c index c5cf0dbca1db..2c49b4151da1 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2951,8 +2951,11 @@ void blk_mq_submit_bio(struct bio *bio) blk_status_t ret; bio = blk_queue_bounce(bio, q); - if (bio_may_exceed_limits(bio, &q->limits)) + if (bio_may_exceed_limits(bio, &q->limits)) { bio = __bio_split_to_limits(bio, &q->limits, &nr_segs); + if (!bio) + return; + } if (!bio_integrity_prep(bio)) return; diff --git a/drivers/block/drbd/drbd_req.c b/drivers/block/drbd/drbd_req.c index eb14ec8ec04c..e36216d50753 100644 --- a/drivers/block/drbd/drbd_req.c +++ b/drivers/block/drbd/drbd_req.c @@ -1607,6 +1607,8 @@ void drbd_submit_bio(struct bio *bio) struct drbd_device *device = bio->bi_bdev->bd_disk->private_data; bio = bio_split_to_limits(bio); + if (!bio) + return; /* * what we "blindly" assume: diff --git a/drivers/block/ps3vram.c b/drivers/block/ps3vram.c index c76e0148eada..574e470b220b 100644 --- a/drivers/block/ps3vram.c +++ b/drivers/block/ps3vram.c @@ -587,6 +587,8 @@ static void ps3vram_submit_bio(struct bio *bio) dev_dbg(&dev->core, "%s\n", __func__); bio = bio_split_to_limits(bio); + if (!bio) + return; spin_lock_irq(&priv->lock); busy = !bio_list_empty(&priv->list); diff --git a/drivers/md/dm.c b/drivers/md/dm.c index e1ea3a7bd9d9..b424a6ee27ba 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1742,6 +1742,8 @@ static void dm_split_and_process_bio(struct mapped_device *md, * otherwise associated queue_limits won't be imposed. */ bio = bio_split_to_limits(bio); + if (!bio) + return; } init_clone_info(&ci, md, map, bio, is_abnormal); diff --git a/drivers/md/md.c b/drivers/md/md.c index 775f1dde190a..8af639296b3c 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -455,6 +455,8 @@ static void md_submit_bio(struct bio *bio) } bio = bio_split_to_limits(bio); + if (!bio) + return; if (mddev->ro == MD_RDONLY && unlikely(rw == WRITE)) { if (bio_sectors(bio) != 0) diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index c03093b6813c..fc39d01e7b63 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -376,6 +376,8 @@ static void nvme_ns_head_submit_bio(struct bio *bio) * pool from the original queue to allocate the bvecs from. */ bio = bio_split_to_limits(bio); + if (!bio) + return; srcu_idx = srcu_read_lock(&head->srcu); ns = nvme_find_path(head); diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c index b392b9f5482e..c0f85ffb2b62 100644 --- a/drivers/s390/block/dcssblk.c +++ b/drivers/s390/block/dcssblk.c @@ -865,6 +865,8 @@ dcssblk_submit_bio(struct bio *bio) unsigned long bytes_done; bio = bio_split_to_limits(bio); + if (!bio) + return; bytes_done = 0; dev_info = bio->bi_bdev->bd_disk->private_data;
This can't happen right now, but in preparation for allowing bio_split_to_limits() returning NULL if it ended the bio, check for it in all the callers. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- block/blk-merge.c | 4 +++- block/blk-mq.c | 5 ++++- drivers/block/drbd/drbd_req.c | 2 ++ drivers/block/ps3vram.c | 2 ++ drivers/md/dm.c | 2 ++ drivers/md/md.c | 2 ++ drivers/nvme/host/multipath.c | 2 ++ drivers/s390/block/dcssblk.c | 2 ++ 8 files changed, 19 insertions(+), 2 deletions(-)