[PATCH] bio: have bio_kmap_irq return the size of mapped data (fwd)

[PATCH] bio: have bio_kmap_irq return the size of mapped data (fwd)

[Mikulas Patocka]

Hi

I need the function bio_kmap_irq in the driver that I am developing, but 
it doesn't return the size of the mapped data. I've made this patch to fix 
it.

Mikulas


From: Mikulas Patocka <mpatocka@redhat.com>

The function bio_kmap_irq is not usable because it does not return the
size of the mapped data.

Fix bio_kmap_irq and __bio_kmap_irq so that they return the size of
mapped data.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 include/linux/bio.h |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Index: linux-2.6/include/linux/bio.h
===================================================================
--- linux-2.6.orig/include/linux/bio.h
+++ linux-2.6/include/linux/bio.h
@@ -576,14 +576,16 @@ static inline void bvec_kunmap_irq(char
 #endif
 
 static inline char *__bio_kmap_irq(struct bio *bio, struct bvec_iter iter,
-				   unsigned long *flags)
+				   unsigned long *flags, unsigned *size)
 {
-	return bvec_kmap_irq(&bio_iter_iovec(bio, iter), flags);
+	struct bio_vec bv = bio_iter_iovec(bio, iter);
+	*size = bv.bv_len;
+	return bvec_kmap_irq(&bv, flags);
 }
 #define __bio_kunmap_irq(buf, flags)	bvec_kunmap_irq(buf, flags)
 
-#define bio_kmap_irq(bio, flags) \
-	__bio_kmap_irq((bio), (bio)->bi_iter, (flags))
+#define bio_kmap_irq(bio, flags, size) \
+	__bio_kmap_irq((bio), (bio)->bi_iter, (flags), (size))
 #define bio_kunmap_irq(buf,flags)	__bio_kunmap_irq(buf, flags)
 
 /*

Re: [PATCH] bio: have bio_kmap_irq return the size of mapped data (fwd)

[Christoph Hellwig]

On Tue, Nov 07, 2017 at 04:45:17PM -0500, Mikulas Patocka wrote:
> Hi
> 
> I need the function bio_kmap_irq in the driver that I am developing, but 
> it doesn't return the size of the mapped data. I've made this patch to fix 
> it.

To be honest I think we should just remove bio_kmap_irq.  It is currently
unused and assumes there is only a single bvec to map.

I think you're much better off using a proper bvec iterator in your
caller and then call bvec_kmap_irq/bvec_kunmap_irq manually.

Re: [PATCH] bio: have bio_kmap_irq return the size of mapped data (fwd)

[Mikulas Patocka]

On Wed, 8 Nov 2017, Christoph Hellwig wrote:

> On Tue, Nov 07, 2017 at 04:45:17PM -0500, Mikulas Patocka wrote:
> > Hi
> > 
> > I need the function bio_kmap_irq in the driver that I am developing, but 
> > it doesn't return the size of the mapped data. I've made this patch to fix 
> > it.
> 
> To be honest I think we should just remove bio_kmap_irq.  It is currently
> unused and assumes there is only a single bvec to map.

It could be removed from include/linux/bio.h and moved to my driver. But 
if we leave it in bio.h, it could be used by others as well.

bio_kmap_irq can iterate over the whole bio if we use bio_advance on the 
bio.

> I think you're much better off using a proper bvec iterator in your
> caller and then call bvec_kmap_irq/bvec_kunmap_irq manually.

Mikulas

Re: [PATCH] bio: have bio_kmap_irq return the size of mapped data (fwd)

[Christoph Hellwig]

On Wed, Nov 08, 2017 at 07:38:44AM -0500, Mikulas Patocka wrote:
> > To be honest I think we should just remove bio_kmap_irq.  It is currently
> > unused and assumes there is only a single bvec to map.
> 
> It could be removed from include/linux/bio.h and moved to my driver. But 
> if we leave it in bio.h, it could be used by others as well.
> 
> bio_kmap_irq can iterate over the whole bio if we use bio_advance on the 
> bio.

The bio_kmap_irq name implies it maps the whole bio, but it only maps
the current segment.  Now if there were plenty of users and it was
non-trivial we could say we should fix the name and documentation.
But given how trivial it is I'd rather have you open code it to clearly
document what you are doing.

Re: [PATCH] bio: have bio_kmap_irq return the size of mapped data (fwd)

[Jens Axboe]

On 11/08/2017 08:05 AM, Christoph Hellwig wrote:
> On Wed, Nov 08, 2017 at 07:38:44AM -0500, Mikulas Patocka wrote:
>>> To be honest I think we should just remove bio_kmap_irq.  It is currently
>>> unused and assumes there is only a single bvec to map.
>>
>> It could be removed from include/linux/bio.h and moved to my driver. But 
>> if we leave it in bio.h, it could be used by others as well.
>>
>> bio_kmap_irq can iterate over the whole bio if we use bio_advance on the 
>> bio.
> 
> The bio_kmap_irq name implies it maps the whole bio, but it only maps
> the current segment.  Now if there were plenty of users and it was
> non-trivial we could say we should fix the name and documentation.
> But given how trivial it is I'd rather have you open code it to clearly
> document what you are doing.

On top of that, there are no users of it at all...


diff --git a/Documentation/block/biodoc.txt b/Documentation/block/biodoc.txt
index 9490f2845f06..01c0a03407cc 100644
--- a/Documentation/block/biodoc.txt
+++ b/Documentation/block/biodoc.txt
@@ -216,7 +216,7 @@ may need to abort DMA operations and revert to PIO for the transfer, in
 which case a virtual mapping of the page is required. For SCSI it is also
 done in some scenarios where the low level driver cannot be trusted to
 handle a single sg entry correctly. The driver is expected to perform the
-kmaps as needed on such occasions using the __bio_kmap_atomic and bio_kmap_irq
+kmaps as needed on such occasions using the bio_kmap_irq and friends
 routines as appropriate. A driver could also use the blk_queue_bounce()
 routine on its own to bounce highmem i/o to low memory for specific requests
 if so desired.
@@ -1137,7 +1137,7 @@ use dma_map_sg for scatter gather) to be able to ship it to the driver. For
 PIO drivers (or drivers that need to revert to PIO transfer once in a
 while (IDE for example)), where the CPU is doing the actual data
 transfer a virtual mapping is needed. If the driver supports highmem I/O,
-(Sec 1.1, (ii) ) it needs to use __bio_kmap_atomic and bio_kmap_irq to
+(Sec 1.1, (ii) ) it needs to use __bio_kmap_atomic or similar to
 temporarily map a bio into the virtual address space.
 
 
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 9c75f58f6a50..1d7e63d7505f 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -573,17 +573,6 @@ static inline void bvec_kunmap_irq(char *buffer, unsigned long *flags)
 }
 #endif
 
-static inline char *__bio_kmap_irq(struct bio *bio, struct bvec_iter iter,
-				   unsigned long *flags)
-{
-	return bvec_kmap_irq(&bio_iter_iovec(bio, iter), flags);
-}
-#define __bio_kunmap_irq(buf, flags)	bvec_kunmap_irq(buf, flags)
-
-#define bio_kmap_irq(bio, flags) \
-	__bio_kmap_irq((bio), (bio)->bi_iter, (flags))
-#define bio_kunmap_irq(buf,flags)	__bio_kunmap_irq(buf, flags)
-
 /*
  * BIO list management for use by remapping drivers (e.g. DM or MD) and loop.
  *

-- 
Jens Axboe

Re: [dm-devel] [PATCH] bio: have bio_kmap_irq return the size of mapped data (fwd)

[Christoph Hellwig]

On Wed, Nov 08, 2017 at 08:20:58AM -0700, Jens Axboe wrote:
> On top of that, there are no users of it at all...

But Mikulas wants to add one :)

Note that __bio_kmap_atomic has the same issues, only has a single
users either and would be cleaner if opencoded there as it already
has the current bvec.  See patch below.

diff --git a/Documentation/block/biodoc.txt b/Documentation/block/biodoc.txt
index 9490f2845f06..0ec30dc950eb 100644
--- a/Documentation/block/biodoc.txt
+++ b/Documentation/block/biodoc.txt
@@ -216,7 +216,7 @@ may need to abort DMA operations and revert to PIO for the transfer, in
 which case a virtual mapping of the page is required. For SCSI it is also
 done in some scenarios where the low level driver cannot be trusted to
 handle a single sg entry correctly. The driver is expected to perform the
-kmaps as needed on such occasions using the __bio_kmap_atomic and bio_kmap_irq
+kmaps as needed on such occasions using the bio_kmap_irq and friends
 routines as appropriate. A driver could also use the blk_queue_bounce()
 routine on its own to bounce highmem i/o to low memory for specific requests
 if so desired.
@@ -1137,8 +1137,8 @@ use dma_map_sg for scatter gather) to be able to ship it to the driver. For
 PIO drivers (or drivers that need to revert to PIO transfer once in a
 while (IDE for example)), where the CPU is doing the actual data
 transfer a virtual mapping is needed. If the driver supports highmem I/O,
-(Sec 1.1, (ii) ) it needs to use __bio_kmap_atomic and bio_kmap_irq to
-temporarily map a bio into the virtual address space.
+(Sec 1.1, (ii) ) it needs to use kmap_atomic or similar to temporarily map
+a bio into the virtual address space.
 
 
 8. Prior/Related/Impacted patches
diff --git a/arch/xtensa/platforms/iss/simdisk.c b/arch/xtensa/platforms/iss/simdisk.c
index c45b90bb9339..eacf1e433518 100644
--- a/arch/xtensa/platforms/iss/simdisk.c
+++ b/arch/xtensa/platforms/iss/simdisk.c
@@ -110,13 +110,13 @@ static blk_qc_t simdisk_make_request(struct request_queue *q, struct bio *bio)
 	sector_t sector = bio->bi_iter.bi_sector;
 
 	bio_for_each_segment(bvec, bio, iter) {
-		char *buffer = __bio_kmap_atomic(bio, iter);
+		char *buffer = kmap_atomic(bvec.bv_page) + bvec.bv_offset;
 		unsigned len = bvec.bv_len >> SECTOR_SHIFT;
 
 		simdisk_transfer(dev, sector, len, buffer,
 				bio_data_dir(bio) == WRITE);
 		sector += len;
-		__bio_kunmap_atomic(buffer);
+		kunmap_atomic(buffer)
 	}
 
 	bio_endio(bio);
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 8559e9563c52..48ebe6be07b7 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -157,7 +157,7 @@ EXPORT_SYMBOL(blk_set_stacking_limits);
  * Caveat:
  *    The driver that does this *must* be able to deal appropriately
  *    with buffers in "highmemory". This can be accomplished by either calling
- *    __bio_kmap_atomic() to get a temporary kernel mapping, or by calling
+ *    kmap_atomic() to get a temporary kernel mapping, or by calling
  *    blk_queue_bounce() to create a buffer in normal memory.
  **/
 void blk_queue_make_request(struct request_queue *q, make_request_fn *mfn)
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 275c91c99516..65f613612fb0 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -128,18 +128,6 @@ static inline void *bio_data(struct bio *bio)
  */
 #define bvec_to_phys(bv)	(page_to_phys((bv)->bv_page) + (unsigned long) (bv)->bv_offset)
 
-/*
- * queues that have highmem support enabled may still need to revert to
- * PIO transfers occasionally and thus map high pages temporarily. For
- * permanent PIO fall back, user is probably better off disabling highmem
- * I/O completely on that queue (see ide-dma for example)
- */
-#define __bio_kmap_atomic(bio, iter)				\
-	(kmap_atomic(bio_iter_iovec((bio), (iter)).bv_page) +	\
-		bio_iter_iovec((bio), (iter)).bv_offset)
-
-#define __bio_kunmap_atomic(addr)	kunmap_atomic(addr)
-
 /*
  * merge helpers etc
  */
@@ -575,17 +563,6 @@ static inline void bvec_kunmap_irq(char *buffer, unsigned long *flags)
 }
 #endif
 
-static inline char *__bio_kmap_irq(struct bio *bio, struct bvec_iter iter,
-				   unsigned long *flags)
-{
-	return bvec_kmap_irq(&bio_iter_iovec(bio, iter), flags);
-}
-#define __bio_kunmap_irq(buf, flags)	bvec_kunmap_irq(buf, flags)
-
-#define bio_kmap_irq(bio, flags) \
-	__bio_kmap_irq((bio), (bio)->bi_iter, (flags))
-#define bio_kunmap_irq(buf,flags)	__bio_kunmap_irq(buf, flags)
-
 /*
  * BIO list management for use by remapping drivers (e.g. DM or MD) and loop.
  *

Re: [dm-devel] [PATCH] bio: have bio_kmap_irq return the size of mapped data (fwd)

[Jens Axboe]

On 11/08/2017 08:34 AM, Christoph Hellwig wrote:
> On Wed, Nov 08, 2017 at 08:20:58AM -0700, Jens Axboe wrote:
>> On top of that, there are no users of it at all...
> 
> But Mikulas wants to add one :)

I know...

> Note that __bio_kmap_atomic has the same issues, only has a single
> users either and would be cleaner if opencoded there as it already
> has the current bvec.  See patch below.

Kill them all with fire. The open coded version is much clearer.

-- 
Jens Axboe

Re: [dm-devel] [PATCH] bio: have bio_kmap_irq return the size of mapped data (fwd)

[Christoph Hellwig]

On Wed, Nov 08, 2017 at 08:36:25AM -0700, Jens Axboe wrote:
> On 11/08/2017 08:34 AM, Christoph Hellwig wrote:
> > On Wed, Nov 08, 2017 at 08:20:58AM -0700, Jens Axboe wrote:
> >> On top of that, there are no users of it at all...
> > 
> > But Mikulas wants to add one :)
> 
> I know...
> 
> > Note that __bio_kmap_atomic has the same issues, only has a single
> > users either and would be cleaner if opencoded there as it already
> > has the current bvec.  See patch below.
> 
> Kill them all with fire. The open coded version is much clearer.

Feel free to throw the patch in under either your or my name.

Re: [dm-devel] [PATCH] bio: have bio_kmap_irq return the size of mapped data (fwd)

[Jens Axboe]

On 11/08/2017 11:03 AM, Christoph Hellwig wrote:
> On Wed, Nov 08, 2017 at 08:36:25AM -0700, Jens Axboe wrote:
>> On 11/08/2017 08:34 AM, Christoph Hellwig wrote:
>>> On Wed, Nov 08, 2017 at 08:20:58AM -0700, Jens Axboe wrote:
>>>> On top of that, there are no users of it at all...
>>>
>>> But Mikulas wants to add one :)
>>
>> I know...
>>
>>> Note that __bio_kmap_atomic has the same issues, only has a single
>>> users either and would be cleaner if opencoded there as it already
>>> has the current bvec.  See patch below.
>>
>> Kill them all with fire. The open coded version is much clearer.
> 
> Feel free to throw the patch in under either your or my name.

I'll do a combo. Just checking if it's OK I add your signed-off-by
to the patch, I don't want something with you as an author, but
not having an SOB.

-- 
Jens Axboe

Re: [dm-devel] [PATCH] bio: have bio_kmap_irq return the size of mapped data (fwd)

[Christoph Hellwig]

On Wed, Nov 08, 2017 at 11:08:13AM -0700, Jens Axboe wrote:
> On 11/08/2017 11:03 AM, Christoph Hellwig wrote:
> > On Wed, Nov 08, 2017 at 08:36:25AM -0700, Jens Axboe wrote:
> >> On 11/08/2017 08:34 AM, Christoph Hellwig wrote:
> >>> On Wed, Nov 08, 2017 at 08:20:58AM -0700, Jens Axboe wrote:
> >>>> On top of that, there are no users of it at all...
> >>>
> >>> But Mikulas wants to add one :)
> >>
> >> I know...
> >>
> >>> Note that __bio_kmap_atomic has the same issues, only has a single
> >>> users either and would be cleaner if opencoded there as it already
> >>> has the current bvec.  See patch below.
> >>
> >> Kill them all with fire. The open coded version is much clearer.
> > 
> > Feel free to throw the patch in under either your or my name.
> 
> I'll do a combo. Just checking if it's OK I add your signed-off-by
> to the patch, I don't want something with you as an author, but
> not having an SOB.

sure:

Reviewed-by: Christoph Hellwig <hch@lst.de>

for my incremental bio_kmap_atomic removal.  But I can also just
send a correct patch, give me a few minutes.

> 
> -- 
> Jens Axboe
> 
---end quoted text---