<div dir="ltr"><div>Thank you for the comment, Kazu.</div><div><br></div><div dir="ltr">On Fri, May 27, 2022 at 5:11 PM HAGIO KAZUHITO(萩尾 一仁) <<a href="mailto:k-hagio-ab@nec.com" target="_blank">k-hagio-ab@nec.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 2022/05/27 15:32, lijiang wrote:<br>
> >> + /* static_rqs */<br>
> >> + ret = 0;<br>
> >> + addr = tag + OFFSET(blk_mq_tags_static_rqs);<br>
> >> + if (!readmem(addr, KVADDR, &rqs_addr, sizeof(void *), "blk_mq_tags.static_rqs", RETURN_ON_ERROR))<br>
> >> + return ret;<br>
> >> + addr = rqs_addr + bitnr * sizeof(ulong); /* static_rqs[bitnr] */<br>
> >> + if (!readmem(addr, KVADDR, &rq, sizeof(ulong), "blk_mq_tags.static_rqs[]", RETURN_ON_ERROR))<br>
> >> + return ret;<br>
> >> + ret = tag_iter->fn(rq, tag_iter->data);<br>
> >> +<br>
> >> + return ret;<br>
> ><br>
> > I'm not familiar with blk-mq, I'd like some information.<br>
> ><br>
> > I think that the "dev -d" displays the same inflight stats in /proc/diskstats<br>
> > for devices without mq:<br>
> ><br>
> > diskstats_show<br>
> > part_in_flight<br>
> > count up per_cpu disk_stats->in_flight<br>
> ><br>
> > so, if we do the same thing for mq devices, we should imitate this path:<br>
> ><br>
> > diskstats_show<br>
> > blk_mq_in_flight<br>
> > blk_mq_queue_tag_busy_iter<br>
> > queue_for_each_hw_ctx<br>
> > bt_for_each<br>
> > sbitmap_for_each_set<br>
> > bt_iter<br>
> > blk_mq_check_inflight<br>
> ><br>
> > On the path, I cannot see the static_rqs being counted.<br>
> > Why does it need to be counted?<br>
> ><br>
> <br>
> You are right. Basically, the implementation imitates the above path in the kernel. But not exactly, there are<br>
> two changes in this patch:<br>
> [1] add additional statistics for the static_rqs[] and sched_tags<br>
> [2] the calculate_disk_io() imitates the blk_mq_check_inflight(), but doesn't check if the status of rq is in flight.<br>
> <br>
> For the [1], because the rqs[] only contains the requests from the driver tag, once the io scheduler is used, crash<br>
> may miss the check of sched_tags and can not watch its value . You mean that crash shouldn't cover this case,<br>
> or do I misunderstand the sched_tags and static_rqs[]?<br>
<br>
hmm, if so, I would like to know the reason why kernel doesn't count them.<br>
<br>
> For the [2], the intent is to calculate all requests in the current queue from the bitmap(not only inflight, but also handling<br>
> rq), that can help to get actual read and write request counts. But, to be honest, I'm not sure whether this is a reasonable<br>
> method. Are you concerned that it might have repeat read/write request counts? Or should the crash follow the above<br>
> kernel path exactly? Any idea about this?<br>
<br>
Yes, it's one of my concerns. It does not count a request twice?<br></blockquote><div><br></div><div>I didn't watch them, seems that the blk-mq bypass the io scheduler?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
IMHO, basically crash should follow the kernel path exactly, including the<br>
check for MQ_RQ_IN_FLIGHT. The "dev -d" option displays the same stats as<br>
/proc/diskstats for non-mq devices, so displaying different stats for mq<br>
devices will be confusing. And it will make tracking kernel changes easier.<br>
<br></blockquote><div> </div><div>The various scenarios of the block layer are complicated, it might be safe to keep exactly the same as the kernel path.</div><div>Let me adjust it a bit. Thank you for the suggestions.</div><div><br></div><div>Thanks.</div><div>Lianbo</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks,<br>
Kazu</blockquote></div></div>