Messages in this thread Patch in this message | | | Date | Wed, 28 Jul 2021 23:53:05 +0800 | From | Ming Lei <> | Subject | Re: New warning in nvme_setup_discard |
| |
On Wed, Jul 28, 2021 at 03:44:06PM +0200, Oleksandr Natalenko wrote: > Hello. > > On úterý 27. července 2021 17:58:39 CEST Ming Lei wrote: > > BTW, can you test the following patch? which is another approach on the same > > issue with other benefits. > > > > From c853e7ed05a75f631da5b7952b9a989983437819 Mon Sep 17 00:00:00 2001 > > From: Ming Lei <ming.lei@redhat.com> > > Date: Mon, 7 Jun 2021 16:03:51 +0800 > > Subject: [PATCH 2/2] block: support bio merge for multi-range discard > > > > So far multi-range discard treats each bio as one segment(range) of single > > discard request. This way becomes not efficient if lots of small sized > > discard bios are submitted, and one example is raid456. > > > > Support bio merge for multi-range discard for improving lots of small > > sized discard bios. > > > > Turns out it is easy to support it: > > > > 1) always try to merge bio first > > > > 2) run into multi-range discard only if bio merge can't be done > > > > 3) add rq_for_each_discard_range() for retrieving each range(segment) > > of discard request > > > > Reported-by: Wang Shanker <shankerwangmiao@gmail.com> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > block/blk-merge.c | 12 ++++----- > > drivers/block/virtio_blk.c | 9 ++++--- > > drivers/nvme/host/core.c | 8 +++--- > > include/linux/blkdev.h | 51 ++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 66 insertions(+), 14 deletions(-) > > > > diff --git a/block/blk-merge.c b/block/blk-merge.c > > index bcdff1879c34..65210e9a8efa 100644 > > --- a/block/blk-merge.c > > +++ b/block/blk-merge.c > > @@ -724,10 +724,10 @@ static inline bool blk_discard_mergable(struct request > > *req) static enum elv_merge blk_try_req_merge(struct request *req, > > struct request *next) > > { > > - if (blk_discard_mergable(req)) > > - return ELEVATOR_DISCARD_MERGE; > > - else if (blk_rq_pos(req) + blk_rq_sectors(req) == blk_rq_pos(next)) > > + if (blk_rq_pos(req) + blk_rq_sectors(req) == blk_rq_pos(next)) > > return ELEVATOR_BACK_MERGE; > > + else if (blk_discard_mergable(req)) > > + return ELEVATOR_DISCARD_MERGE; > > > > return ELEVATOR_NO_MERGE; > > } > > @@ -908,12 +908,12 @@ bool blk_rq_merge_ok(struct request *rq, struct bio > > *bio) > > > > enum elv_merge blk_try_merge(struct request *rq, struct bio *bio) > > { > > - if (blk_discard_mergable(rq)) > > - return ELEVATOR_DISCARD_MERGE; > > - else if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector) > > + if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector) > > return ELEVATOR_BACK_MERGE; > > else if (blk_rq_pos(rq) - bio_sectors(bio) == bio->bi_iter.bi_sector) > > return ELEVATOR_FRONT_MERGE; > > + else if (blk_discard_mergable(rq)) > > + return ELEVATOR_DISCARD_MERGE; > > return ELEVATOR_NO_MERGE; > > } > > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > > index b9fa3ef5b57c..970cb0d8acaa 100644 > > --- a/drivers/block/virtio_blk.c > > +++ b/drivers/block/virtio_blk.c > > @@ -116,7 +116,6 @@ static int virtblk_setup_discard_write_zeroes(struct > > request *req, bool unmap) unsigned short segments = > > blk_rq_nr_discard_segments(req); > > unsigned short n = 0; > > struct virtio_blk_discard_write_zeroes *range; > > - struct bio *bio; > > u32 flags = 0; > > > > if (unmap) > > @@ -138,9 +137,11 @@ static int virtblk_setup_discard_write_zeroes(struct > > request *req, bool unmap) range[0].sector = cpu_to_le64(blk_rq_pos(req)); > > n = 1; > > } else { > > - __rq_for_each_bio(bio, req) { > > - u64 sector = bio->bi_iter.bi_sector; > > - u32 num_sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT; > > + struct req_discard_range r; > > + > > + rq_for_each_discard_range(r, req) { > > + u64 sector = r.sector; > > + u32 num_sectors = r.size >> SECTOR_SHIFT; > > > > range[n].flags = cpu_to_le32(flags); > > range[n].num_sectors = cpu_to_le32(num_sectors); > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > > index 24bcae88587a..4b0a39360ce9 100644 > > --- a/drivers/nvme/host/core.c > > +++ b/drivers/nvme/host/core.c > > @@ -813,7 +813,7 @@ static blk_status_t nvme_setup_discard(struct nvme_ns > > *ns, struct request *req, { > > unsigned short segments = blk_rq_nr_discard_segments(req), n = 0; > > struct nvme_dsm_range *range; > > - struct bio *bio; > > + struct req_discard_range r; > > > > /* > > * Some devices do not consider the DSM 'Number of Ranges' field when > > @@ -835,9 +835,9 @@ static blk_status_t nvme_setup_discard(struct nvme_ns > > *ns, struct request *req, range = page_address(ns->ctrl->discard_page); > > } > > > > - __rq_for_each_bio(bio, req) { > > - u64 slba = nvme_sect_to_lba(ns, bio->bi_iter.bi_sector); > > - u32 nlb = bio->bi_iter.bi_size >> ns->lba_shift; > > + rq_for_each_discard_range(r, req) { > > + u64 slba = nvme_sect_to_lba(ns, r.sector); > > + u32 nlb = r.size >> ns->lba_shift; > > > > if (n < segments) { > > range[n].cattr = cpu_to_le32(0); > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > > index d66d0da72529..bd9d22269a7b 100644 > > --- a/include/linux/blkdev.h > > +++ b/include/linux/blkdev.h > > @@ -1007,6 +1007,57 @@ static inline unsigned int blk_rq_stats_sectors(const > > struct request *rq) return rq->stats_sectors; > > } > > > > +struct req_discard_range { > > + sector_t sector; > > + unsigned int size; > > + > > + /* > > + * internal field: driver don't use it, and it always points to > > + * next bio to be processed > > + */ > > + struct bio *__bio; > > +}; > > + > > +static inline void req_init_discard_range_iter(const struct request *rq, > > + struct req_discard_range *range) > > +{ > > + range->__bio = rq->bio; > > +} > > + > > +/* return true if @range stores one valid discard range */ > > +static inline bool req_get_discard_range(struct req_discard_range *range) > > +{ > > + struct bio *bio; > > + > > + if (!range->__bio) > > + return false; > > + > > + bio = range->__bio; > > + range->sector = bio->bi_iter.bi_sector; > > + range->size = bio->bi_iter.bi_size; > > + range->__bio = bio->bi_next; > > + > > + while (range->__bio) { > > + struct bio *bio = range->__bio; > > + > > + if (range->sector + (range->size >> SECTOR_SHIFT) != > > + bio->bi_iter.bi_sector) > > + break; > > + > > + /* > > + * ->size won't overflow because req->__data_len is defined > > + * as 'unsigned int' > > + */ > > + range->size += bio->bi_iter.bi_size; > > + range->__bio = bio->bi_next; > > + } > > + return true; > > +} > > + > > +#define rq_for_each_discard_range(range, rq) \ > > + for (req_init_discard_range_iter((rq), &range); \ > > + req_get_discard_range(&range);) > > + > > #ifdef CONFIG_BLK_DEV_ZONED > > > > /* Helper to convert BLK_ZONE_ZONE_XXX to its string format XXX */ > > Do I have to revert the previous one and apply this one? If so, with this one the issue is triggered pretty quick:
Yeah, the previous one needs to be reverted.
> > ``` > kernel: ------------[ cut here ]------------ > kernel: WARNING: CPU: 20 PID: 490 at drivers/nvme/host/core.c:850 nvme_setup_discard+0x1b9/0x220
Can you collect debug log by applying the following patch against the last one?
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 8780e4aa9df2..fbd8a68c619b 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -828,6 +828,24 @@ static inline void nvme_setup_flush(struct nvme_ns *ns, cmnd->common.nsid = cpu_to_le32(ns->head->ns_id); } +static inline void blk_dump_rq(const struct request *req) +{ + struct bio *bio; + int i = 0; + + printk("dump req %p(f:%x, seg: %d)\n", req, req->cmd_flags, + req->nr_phys_segments); + + __rq_for_each_bio(bio, req) { + printk("%d-%p: %hx/%hx %llu %u\n", + i++, bio, + bio->bi_flags, bio->bi_opf, + (unsigned long long)bio->bi_iter.bi_sector, + bio->bi_iter.bi_size>>9); + } +} + + static blk_status_t nvme_setup_discard(struct nvme_ns *ns, struct request *req, struct nvme_command *cmnd) { @@ -868,6 +886,8 @@ static blk_status_t nvme_setup_discard(struct nvme_ns *ns, struct request *req, } if (WARN_ON_ONCE(n != segments)) { + printk("%s: ranges %u segments %u\n", __func__, n, segments); + blk_dump_rq(req); if (virt_to_page(range) == ns->ctrl->discard_page) clear_bit_unlock(0, &ns->ctrl->discard_page_busy); else
Thanks, Ming
| |