| Message ID | 20190828215429.4572-14-logang@deltatee.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nvmet: add target passthru commands support | expand |
> Support block disk accounting by setting the RQF_IO_STAT flag > and gendisk in the request. > > After this change, IO counts will be reflected correctly in > /proc/diskstats for drives being used by passthru. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > --- > drivers/nvme/target/io-cmd-passthru.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/nvme/target/io-cmd-passthru.c b/drivers/nvme/target/io-cmd-passthru.c > index 7557927a3451..63f12750a80d 100644 > --- a/drivers/nvme/target/io-cmd-passthru.c > +++ b/drivers/nvme/target/io-cmd-passthru.c > @@ -410,6 +410,9 @@ static struct request *nvmet_passthru_blk_make_request(struct nvmet_req *req, > if (unlikely(IS_ERR(rq))) > return rq; > > + if (blk_queue_io_stat(q) && cmd->common.opcode != nvme_cmd_flush) > + rq->rq_flags |= RQF_IO_STAT; Does flush has data bytes in the request? Why the special casing?
On 2019-09-06 6:00 p.m., Sagi Grimberg wrote: > >> Support block disk accounting by setting the RQF_IO_STAT flag >> and gendisk in the request. >> >> After this change, IO counts will be reflected correctly in >> /proc/diskstats for drives being used by passthru. >> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> --- >> drivers/nvme/target/io-cmd-passthru.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/nvme/target/io-cmd-passthru.c b/drivers/nvme/target/io-cmd-passthru.c >> index 7557927a3451..63f12750a80d 100644 >> --- a/drivers/nvme/target/io-cmd-passthru.c >> +++ b/drivers/nvme/target/io-cmd-passthru.c >> @@ -410,6 +410,9 @@ static struct request *nvmet_passthru_blk_make_request(struct nvmet_req *req, >> if (unlikely(IS_ERR(rq))) >> return rq; >> >> + if (blk_queue_io_stat(q) && cmd->common.opcode != nvme_cmd_flush) >> + rq->rq_flags |= RQF_IO_STAT; Thanks for the review! > Does flush has data bytes in the request? Why the special casing? Well it was special cased in the vanilla blk account flow... But I think it's required to be special cased so the IO and in_flight counts don't count flushes (as they do not for regular block device traffic). Logan
>>> Support block disk accounting by setting the RQF_IO_STAT flag >>> and gendisk in the request. >>> >>> After this change, IO counts will be reflected correctly in >>> /proc/diskstats for drives being used by passthru. >>> >>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >>> --- >>> drivers/nvme/target/io-cmd-passthru.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/nvme/target/io-cmd-passthru.c b/drivers/nvme/target/io-cmd-passthru.c >>> index 7557927a3451..63f12750a80d 100644 >>> --- a/drivers/nvme/target/io-cmd-passthru.c >>> +++ b/drivers/nvme/target/io-cmd-passthru.c >>> @@ -410,6 +410,9 @@ static struct request *nvmet_passthru_blk_make_request(struct nvmet_req *req, >>> if (unlikely(IS_ERR(rq))) >>> return rq; >>> >>> + if (blk_queue_io_stat(q) && cmd->common.opcode != nvme_cmd_flush) >>> + rq->rq_flags |= RQF_IO_STAT; > > Thanks for the review! > >> Does flush has data bytes in the request? Why the special casing? > > Well it was special cased in the vanilla blk account flow... But I think > it's required to be special cased so the IO and in_flight counts don't > count flushes (as they do not for regular block device traffic). I think that the accounting exclude I/O that is yielded from the flush sequence. Don't think its relevant here...
On 2019-09-09 1:26 p.m., Sagi Grimberg wrote: > >>>> Support block disk accounting by setting the RQF_IO_STAT flag >>>> and gendisk in the request. >>>> >>>> After this change, IO counts will be reflected correctly in >>>> /proc/diskstats for drives being used by passthru. >>>> >>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >>>> --- >>>> drivers/nvme/target/io-cmd-passthru.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/nvme/target/io-cmd-passthru.c b/drivers/nvme/target/io-cmd-passthru.c >>>> index 7557927a3451..63f12750a80d 100644 >>>> --- a/drivers/nvme/target/io-cmd-passthru.c >>>> +++ b/drivers/nvme/target/io-cmd-passthru.c >>>> @@ -410,6 +410,9 @@ static struct request *nvmet_passthru_blk_make_request(struct nvmet_req *req, >>>> if (unlikely(IS_ERR(rq))) >>>> return rq; >>>> >>>> + if (blk_queue_io_stat(q) && cmd->common.opcode != nvme_cmd_flush) >>>> + rq->rq_flags |= RQF_IO_STAT; >> >> Thanks for the review! >> >>> Does flush has data bytes in the request? Why the special casing? >> >> Well it was special cased in the vanilla blk account flow... But I think >> it's required to be special cased so the IO and in_flight counts don't >> count flushes (as they do not for regular block device traffic). > > I think that the accounting exclude I/O that is yielded from the flush > sequence. Don't think its relevant here... What? Per blk_account_io_done(), RQF_FLUSH_SEQ will not be set by us for passthru commands and I don't think it's appropriate to do so. Thus, if we set RQF_IO_STAT for passthru flush commands, they will be counted which we do not want. Logan
>>>>> Support block disk accounting by setting the RQF_IO_STAT flag >>>>> and gendisk in the request. >>>>> >>>>> After this change, IO counts will be reflected correctly in >>>>> /proc/diskstats for drives being used by passthru. >>>>> >>>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >>>>> --- >>>>> drivers/nvme/target/io-cmd-passthru.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/nvme/target/io-cmd-passthru.c b/drivers/nvme/target/io-cmd-passthru.c >>>>> index 7557927a3451..63f12750a80d 100644 >>>>> --- a/drivers/nvme/target/io-cmd-passthru.c >>>>> +++ b/drivers/nvme/target/io-cmd-passthru.c >>>>> @@ -410,6 +410,9 @@ static struct request *nvmet_passthru_blk_make_request(struct nvmet_req *req, >>>>> if (unlikely(IS_ERR(rq))) >>>>> return rq; >>>>> >>>>> + if (blk_queue_io_stat(q) && cmd->common.opcode != nvme_cmd_flush) >>>>> + rq->rq_flags |= RQF_IO_STAT; >>> >>> Thanks for the review! >>> >>>> Does flush has data bytes in the request? Why the special casing? >>> >>> Well it was special cased in the vanilla blk account flow... But I think >>> it's required to be special cased so the IO and in_flight counts don't >>> count flushes (as they do not for regular block device traffic). >> >> I think that the accounting exclude I/O that is yielded from the flush >> sequence. Don't think its relevant here... > > What? Per blk_account_io_done(), RQF_FLUSH_SEQ will not be set by us for > passthru commands and I don't think it's appropriate to do so. Thus, if > we set RQF_IO_STAT for passthru flush commands, they will be counted > which we do not want. Have you considered to have nvmet_passthru_blk_make_request set RQF_FUA for nvme_cmd_flush? this way blk_insert_flush will be called and RQF_FLUSH_SEQ will be set and you don't need to worry about this special casing...
On 2019-09-09 5:15 p.m., Sagi Grimberg wrote: > >>>>>> Support block disk accounting by setting the RQF_IO_STAT flag >>>>>> and gendisk in the request. >>>>>> >>>>>> After this change, IO counts will be reflected correctly in >>>>>> /proc/diskstats for drives being used by passthru. >>>>>> >>>>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >>>>>> --- >>>>>> drivers/nvme/target/io-cmd-passthru.c | 5 ++++- >>>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/nvme/target/io-cmd-passthru.c >>>>>> b/drivers/nvme/target/io-cmd-passthru.c >>>>>> index 7557927a3451..63f12750a80d 100644 >>>>>> --- a/drivers/nvme/target/io-cmd-passthru.c >>>>>> +++ b/drivers/nvme/target/io-cmd-passthru.c >>>>>> @@ -410,6 +410,9 @@ static struct request >>>>>> *nvmet_passthru_blk_make_request(struct nvmet_req *req, >>>>>> if (unlikely(IS_ERR(rq))) >>>>>> return rq; >>>>>> + if (blk_queue_io_stat(q) && cmd->common.opcode != >>>>>> nvme_cmd_flush) >>>>>> + rq->rq_flags |= RQF_IO_STAT; >>>> >>>> Thanks for the review! >>>> >>>>> Does flush has data bytes in the request? Why the special casing? >>>> >>>> Well it was special cased in the vanilla blk account flow... But I >>>> think >>>> it's required to be special cased so the IO and in_flight counts don't >>>> count flushes (as they do not for regular block device traffic). >>> >>> I think that the accounting exclude I/O that is yielded from the flush >>> sequence. Don't think its relevant here... >> >> What? Per blk_account_io_done(), RQF_FLUSH_SEQ will not be set by us for >> passthru commands and I don't think it's appropriate to do so. Thus, if >> we set RQF_IO_STAT for passthru flush commands, they will be counted >> which we do not want. > > Have you considered to have nvmet_passthru_blk_make_request set RQF_FUA > for nvme_cmd_flush? this way blk_insert_flush will be called and > RQF_FLUSH_SEQ will be set and you don't need to worry about this > special casing... Well, I haven't done that mostly because I'm not sure of all the side effects of doing so. However, I've tried it and everything seems to still work, so I'll make that change for v9 which I'll probably send out after the merge window. Logan
diff --git a/drivers/nvme/target/io-cmd-passthru.c b/drivers/nvme/target/io-cmd-passthru.c index 7557927a3451..63f12750a80d 100644 --- a/drivers/nvme/target/io-cmd-passthru.c +++ b/drivers/nvme/target/io-cmd-passthru.c @@ -410,6 +410,9 @@ static struct request *nvmet_passthru_blk_make_request(struct nvmet_req *req, if (unlikely(IS_ERR(rq))) return rq; + if (blk_queue_io_stat(q) && cmd->common.opcode != nvme_cmd_flush) + rq->rq_flags |= RQF_IO_STAT; + if (req->sg_cnt) { ret = nvmet_passthru_map_sg(req, rq); if (unlikely(ret)) { @@ -474,7 +477,7 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req) rq->end_io_data = req; if (req->sq->qid != 0) { - blk_execute_rq_nowait(rq->q, NULL, rq, 0, + blk_execute_rq_nowait(rq->q, ns->disk, rq, 0, nvmet_passthru_req_done); } else { req->p.rq = rq;
Support block disk accounting by setting the RQF_IO_STAT flag and gendisk in the request. After this change, IO counts will be reflected correctly in /proc/diskstats for drives being used by passthru. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- drivers/nvme/target/io-cmd-passthru.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)