| Message ID | 1347322957-25260-16-git-send-email-koverstreet@google.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
Hello, On Mon, Sep 10, 2012 at 05:22:26PM -0700, Kent Overstreet wrote: > +void bio_copy_data(struct bio *dst, struct bio *src) > +{ ... > + src_p = kmap_atomic(src_bv->bv_page); > + dst_p = kmap_atomic(dst_bv->bv_page); > + > + memcpy(dst_p + dst_bv->bv_offset, > + src_p + src_bv->bv_offset, > + bytes); > + > + kunmap_atomic(dst_p); > + kunmap_atomic(src_p); Wrap these in preempt_disable/enable() to allow the function to be called from any context? Thanks.
On Thu, Sep 20, 2012 at 05:06:32PM -0700, Tejun Heo wrote: > Hello, > > On Mon, Sep 10, 2012 at 05:22:26PM -0700, Kent Overstreet wrote: > > +void bio_copy_data(struct bio *dst, struct bio *src) > > +{ > ... > > + src_p = kmap_atomic(src_bv->bv_page); > > + dst_p = kmap_atomic(dst_bv->bv_page); > > + > > + memcpy(dst_p + dst_bv->bv_offset, > > + src_p + src_bv->bv_offset, > > + bytes); > > + > > + kunmap_atomic(dst_p); > > + kunmap_atomic(src_p); > > Wrap these in preempt_disable/enable() to allow the function to be > called from any context? I checked the implementation of kmap_atomic(), it already does preempt_disable() so it's safe in process context - if I understand correctly it needs local_irq_save()/restore() to be safe in any context and I figured calling this from irq context is not the norm so that should be the caller's responsibility. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, Sep 20, 2012 at 05:06:32PM -0700, Tejun Heo wrote: > Hello, > > On Mon, Sep 10, 2012 at 05:22:26PM -0700, Kent Overstreet wrote: > > +void bio_copy_data(struct bio *dst, struct bio *src) > > +{ > ... > > + src_p = kmap_atomic(src_bv->bv_page); > > + dst_p = kmap_atomic(dst_bv->bv_page); > > + > > + memcpy(dst_p + dst_bv->bv_offset, > > + src_p + src_bv->bv_offset, > > + bytes); > > + > > + kunmap_atomic(dst_p); > > + kunmap_atomic(src_p); > > Wrap these in preempt_disable/enable() to allow the function to be > called from any context? Ooh, and maybe return the amount of copied data?
On Thu, Sep 20, 2012 at 05:09:47PM -0700, Tejun Heo wrote: > On Thu, Sep 20, 2012 at 05:06:32PM -0700, Tejun Heo wrote: > > Hello, > > > > On Mon, Sep 10, 2012 at 05:22:26PM -0700, Kent Overstreet wrote: > > > +void bio_copy_data(struct bio *dst, struct bio *src) > > > +{ > > ... > > > + src_p = kmap_atomic(src_bv->bv_page); > > > + dst_p = kmap_atomic(dst_bv->bv_page); > > > + > > > + memcpy(dst_p + dst_bv->bv_offset, > > > + src_p + src_bv->bv_offset, > > > + bytes); > > > + > > > + kunmap_atomic(dst_p); > > > + kunmap_atomic(src_p); > > > > Wrap these in preempt_disable/enable() to allow the function to be > > called from any context? > > Ooh, and maybe return the amount of copied data? Possibly, but I think I want to wait until a user needs it before adding something like that. >From looking at other code that copies bio data, a parameter that specifies the amount of data to be copied might be more useful. I'm not sure I've seen all the places where bio data is copied yet, so I've just been waiting until I find more uses to make it do more. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, Sep 20, 2012 at 05:09:45PM -0700, Kent Overstreet wrote: > On Thu, Sep 20, 2012 at 05:06:32PM -0700, Tejun Heo wrote: > > Hello, > > > > On Mon, Sep 10, 2012 at 05:22:26PM -0700, Kent Overstreet wrote: > > > +void bio_copy_data(struct bio *dst, struct bio *src) > > > +{ > > ... > > > + src_p = kmap_atomic(src_bv->bv_page); > > > + dst_p = kmap_atomic(dst_bv->bv_page); > > > + > > > + memcpy(dst_p + dst_bv->bv_offset, > > > + src_p + src_bv->bv_offset, > > > + bytes); > > > + > > > + kunmap_atomic(dst_p); > > > + kunmap_atomic(src_p); > > > > Wrap these in preempt_disable/enable() to allow the function to be > > called from any context? > > I checked the implementation of kmap_atomic(), it already does > preempt_disable() so it's safe in process context - if I understand > correctly it needs local_irq_save()/restore() to be safe in any context > and I figured calling this from irq context is not the norm so that > should be the caller's responsibility. Ooh, that means the patch I just sent Andrew about sg_mapping_iter is still too strict. Thanks.
diff --git a/fs/bio.c b/fs/bio.c index 1342a16..7fb9f4e 100644 --- a/fs/bio.c +++ b/fs/bio.c @@ -827,6 +827,76 @@ void bio_advance(struct bio *bio, unsigned bytes) } EXPORT_SYMBOL(bio_advance); +/** + * bio_copy_data - copy contents of data buffers from one chain of bios to + * another + * @src: source bio list + * @dst: destination bio list + * + * If @src and @dst are single bios, bi_next must be NULL - otherwise, treats + * @src and @dst as linked lists of bios. + * + * Stops when it reaches the end of either @src or @dst - that is, copies + * min(src->bi_size, dst->bi_size) bytes (or the equivalent for lists of bios). + */ +void bio_copy_data(struct bio *dst, struct bio *src) +{ + struct bio_vec *src_bv, *dst_bv; + unsigned src_offset, dst_offset, bytes; + void *src_p, *dst_p; + + src_bv = bio_iovec(src); + dst_bv = bio_iovec(dst); + + src_offset = src_bv->bv_offset; + dst_offset = dst_bv->bv_offset; + + while (1) { + if (src_offset == src_bv->bv_offset + src_bv->bv_len) { + src_bv++; + if (src_bv == bio_iovec_idx(src, src->bi_vcnt)) { + src = src->bi_next; + if (!src) + break; + + src_bv = bio_iovec(src); + } + + src_offset = src_bv->bv_offset; + } + + if (dst_offset == dst_bv->bv_offset + dst_bv->bv_len) { + dst_bv++; + if (dst_bv == bio_iovec_idx(dst, dst->bi_vcnt)) { + dst = dst->bi_next; + if (!dst) + break; + + dst_bv = bio_iovec(dst); + } + + dst_offset = dst_bv->bv_offset; + } + + bytes = min(dst_bv->bv_offset + dst_bv->bv_len - dst_offset, + src_bv->bv_offset + src_bv->bv_len - src_offset); + + src_p = kmap_atomic(src_bv->bv_page); + dst_p = kmap_atomic(dst_bv->bv_page); + + memcpy(dst_p + dst_bv->bv_offset, + src_p + src_bv->bv_offset, + bytes); + + kunmap_atomic(dst_p); + kunmap_atomic(src_p); + + src_offset += bytes; + dst_offset += bytes; + } +} +EXPORT_SYMBOL(bio_copy_data); + struct bio_map_data { struct bio_vec *iovecs; struct sg_iovec *sgvecs; diff --git a/include/linux/bio.h b/include/linux/bio.h index 949c48a..92015ce 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -286,6 +286,8 @@ static inline void bio_flush_dcache_pages(struct bio *bi) } #endif +extern void bio_copy_data(struct bio *dst, struct bio *src); + extern struct bio *bio_copy_user(struct request_queue *, struct rq_map_data *, unsigned long, unsigned int, int, gfp_t); extern struct bio *bio_copy_user_iov(struct request_queue *,
This gets open coded quite a bit and it's tricky to get right, so make a generic version and convert some existing users over to it instead. Signed-off-by: Kent Overstreet <koverstreet@google.com> CC: Jens Axboe <axboe@kernel.dk> --- fs/bio.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/bio.h | 2 ++ 2 files changed, 72 insertions(+)