Patchwork bio: have bio_kmap_irq return the size of mapped data (fwd)

login
register
mail settings
Submitter Jens Axboe
Date Nov. 8, 2017, 3:20 p.m.
Message ID <650d7a0d-f344-155a-5b53-4ce368c4aca4@kernel.dk>
Download mbox | patch
Permalink /patch/10048667/
State Not Applicable, archived
Delegated to: Mike Snitzer
Headers show

Comments

Jens Axboe - Nov. 8, 2017, 3:20 p.m.
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...
Jens Axboe - Nov. 8, 2017, 3:36 p.m.
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.
Christoph Hellwig - Nov. 8, 2017, 6:03 p.m.
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.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Jens Axboe - Nov. 8, 2017, 6:08 p.m.
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.
Christoph Hellwig - Nov. 8, 2017, 6:09 p.m.
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---

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Patch

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.
  *