* [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
@ 2020-10-09 23:18 Logan Gunthorpe
2020-10-13 22:26 ` Sagi Grimberg
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Logan Gunthorpe @ 2020-10-09 23:18 UTC (permalink / raw)
To: linux-nvme
Cc: Sagi Grimberg, Logan Gunthorpe, Christoph Hellwig,
Chaitanya Kulkarni, Douglas Gilbert
Clean up some confusing elements of nvmet_passthru_map_sg() by returning
early if the request is greater than the maximum bio size. This allows
us to drop the sg_cnt variable.
This should not result in any functional change but makes the code
clearer and more understandable. The original code allocated a truncated
bio then would return EINVAL when bio_add_pc_page() filled that bio. The
new code just returns EINVAL early if this would happen.
Fixes: c1fef73f793b ("nvmet: add passthru code to process commands")
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Suggested-by: Douglas Gilbert <dgilbert@interlog.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
drivers/nvme/target/passthru.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index dacfa7435d0b..076e829490a1 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -180,18 +180,20 @@ static void nvmet_passthru_req_done(struct request *rq,
static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
{
- int sg_cnt = req->sg_cnt;
struct scatterlist *sg;
int op_flags = 0;
struct bio *bio;
int i, ret;
+ if (req->sg_cnt > BIO_MAX_PAGES)
+ return -EINVAL;
+
if (req->cmd->common.opcode == nvme_cmd_flush)
op_flags = REQ_FUA;
else if (nvme_is_write(req->cmd))
op_flags = REQ_SYNC | REQ_IDLE;
- bio = bio_alloc(GFP_KERNEL, min(sg_cnt, BIO_MAX_PAGES));
+ bio = bio_alloc(GFP_KERNEL, req->sg_cnt);
bio->bi_end_io = bio_put;
bio->bi_opf = req_op(rq) | op_flags;
@@ -201,7 +203,6 @@ static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
bio_put(bio);
return -EINVAL;
}
- sg_cnt--;
}
ret = blk_rq_append_bio(rq, &bio);
base-commit: 549738f15da0e5a00275977623be199fbbf7df50
--
2.20.1
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-09 23:18 [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg() Logan Gunthorpe
@ 2020-10-13 22:26 ` Sagi Grimberg
2020-10-13 22:38 ` Douglas Gilbert
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Sagi Grimberg @ 2020-10-13 22:26 UTC (permalink / raw)
To: Logan Gunthorpe, linux-nvme
Cc: Christoph Hellwig, Chaitanya Kulkarni, Douglas Gilbert
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-09 23:18 [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg() Logan Gunthorpe
2020-10-13 22:26 ` Sagi Grimberg
@ 2020-10-13 22:38 ` Douglas Gilbert
2020-10-14 0:16 ` Chaitanya Kulkarni
2020-10-15 7:56 ` Christoph Hellwig
3 siblings, 0 replies; 14+ messages in thread
From: Douglas Gilbert @ 2020-10-13 22:38 UTC (permalink / raw)
To: Logan Gunthorpe, linux-nvme
Cc: Christoph Hellwig, Chaitanya Kulkarni, Sagi Grimberg
On 2020-10-09 7:18 p.m., Logan Gunthorpe wrote:
> Clean up some confusing elements of nvmet_passthru_map_sg() by returning
> early if the request is greater than the maximum bio size. This allows
> us to drop the sg_cnt variable.
>
> This should not result in any functional change but makes the code
> clearer and more understandable. The original code allocated a truncated
> bio then would return EINVAL when bio_add_pc_page() filled that bio. The
> new code just returns EINVAL early if this would happen.
>
> Fixes: c1fef73f793b ("nvmet: add passthru code to process commands")
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Suggested-by: Douglas Gilbert <dgilbert@interlog.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Reviewed-by: Douglas Gilbert <dgilbert@interlog.com>
I have commented offline to Logan about the return from bio_alloc()
not being checked for NULL. He assures me that can't happen but I believe
that is true only for small scatter-gather lists. So I guess it depends
on the size of the data this is planned to be sent through this
pass-through.
> ---
> drivers/nvme/target/passthru.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
> index dacfa7435d0b..076e829490a1 100644
> --- a/drivers/nvme/target/passthru.c
> +++ b/drivers/nvme/target/passthru.c
> @@ -180,18 +180,20 @@ static void nvmet_passthru_req_done(struct request *rq,
>
> static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
> {
> - int sg_cnt = req->sg_cnt;
> struct scatterlist *sg;
> int op_flags = 0;
> struct bio *bio;
> int i, ret;
>
> + if (req->sg_cnt > BIO_MAX_PAGES)
> + return -EINVAL;
> +
> if (req->cmd->common.opcode == nvme_cmd_flush)
> op_flags = REQ_FUA;
> else if (nvme_is_write(req->cmd))
> op_flags = REQ_SYNC | REQ_IDLE;
>
> - bio = bio_alloc(GFP_KERNEL, min(sg_cnt, BIO_MAX_PAGES));
> + bio = bio_alloc(GFP_KERNEL, req->sg_cnt);
> bio->bi_end_io = bio_put;
> bio->bi_opf = req_op(rq) | op_flags;
>
> @@ -201,7 +203,6 @@ static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
> bio_put(bio);
> return -EINVAL;
> }
> - sg_cnt--;
> }
>
> ret = blk_rq_append_bio(rq, &bio);
>
> base-commit: 549738f15da0e5a00275977623be199fbbf7df50
>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-09 23:18 [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg() Logan Gunthorpe
2020-10-13 22:26 ` Sagi Grimberg
2020-10-13 22:38 ` Douglas Gilbert
@ 2020-10-14 0:16 ` Chaitanya Kulkarni
2020-10-14 0:20 ` Logan Gunthorpe
2020-10-15 7:56 ` Christoph Hellwig
3 siblings, 1 reply; 14+ messages in thread
From: Chaitanya Kulkarni @ 2020-10-14 0:16 UTC (permalink / raw)
To: Logan Gunthorpe, linux-nvme
Cc: Sagi Grimberg, Christoph Hellwig, Douglas Gilbert
On 10/9/20 16:18, Logan Gunthorpe wrote:
> Clean up some confusing elements of nvmet_passthru_map_sg() by returning
> early if the request is greater than the maximum bio size. This allows
> us to drop the sg_cnt variable.
>
> This should not result in any functional change but makes the code
> clearer and more understandable. The original code allocated a truncated
> bio then would return EINVAL when bio_add_pc_page() filled that bio. The
> new code just returns EINVAL early if this would happen.
>
> Fixes: c1fef73f793b ("nvmet: add passthru code to process commands")
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Suggested-by: Douglas Gilbert <dgilbert@interlog.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
why the prefix is nvmet-passthru ?
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-14 0:16 ` Chaitanya Kulkarni
@ 2020-10-14 0:20 ` Logan Gunthorpe
2020-10-14 0:25 ` Chaitanya Kulkarni
0 siblings, 1 reply; 14+ messages in thread
From: Logan Gunthorpe @ 2020-10-14 0:20 UTC (permalink / raw)
To: Chaitanya Kulkarni, linux-nvme
Cc: Sagi Grimberg, Christoph Hellwig, Douglas Gilbert
On 2020-10-13 6:16 p.m., Chaitanya Kulkarni wrote:
> On 10/9/20 16:18, Logan Gunthorpe wrote:
>> Clean up some confusing elements of nvmet_passthru_map_sg() by returning
>> early if the request is greater than the maximum bio size. This allows
>> us to drop the sg_cnt variable.
>>
>> This should not result in any functional change but makes the code
>> clearer and more understandable. The original code allocated a truncated
>> bio then would return EINVAL when bio_add_pc_page() filled that bio. The
>> new code just returns EINVAL early if this would happen.
>>
>> Fixes: c1fef73f793b ("nvmet: add passthru code to process commands")
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>> Suggested-by: Douglas Gilbert <dgilbert@interlog.com>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: Sagi Grimberg <sagi@grimberg.me>
>> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>
> why the prefix is nvmet-passthru ?
Well that's the prefix I used in the first patch, which follows other
similar conventions like nvmet-tcp, nvmet-fc, nvmet-rdma, nvmet-loop, etc.
Logan
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-14 0:20 ` Logan Gunthorpe
@ 2020-10-14 0:25 ` Chaitanya Kulkarni
2020-10-14 15:47 ` Logan Gunthorpe
0 siblings, 1 reply; 14+ messages in thread
From: Chaitanya Kulkarni @ 2020-10-14 0:25 UTC (permalink / raw)
To: Logan Gunthorpe, linux-nvme
Cc: Sagi Grimberg, Christoph Hellwig, Douglas Gilbert
On 10/13/20 17:20, Logan Gunthorpe wrote:
>
> On 2020-10-13 6:16 p.m., Chaitanya Kulkarni wrote:
>> On 10/9/20 16:18, Logan Gunthorpe wrote:
>>> Clean up some confusing elements of nvmet_passthru_map_sg() by returning
>>> early if the request is greater than the maximum bio size. This allows
>>> us to drop the sg_cnt variable.
>>>
>>> This should not result in any functional change but makes the code
>>> clearer and more understandable. The original code allocated a truncated
>>> bio then would return EINVAL when bio_add_pc_page() filled that bio. The
>>> new code just returns EINVAL early if this would happen.
>>>
>>> Fixes: c1fef73f793b ("nvmet: add passthru code to process commands")
>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>>> Suggested-by: Douglas Gilbert <dgilbert@interlog.com>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Cc: Sagi Grimberg <sagi@grimberg.me>
>>> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>> why the prefix is nvmet-passthru ?
> Well that's the prefix I used in the first patch, which follows other
> similar conventions like nvmet-tcp, nvmet-fc, nvmet-rdma, nvmet-loop, etc.
>
> Logan
>
>
Those are transport types and passthru is a target backend.
I found this [1] in the git log, only one patch has that prefix.
The convention is to not add backend prefixes for target patches (see
bdev/file),
I'm not sure we want to start adding prefixes unless there is a specific
reason.
# git log --oneline drivers/nvme/target/passthru.c
dc1890b99602 nvmet: add passthru ZNS support
df8dd70a9db8 nvme: lift the file open code from nvme_ctrl_get_by_path
3a6b076168e2 (tag: nvme-5.9-2020-09-17) nvmet: get transport reference
for passthru ctrl
7ee51cf60a90 nvmet: call blk_mq_free_request() directly
a2138fd49467 nvmet: fix oops in pt cmd execution
4db69a3d7cfe nvmet: add ns tear down label for pt-cmd handling
0ceeab96ba59 nvmet-passthru: Reject commands with non-sgl flags set
ba76af676cd0 nvmet: Add passthru enable/disable helpers
c1fef73f793b nvmet: add passthru code to process commands
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-14 0:25 ` Chaitanya Kulkarni
@ 2020-10-14 15:47 ` Logan Gunthorpe
0 siblings, 0 replies; 14+ messages in thread
From: Logan Gunthorpe @ 2020-10-14 15:47 UTC (permalink / raw)
To: Chaitanya Kulkarni, linux-nvme
Cc: Sagi Grimberg, Christoph Hellwig, Douglas Gilbert
On 2020-10-13 6:25 p.m., Chaitanya Kulkarni wrote:
> On 10/13/20 17:20, Logan Gunthorpe wrote:
>>
>> On 2020-10-13 6:16 p.m., Chaitanya Kulkarni wrote:
>>> On 10/9/20 16:18, Logan Gunthorpe wrote:
>>>> Clean up some confusing elements of nvmet_passthru_map_sg() by returning
>>>> early if the request is greater than the maximum bio size. This allows
>>>> us to drop the sg_cnt variable.
>>>>
>>>> This should not result in any functional change but makes the code
>>>> clearer and more understandable. The original code allocated a truncated
>>>> bio then would return EINVAL when bio_add_pc_page() filled that bio. The
>>>> new code just returns EINVAL early if this would happen.
>>>>
>>>> Fixes: c1fef73f793b ("nvmet: add passthru code to process commands")
>>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>>>> Suggested-by: Douglas Gilbert <dgilbert@interlog.com>
>>>> Cc: Christoph Hellwig <hch@lst.de>
>>>> Cc: Sagi Grimberg <sagi@grimberg.me>
>>>> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>>> why the prefix is nvmet-passthru ?
>> Well that's the prefix I used in the first patch, which follows other
>> similar conventions like nvmet-tcp, nvmet-fc, nvmet-rdma, nvmet-loop, etc.
>>
>> Logan
>>
>>
> Those are transport types and passthru is a target backend.
>
> I found this [1] in the git log, only one patch has that prefix.
>
> The convention is to not add backend prefixes for target patches (see
> bdev/file),
>
> I'm not sure we want to start adding prefixes unless there is a specific
>
> reason.
Well, I don't see the harm in it, but if whomever merges it wants to
change the prefix, I won't object.
Logan
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-09 23:18 [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg() Logan Gunthorpe
` (2 preceding siblings ...)
2020-10-14 0:16 ` Chaitanya Kulkarni
@ 2020-10-15 7:56 ` Christoph Hellwig
2020-10-15 16:01 ` Logan Gunthorpe
3 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2020-10-15 7:56 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Sagi Grimberg, Chaitanya Kulkarni, Christoph Hellwig, linux-nvme,
Douglas Gilbert
On Fri, Oct 09, 2020 at 05:18:16PM -0600, Logan Gunthorpe wrote:
> static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
> {
> - int sg_cnt = req->sg_cnt;
> struct scatterlist *sg;
> int op_flags = 0;
> struct bio *bio;
> int i, ret;
>
> + if (req->sg_cnt > BIO_MAX_PAGES)
> + return -EINVAL;
Don't you need to handle larger requests as well? Or at least
limit MDTS?
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-15 7:56 ` Christoph Hellwig
@ 2020-10-15 16:01 ` Logan Gunthorpe
2020-10-15 17:24 ` Douglas Gilbert
2020-10-15 18:01 ` Christoph Hellwig
0 siblings, 2 replies; 14+ messages in thread
From: Logan Gunthorpe @ 2020-10-15 16:01 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Chaitanya Kulkarni, Sagi Grimberg, linux-nvme, Douglas Gilbert
On 2020-10-15 1:56 a.m., Christoph Hellwig wrote:
> On Fri, Oct 09, 2020 at 05:18:16PM -0600, Logan Gunthorpe wrote:
>> static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
>> {
>> - int sg_cnt = req->sg_cnt;
>> struct scatterlist *sg;
>> int op_flags = 0;
>> struct bio *bio;
>> int i, ret;
>>
>> + if (req->sg_cnt > BIO_MAX_PAGES)
>> + return -EINVAL;
>
> Don't you need to handle larger requests as well? Or at least
> limit MDTS?
No and Yes: there is already code in nvmet_passthru_override_id_ctrl()
to limit MDTS based on max_segments and max_hw_sectors. So any request
that doesn't pass this check should be from a misbehaving client and an
error should be appropriate.
Logan
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-15 16:01 ` Logan Gunthorpe
@ 2020-10-15 17:24 ` Douglas Gilbert
2020-10-15 18:01 ` Christoph Hellwig
1 sibling, 0 replies; 14+ messages in thread
From: Douglas Gilbert @ 2020-10-15 17:24 UTC (permalink / raw)
To: Logan Gunthorpe, Christoph Hellwig
Cc: Chaitanya Kulkarni, Sagi Grimberg, linux-nvme
On 2020-10-15 12:01 p.m., Logan Gunthorpe wrote:
>
>
> On 2020-10-15 1:56 a.m., Christoph Hellwig wrote:
>> On Fri, Oct 09, 2020 at 05:18:16PM -0600, Logan Gunthorpe wrote:
>>> static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
>>> {
>>> - int sg_cnt = req->sg_cnt;
>>> struct scatterlist *sg;
>>> int op_flags = 0;
>>> struct bio *bio;
>>> int i, ret;
>>>
>>> + if (req->sg_cnt > BIO_MAX_PAGES)
>>> + return -EINVAL;
>>
>> Don't you need to handle larger requests as well? Or at least
>> limit MDTS?
>
> No and Yes: there is already code in nvmet_passthru_override_id_ctrl()
> to limit MDTS based on max_segments and max_hw_sectors. So any request
> that doesn't pass this check should be from a misbehaving client and an
> error should be appropriate.
Running the numbers: with PAGE_SIZE of 4096 bytes and BIO_MAX_PAGES at
256 gives 1 MiB. From memory the block layer won't accept single requests
bigger than 4 MiB (or 8 MiB). Then it is possible that the sgl was
built with sgl_alloc_order(order > 0, chainable=false) in which case
the maximum (unchained) bio carrying size goes up to:
PAGE_SIZE * (2^order) * 256
I'm using order=3 by default in my driver. So one (unchained) bio will
hold as much (or more) than the block layer will take.
Doug Gilbert
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-15 16:01 ` Logan Gunthorpe
2020-10-15 17:24 ` Douglas Gilbert
@ 2020-10-15 18:01 ` Christoph Hellwig
2020-10-15 18:40 ` Logan Gunthorpe
1 sibling, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2020-10-15 18:01 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Sagi Grimberg, Chaitanya Kulkarni, Christoph Hellwig, linux-nvme,
Douglas Gilbert
On Thu, Oct 15, 2020 at 10:01:30AM -0600, Logan Gunthorpe wrote:
>
>
> On 2020-10-15 1:56 a.m., Christoph Hellwig wrote:
> > On Fri, Oct 09, 2020 at 05:18:16PM -0600, Logan Gunthorpe wrote:
> >> static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
> >> {
> >> - int sg_cnt = req->sg_cnt;
> >> struct scatterlist *sg;
> >> int op_flags = 0;
> >> struct bio *bio;
> >> int i, ret;
> >>
> >> + if (req->sg_cnt > BIO_MAX_PAGES)
> >> + return -EINVAL;
> >
> > Don't you need to handle larger requests as well? Or at least
> > limit MDTS?
>
> No and Yes: there is already code in nvmet_passthru_override_id_ctrl()
> to limit MDTS based on max_segments and max_hw_sectors.
But those are entirely unrelated to the bio size. BIO_MAX_PAGES is
256, so with 4k pages and assuming none can't be merged that is 1MB,
while max_segments/max_hw_sectors could be something much larger.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-15 18:01 ` Christoph Hellwig
@ 2020-10-15 18:40 ` Logan Gunthorpe
2020-10-16 13:57 ` Christoph Hellwig
0 siblings, 1 reply; 14+ messages in thread
From: Logan Gunthorpe @ 2020-10-15 18:40 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Chaitanya Kulkarni, Sagi Grimberg, linux-nvme, Douglas Gilbert
On 2020-10-15 12:01 p.m., Christoph Hellwig wrote:
> On Thu, Oct 15, 2020 at 10:01:30AM -0600, Logan Gunthorpe wrote:
>>
>>
>> On 2020-10-15 1:56 a.m., Christoph Hellwig wrote:
>>> On Fri, Oct 09, 2020 at 05:18:16PM -0600, Logan Gunthorpe wrote:
>>>> static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
>>>> {
>>>> - int sg_cnt = req->sg_cnt;
>>>> struct scatterlist *sg;
>>>> int op_flags = 0;
>>>> struct bio *bio;
>>>> int i, ret;
>>>>
>>>> + if (req->sg_cnt > BIO_MAX_PAGES)
>>>> + return -EINVAL;
>>>
>>> Don't you need to handle larger requests as well? Or at least
>>> limit MDTS?
>>
>> No and Yes: there is already code in nvmet_passthru_override_id_ctrl()
>> to limit MDTS based on max_segments and max_hw_sectors.
>
> But those are entirely unrelated to the bio size. BIO_MAX_PAGES is
> 256, so with 4k pages and assuming none can't be merged that is 1MB,
> while max_segments/max_hw_sectors could be something much larger.
Isn't it constrained by max_segments which is set to NVME_MAX_SEGS=127
(for PCI)... less than BIO_MAX_PAGES...
Would the NVME driver even work if max_segments was greater than
BIO_MAX_PAGES? Correct me if I'm wrong, but it looks like
blk_rq_map_sg() will only map one bio within a request. So there has to
be one bio per request by the time it hits nvme_map_data()...
So I'm not really sure how we could even construct a valid passthrough
request in nvmet_passthru_map_sg() that is larger than BIO_MAX_PAGES.
If you want me to send a patch to future proof the MDTS limit with
BIO_MAX_PAGES, I can do that, but it doesn't look like it will have any
effect right now unless big things change.
Logan
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-15 18:40 ` Logan Gunthorpe
@ 2020-10-16 13:57 ` Christoph Hellwig
2020-10-16 16:49 ` Logan Gunthorpe
0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2020-10-16 13:57 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Sagi Grimberg, Chaitanya Kulkarni, Christoph Hellwig, linux-nvme,
Douglas Gilbert
On Thu, Oct 15, 2020 at 12:40:52PM -0600, Logan Gunthorpe wrote:
> > But those are entirely unrelated to the bio size. BIO_MAX_PAGES is
> > 256, so with 4k pages and assuming none can't be merged that is 1MB,
> > while max_segments/max_hw_sectors could be something much larger.
>
> Isn't it constrained by max_segments which is set to NVME_MAX_SEGS=127
> (for PCI)... less than BIO_MAX_PAGES...
nvmet-passthrough is not limited to work on PCIe controllers. And even
if it did relying on an implicit limit like that is a time bomb. I
have work pending for one of the next merge windows to lift that limit
for example, and I would not have thought that nvmet-passthrough
relies on it in any way.
> Would the NVME driver even work if max_segments was greater than
> BIO_MAX_PAGES? Correct me if I'm wrong, but it looks like
> blk_rq_map_sg() will only map one bio within a request. So there has to
> be one bio per request by the time it hits nvme_map_data()...
blk_rq_map_sg maps the bio chain. Take a look at the for_each_bio
loop in __blk_bios_map_sg.
> If you want me to send a patch to future proof the MDTS limit with
> BIO_MAX_PAGES, I can do that, but it doesn't look like it will have any
> effect right now unless big things change.
Yes, please do.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()
2020-10-16 13:57 ` Christoph Hellwig
@ 2020-10-16 16:49 ` Logan Gunthorpe
0 siblings, 0 replies; 14+ messages in thread
From: Logan Gunthorpe @ 2020-10-16 16:49 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Chaitanya Kulkarni, Sagi Grimberg, linux-nvme, Douglas Gilbert
On 2020-10-16 7:57 a.m., Christoph Hellwig wrote:
> On Thu, Oct 15, 2020 at 12:40:52PM -0600, Logan Gunthorpe wrote:
> blk_rq_map_sg maps the bio chain. Take a look at the for_each_bio
> loop in __blk_bios_map_sg.
Ah, yes, ok... I originally had a loop that created a chain of bios but
used bio_add_page() instead of bio_add_pc_page(). This was combined with
a separate check for max_segments and max_hw_sectors. When I switched to
bio_add_pc_page() to drop the separate check, it no longer made sense to
do the chain because there was no way to tell if bio_add_pc_page()
failed because of bio_full() or because it exceeded
max_segments/hw_sectors. If the bio was full, we'd want to create the
chain, but if it exceeded the maximums we'd want to error out...
>> If you want me to send a patch to future proof the MDTS limit with
>> BIO_MAX_PAGES, I can do that, but it doesn't look like it will have any
>> effect right now unless big things change.
>
> Yes, please do.
Will do.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-10-16 16:49 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-09 23:18 [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg() Logan Gunthorpe
2020-10-13 22:26 ` Sagi Grimberg
2020-10-13 22:38 ` Douglas Gilbert
2020-10-14 0:16 ` Chaitanya Kulkarni
2020-10-14 0:20 ` Logan Gunthorpe
2020-10-14 0:25 ` Chaitanya Kulkarni
2020-10-14 15:47 ` Logan Gunthorpe
2020-10-15 7:56 ` Christoph Hellwig
2020-10-15 16:01 ` Logan Gunthorpe
2020-10-15 17:24 ` Douglas Gilbert
2020-10-15 18:01 ` Christoph Hellwig
2020-10-15 18:40 ` Logan Gunthorpe
2020-10-16 13:57 ` Christoph Hellwig
2020-10-16 16:49 ` Logan Gunthorpe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).