diff mbox series

[v11,15/26] block: reexport bio_map_kern

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

Commit Message

Jinpu Wang March 20, 2020, 12:16 p.m. UTC
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(+)

Comments

Bart Van Assche March 28, 2020, 3:58 a.m. UTC | #1
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>
Christoph Hellwig March 28, 2020, 8:29 a.m. UTC | #2
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?
Bart Van Assche March 28, 2020, 4:16 p.m. UTC | #3
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.
Christoph Hellwig March 29, 2020, 3:05 p.m. UTC | #4
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.
Chaitanya Kulkarni March 29, 2020, 6:08 p.m. UTC | #5
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.
Christoph Hellwig March 30, 2020, 6:28 a.m. UTC | #6
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.
Jinpu Wang March 30, 2020, 10:44 a.m. UTC | #7
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!
Chaitanya Kulkarni March 30, 2020, 6:57 p.m. UTC | #8
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 mbox series

Patch

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)
 {