Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
* [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	[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, back to index

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

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git