| Message ID | 20200320121657.1165-16-jinpu.wang@cloud.ionos.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | RTRS (former IBTRS) RDMA Transport Library and RNBD (former IBNBD) RDMA Network Block Device | expand |
On 2020-03-20 05:16, Jack Wang wrote: > To avoid duplicate code in rnbd-srv, we need to reexport > bio_map_kern. > > This reverts commit 00ec4f3039a9e36cbccd1aea82d06c77c440a51c. > > Suggested-by: Bart Van Assche <bvanassche@acm.org> > Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com> > --- > block/bio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/block/bio.c b/block/bio.c > index 94d697217887..9190d68adad7 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -1564,6 +1564,7 @@ struct bio *bio_map_kern(struct request_queue *q, void *data, unsigned int len, > bio->bi_end_io = bio_map_kern_endio; > return bio; > } > +EXPORT_SYMBOL(bio_map_kern); Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Fri, Mar 27, 2020 at 08:58:09PM -0700, Bart Van Assche wrote: > On 2020-03-20 05:16, Jack Wang wrote: > > To avoid duplicate code in rnbd-srv, we need to reexport > > bio_map_kern. NAK. If you need this helper you are doing something wrong. What is the use case where a simple bio_add_page loop won't work?
On 2020-03-28 01:29, Christoph Hellwig wrote: > On Fri, Mar 27, 2020 at 08:58:09PM -0700, Bart Van Assche wrote: >> On 2020-03-20 05:16, Jack Wang wrote: >>> To avoid duplicate code in rnbd-srv, we need to reexport >>> bio_map_kern. > > NAK. If you need this helper you are doing something wrong. What is > the use case where a simple bio_add_page loop won't work? Hi Christoph, There are more users in the Linux kernel of bio_add_pc_page() than only bio_map_kern(), e.g. the SCSI target pass-through code (drivers/target/target_core_pscsi.c). The code that uses bio_map_kern() is in patch 22/26: "block/rnbd: server: functionality for IO submission to file or block dev". Isn't that use case similar to the SCSI pass-through code? I think the RNBD server code also implements storage target functionality. Thanks, Bart.
On Sat, Mar 28, 2020 at 09:16:55AM -0700, Bart Van Assche wrote: > There are more users in the Linux kernel of bio_add_pc_page() than only > bio_map_kern(), e.g. the SCSI target pass-through code > (drivers/target/target_core_pscsi.c). The code that uses bio_map_kern() > is in patch 22/26: "block/rnbd: server: functionality for IO submission > to file or block dev". Isn't that use case similar to the SCSI > pass-through code? I think the RNBD server code also implements storage > target functionality. No, it is not at all. The RNBD case submits normal read/write bios, for which bio_map_kerl is the wrong interfac given that it uses bio_add_pc_page. Read, write and other non-passthrough requests must use bio_add_page instead.
On 03/29/2020 08:05 AM, Christoph Hellwig wrote: > On Sat, Mar 28, 2020 at 09:16:55AM -0700, Bart Van Assche wrote: >> >There are more users in the Linux kernel of bio_add_pc_page() than only >> >bio_map_kern(), e.g. the SCSI target pass-through code >> >(drivers/target/target_core_pscsi.c). The code that uses bio_map_kern() >> >is in patch 22/26: "block/rnbd: server: functionality for IO submission >> >to file or block dev". Isn't that use case similar to the SCSI >> >pass-through code? I think the RNBD server code also implements storage >> >target functionality. > No, it is not at all. The RNBD case submits normal read/write bios, for > which bio_map_kerl is the wrong interfac given that it > uses bio_add_pc_page. Read, write and other non-passthrough requests > must use bio_add_page instead. > Since rw are most common operations, it'd be nice to have a helper function for REQ_OP_[READ|WRITE] to map and submit bio from data buffer with chaining to avoid code duplication in each driver which based on the bio_add_page(). I'd be happy to send a patch for that if that is acceptable.
On Sun, Mar 29, 2020 at 06:08:31PM +0000, Chaitanya Kulkarni wrote: > > which bio_map_kerl is the wrong interfac given that it > > uses bio_add_pc_page. Read, write and other non-passthrough requests > > must use bio_add_page instead. > > > > Since rw are most common operations, it'd be nice to have a helper > function for REQ_OP_[READ|WRITE] to map and submit bio from data buffer > with chaining to avoid code duplication in each driver which based on > the bio_add_page(). > > I'd be happy to send a patch for that if that is acceptable. Well, there aren't a whole lot of driver submitting bios - it's mostly file systems, and those often use shared code and/or have very specific requirements. I've started to factor some reasonably common code into self-contained helpers with recent XFS work: xlog_map_iclog_data and xfs_rw_bdev. Both could probably move to the block layer with a little more work, but we'll have to be careful and actually find enough suitable users.
On Sun, Mar 29, 2020 at 5:05 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Sat, Mar 28, 2020 at 09:16:55AM -0700, Bart Van Assche wrote: > > There are more users in the Linux kernel of bio_add_pc_page() than only > > bio_map_kern(), e.g. the SCSI target pass-through code > > (drivers/target/target_core_pscsi.c). The code that uses bio_map_kern() > > is in patch 22/26: "block/rnbd: server: functionality for IO submission > > to file or block dev". Isn't that use case similar to the SCSI > > pass-through code? I think the RNBD server code also implements storage > > target functionality. > > No, it is not at all. The RNBD case submits normal read/write bios, for > which bio_map_kerl is the wrong interfac given that it > uses bio_add_pc_page. Read, write and other non-passthrough requests > must use bio_add_page instead. Thanks for comments, Christoph. If I understood you correctly, so the right way is to construct bio similar to bio_map_kern, but we have to use bio_add_page instead of bio_add_pc_page? As mentioned by Bart also Chaitanya, looks it's reasonable to add a helper for normal READ|WRITE operation to map and submit bio from the data buffer, but sure can be done later. Thanks!
On 03/30/2020 03:44 AM, Jinpu Wang wrote: > Thanks for comments, Christoph. > If I understood you correctly, so the right way is to construct bio > similar to bio_map_kern, but we have to use bio_add_page instead of > bio_add_pc_page? > > As mentioned by Bart also Chaitanya, looks it's reasonable to add a > helper for normal READ|WRITE operation to map and submit bio from the > data buffer, > but sure can be done later. > Yes helper can be done later. > Thanks!
diff --git a/block/bio.c b/block/bio.c index 94d697217887..9190d68adad7 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1564,6 +1564,7 @@ struct bio *bio_map_kern(struct request_queue *q, void *data, unsigned int len, bio->bi_end_io = bio_map_kern_endio; return bio; } +EXPORT_SYMBOL(bio_map_kern); static void bio_copy_kern_endio(struct bio *bio) {
To avoid duplicate code in rnbd-srv, we need to reexport bio_map_kern. This reverts commit 00ec4f3039a9e36cbccd1aea82d06c77c440a51c. Suggested-by: Bart Van Assche <bvanassche@acm.org> Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com> --- block/bio.c | 1 + 1 file changed, 1 insertion(+)