From: Max Gurtovoy <maxg@mellanox.com> To: Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk> Cc: Sagi Grimberg <sagi@grimberg.me>, <linux-block@vger.kernel.org>, <martin.petersen@oracle.com>, <linux-nvme@lists.infradead.org>, <keith.busch@intel.com>, <shlomin@mellanox.com>, <israelr@mellanox.com> Subject: Re: [PATCH 1/4] block: centrelize PI remapping logic to the block layer Date: Wed, 4 Sep 2019 11:32:04 +0300 [thread overview] Message-ID: <fd70d115-bb29-a8a7-83ae-7e3dcaa1dc1c@mellanox.com> (raw) In-Reply-To: <20190904054956.GA10553@lst.de> On 9/4/2019 8:49 AM, Christoph Hellwig wrote: > On Tue, Sep 03, 2019 at 01:21:59PM -0600, Jens Axboe wrote: >> On 9/3/19 1:11 PM, Sagi Grimberg wrote: >>>> + if (blk_integrity_rq(req) && req_op(req) == REQ_OP_READ && >>>> + error == BLK_STS_OK) >>>> + t10_pi_complete(req, >>>> + nr_bytes / queue_logical_block_size(req->q)); >>>> + >>> div in this path? better to use >> ilog2(block_size). >>> >>> Also, would be better to have a wrapper in place like: >>> >>> static inline unsigned short blk_integrity_interval(struct request *rq) >>> { >>> return queue_logical_block_size(rq->q); >>> } >> If it's a hot path thing that matters, I'd strongly suggest to add >> a queue block size shift instead. > Make that a protection_interval_shift, please. While that currently > is the same as the logical block size the concepts are a little > different, and that makes it clear. Except for that this patch looks > very nice to me, it is great to avoid having drivers to deal with the > PI remapping. Christoph, I was thinking about the following addition to the code (combination of all the suggestions): diff --git a/block/blk-core.c b/block/blk-core.c index 58ecfd3..cef604c 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1409,7 +1409,7 @@ bool blk_update_request(struct request *req, blk_status_t error, if (blk_integrity_rq(req) && req_op(req) == REQ_OP_READ && error == BLK_STS_OK) t10_pi_complete(req, - nr_bytes / queue_logical_block_size(req->q)); + nr_bytes >> queue_logical_block_shift(req->q)); if (unlikely(error && !blk_rq_is_passthrough(req) && !(req->rq_flags & RQF_QUIET))) diff --git a/block/blk-settings.c b/block/blk-settings.c index 2c18312..8183ffc 100644 --- a/block/blk-settings.c +++ b/block/blk-settings.c @@ -330,6 +330,7 @@ void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size) void blk_queue_logical_block_size(struct request_queue *q, unsigned short size) { q->limits.logical_block_size = size; + q->limits.logical_block_shift = ilog2(size); if (q->limits.physical_block_size < size) q->limits.physical_block_size = size; diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 1ef375d..4a0115e 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -332,6 +332,7 @@ struct queue_limits { unsigned int discard_alignment; unsigned short logical_block_size; + unsigned short logical_block_shift; unsigned short max_segments; unsigned short max_integrity_segments; unsigned short max_discard_segments; @@ -1267,6 +1268,16 @@ static inline unsigned int queue_max_segment_size(struct request_queue *q) return q->limits.max_segment_size; } +static inline unsigned short queue_logical_block_shift(struct request_queue *q) +{ + unsigned short retval = 9; + + if (q && q->limits.logical_block_shift) + retval = q->limits.logical_block_shift; + + return retval; +} + static inline unsigned short queue_logical_block_size(struct request_queue *q)
WARNING: multiple messages have this Message-ID (diff)
From: Max Gurtovoy <maxg@mellanox.com> To: Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk> Cc: keith.busch@intel.com, Sagi Grimberg <sagi@grimberg.me>, martin.petersen@oracle.com, israelr@mellanox.com, linux-nvme@lists.infradead.org, linux-block@vger.kernel.org, shlomin@mellanox.com Subject: Re: [PATCH 1/4] block: centrelize PI remapping logic to the block layer Date: Wed, 4 Sep 2019 11:32:04 +0300 [thread overview] Message-ID: <fd70d115-bb29-a8a7-83ae-7e3dcaa1dc1c@mellanox.com> (raw) In-Reply-To: <20190904054956.GA10553@lst.de> On 9/4/2019 8:49 AM, Christoph Hellwig wrote: > On Tue, Sep 03, 2019 at 01:21:59PM -0600, Jens Axboe wrote: >> On 9/3/19 1:11 PM, Sagi Grimberg wrote: >>>> + if (blk_integrity_rq(req) && req_op(req) == REQ_OP_READ && >>>> + error == BLK_STS_OK) >>>> + t10_pi_complete(req, >>>> + nr_bytes / queue_logical_block_size(req->q)); >>>> + >>> div in this path? better to use >> ilog2(block_size). >>> >>> Also, would be better to have a wrapper in place like: >>> >>> static inline unsigned short blk_integrity_interval(struct request *rq) >>> { >>> return queue_logical_block_size(rq->q); >>> } >> If it's a hot path thing that matters, I'd strongly suggest to add >> a queue block size shift instead. > Make that a protection_interval_shift, please. While that currently > is the same as the logical block size the concepts are a little > different, and that makes it clear. Except for that this patch looks > very nice to me, it is great to avoid having drivers to deal with the > PI remapping. Christoph, I was thinking about the following addition to the code (combination of all the suggestions): diff --git a/block/blk-core.c b/block/blk-core.c index 58ecfd3..cef604c 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1409,7 +1409,7 @@ bool blk_update_request(struct request *req, blk_status_t error, if (blk_integrity_rq(req) && req_op(req) == REQ_OP_READ && error == BLK_STS_OK) t10_pi_complete(req, - nr_bytes / queue_logical_block_size(req->q)); + nr_bytes >> queue_logical_block_shift(req->q)); if (unlikely(error && !blk_rq_is_passthrough(req) && !(req->rq_flags & RQF_QUIET))) diff --git a/block/blk-settings.c b/block/blk-settings.c index 2c18312..8183ffc 100644 --- a/block/blk-settings.c +++ b/block/blk-settings.c @@ -330,6 +330,7 @@ void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size) void blk_queue_logical_block_size(struct request_queue *q, unsigned short size) { q->limits.logical_block_size = size; + q->limits.logical_block_shift = ilog2(size); if (q->limits.physical_block_size < size) q->limits.physical_block_size = size; diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 1ef375d..4a0115e 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -332,6 +332,7 @@ struct queue_limits { unsigned int discard_alignment; unsigned short logical_block_size; + unsigned short logical_block_shift; unsigned short max_segments; unsigned short max_integrity_segments; unsigned short max_discard_segments; @@ -1267,6 +1268,16 @@ static inline unsigned int queue_max_segment_size(struct request_queue *q) return q->limits.max_segment_size; } +static inline unsigned short queue_logical_block_shift(struct request_queue *q) +{ + unsigned short retval = 9; + + if (q && q->limits.logical_block_shift) + retval = q->limits.logical_block_shift; + + return retval; +} + static inline unsigned short queue_logical_block_size(struct request_queue *q) _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2019-09-04 8:32 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-09-03 15:14 [PATCH 1/4] block: centrelize PI remapping logic to the block layer Max Gurtovoy 2019-09-03 15:14 ` Max Gurtovoy 2019-09-03 15:14 ` [PATCH 2/4] nvme-rdma: simplify error flow in nvme_rdma_queue_rq Max Gurtovoy 2019-09-03 15:14 ` Max Gurtovoy 2019-09-04 5:51 ` Christoph Hellwig 2019-09-04 5:51 ` Christoph Hellwig 2019-09-03 15:14 ` [PATCH 3/4] nvme-tcp: introduce nvme_tcp_complete_rq callback Max Gurtovoy 2019-09-03 15:14 ` Max Gurtovoy 2019-09-03 19:15 ` Sagi Grimberg 2019-09-03 19:15 ` Sagi Grimberg 2019-09-04 5:54 ` Christoph Hellwig 2019-09-04 5:54 ` Christoph Hellwig 2019-09-04 9:02 ` Max Gurtovoy 2019-09-04 9:02 ` Max Gurtovoy 2019-09-03 15:14 ` [PATCH 4/4] nvmet-loop: fix possible leakage during error flow Max Gurtovoy 2019-09-03 15:14 ` Max Gurtovoy 2019-09-04 5:50 ` Christoph Hellwig 2019-09-04 5:50 ` Christoph Hellwig 2019-09-03 19:11 ` [PATCH 1/4] block: centrelize PI remapping logic to the block layer Sagi Grimberg 2019-09-03 19:11 ` Sagi Grimberg 2019-09-03 19:21 ` Jens Axboe 2019-09-03 19:21 ` Jens Axboe 2019-09-04 5:49 ` Christoph Hellwig 2019-09-04 5:49 ` Christoph Hellwig 2019-09-04 8:32 ` Max Gurtovoy [this message] 2019-09-04 8:32 ` Max Gurtovoy 2019-09-04 12:49 ` Christoph Hellwig 2019-09-04 12:49 ` Christoph Hellwig
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=fd70d115-bb29-a8a7-83ae-7e3dcaa1dc1c@mellanox.com \ --to=maxg@mellanox.com \ --cc=axboe@kernel.dk \ --cc=hch@lst.de \ --cc=israelr@mellanox.com \ --cc=keith.busch@intel.com \ --cc=linux-block@vger.kernel.org \ --cc=linux-nvme@lists.infradead.org \ --cc=martin.petersen@oracle.com \ --cc=sagi@grimberg.me \ --cc=shlomin@mellanox.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.