[PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Logan Gunthorpe]

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

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Douglas Gilbert]

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

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Chaitanya Kulkarni]

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

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Logan Gunthorpe]

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

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Chaitanya Kulkarni]

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

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Logan Gunthorpe]

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

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Christoph Hellwig]

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

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Logan Gunthorpe]

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

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Douglas Gilbert]

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

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Christoph Hellwig]

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

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Logan Gunthorpe]

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

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Christoph Hellwig]

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

Re: [PATCH] nvmet-passthru: Cleanup nvmet_passthru_map_sg()

[Logan Gunthorpe]

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