Re: New warning in nvme_setup_discard
From: Ming Lei
Date: Wed Jul 28 2021 - 11:53:40 EST
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@xxxxxxxxxx>
> > 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@xxxxxxxxx>
> > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > ---
> > 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