Message ID | 4A3075B2.9040208@ct.jp.nec.com (mailing list archive) |
---|---|
State | Accepted, archived |
Headers | show |
On Thu, Jun 11 2009, Kiyoshi Ueda wrote: > Hi Jens, > > This patch adds the following 2 interfaces for request-stacking drivers: > > - blk_rq_prep_clone(struct request *clone, struct request *orig, > struct bio_set *bs, gfp_t gfp_mask, > int (*bio_ctr)(struct bio *, struct bio*, void *), > void *data) > * Clones bios in the original request to the clone request > (bio_ctr is called for each cloned bios.) > * Copies attributes of the original request to the clone request. > The actual data parts (e.g. ->cmd, ->buffer, ->sense) are not > copied. > > - blk_rq_unprep_clone(struct request *clone) > * Frees cloned bios from the clone request. > > Request stacking drivers (e.g. request-based dm) need to make a clone > request for a submitted request and dispatch it to other devices. > > To allocate request for the clone, request stacking drivers may not > be able to use blk_get_request() because the allocation may be done > in an irq-disabled context. > So blk_rq_prep_clone() takes a request allocated by the caller > as an argument. > > For each clone bio in the clone request, request stacking drivers > should be able to set up their own completion handler. > So blk_rq_prep_clone() takes a callback function which is called > for each clone bio, and a pointer for private data which is passed > to the callback. > > NOTE: > blk_rq_prep_clone() doesn't copy any actual data of the original > request. Pages are shared between original bios and cloned bios. > So caller must not complete the original request before the clone > request. This looks good to me now, I've applied it.
Jens Axboe <jens.axboe@oracle.com> writes: > On Thu, Jun 11 2009, Kiyoshi Ueda wrote: >> Hi Jens, >> >> This patch adds the following 2 interfaces for request-stacking drivers: >> >> - blk_rq_prep_clone(struct request *clone, struct request *orig, >> struct bio_set *bs, gfp_t gfp_mask, >> int (*bio_ctr)(struct bio *, struct bio*, void *), >> void *data) >> * Clones bios in the original request to the clone request >> (bio_ctr is called for each cloned bios.) >> * Copies attributes of the original request to the clone request. >> The actual data parts (e.g. ->cmd, ->buffer, ->sense) are not >> copied. >> >> - blk_rq_unprep_clone(struct request *clone) >> * Frees cloned bios from the clone request. >> >> Request stacking drivers (e.g. request-based dm) need to make a clone >> request for a submitted request and dispatch it to other devices. >> >> To allocate request for the clone, request stacking drivers may not >> be able to use blk_get_request() because the allocation may be done >> in an irq-disabled context. >> So blk_rq_prep_clone() takes a request allocated by the caller >> as an argument. >> >> For each clone bio in the clone request, request stacking drivers >> should be able to set up their own completion handler. >> So blk_rq_prep_clone() takes a callback function which is called >> for each clone bio, and a pointer for private data which is passed >> to the callback. >> >> NOTE: >> blk_rq_prep_clone() doesn't copy any actual data of the original >> request. Pages are shared between original bios and cloned bios. >> So caller must not complete the original request before the clone >> request. > > This looks good to me now, I've applied it. Is blk_rq_unprep_clone really the best name? ^^^^^^ Cheers, Jeff -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, Jun 11 2009, Jeff Moyer wrote: > Jens Axboe <jens.axboe@oracle.com> writes: > > > On Thu, Jun 11 2009, Kiyoshi Ueda wrote: > >> Hi Jens, > >> > >> This patch adds the following 2 interfaces for request-stacking drivers: > >> > >> - blk_rq_prep_clone(struct request *clone, struct request *orig, > >> struct bio_set *bs, gfp_t gfp_mask, > >> int (*bio_ctr)(struct bio *, struct bio*, void *), > >> void *data) > >> * Clones bios in the original request to the clone request > >> (bio_ctr is called for each cloned bios.) > >> * Copies attributes of the original request to the clone request. > >> The actual data parts (e.g. ->cmd, ->buffer, ->sense) are not > >> copied. > >> > >> - blk_rq_unprep_clone(struct request *clone) > >> * Frees cloned bios from the clone request. > >> > >> Request stacking drivers (e.g. request-based dm) need to make a clone > >> request for a submitted request and dispatch it to other devices. > >> > >> To allocate request for the clone, request stacking drivers may not > >> be able to use blk_get_request() because the allocation may be done > >> in an irq-disabled context. > >> So blk_rq_prep_clone() takes a request allocated by the caller > >> as an argument. > >> > >> For each clone bio in the clone request, request stacking drivers > >> should be able to set up their own completion handler. > >> So blk_rq_prep_clone() takes a callback function which is called > >> for each clone bio, and a pointer for private data which is passed > >> to the callback. > >> > >> NOTE: > >> blk_rq_prep_clone() doesn't copy any actual data of the original > >> request. Pages are shared between original bios and cloned bios. > >> So caller must not complete the original request before the clone > >> request. > > > > This looks good to me now, I've applied it. > > Is blk_rq_unprep_clone really the best name? > ^^^^^^ Probably not, but I'm not very good at coming up with elegant names. Your email should have included a new suggestion :-) FWIW, I don't think the name is THAT bad...
Jens Axboe <jens.axboe@oracle.com> writes: > On Thu, Jun 11 2009, Jeff Moyer wrote: >> Jens Axboe <jens.axboe@oracle.com> writes: >> Is blk_rq_unprep_clone really the best name? >> ^^^^^^ > > Probably not, but I'm not very good at coming up with elegant names. > Your email should have included a new suggestion :-) Fair enough. ;) > - blk_rq_unprep_clone(struct request *clone) > * Frees cloned bios from the clone request. Why not blk_rq_free_clone? -Jeff -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hi Jeff, On 06/12/2009 11:33 PM +0900, Jeff Moyer wrote: > Jens Axboe <jens.axboe@oracle.com> writes: >> On Thu, Jun 11 2009, Jeff Moyer wrote: >>> Jens Axboe <jens.axboe@oracle.com> writes: >>> Is blk_rq_unprep_clone really the best name? >>> ^^^^^^ >> Probably not, but I'm not very good at coming up with elegant names. >> Your email should have included a new suggestion :-) > > Fair enough. ;) > >> - blk_rq_unprep_clone(struct request *clone) >> * Frees cloned bios from the clone request. > > Why not blk_rq_free_clone? Because the 'clone' is not freed in this interface. This interface frees only bios in the 'clone'. Allocating/freeing the 'clone' are the caller's work, since only the caller knows how to allocate/free it. 'prep' after 'alloc' and 'unprep' before 'free' is symmetric and I feel a good candidate for my request-stacking driver, so I chose it. Thanks, Kiyoshi Ueda -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 06/15/2009 06:31 AM, Kiyoshi Ueda wrote: > Hi Jeff, > > On 06/12/2009 11:33 PM +0900, Jeff Moyer wrote: >> Jens Axboe <jens.axboe@oracle.com> writes: >>> On Thu, Jun 11 2009, Jeff Moyer wrote: >>>> Jens Axboe <jens.axboe@oracle.com> writes: >>>> Is blk_rq_unprep_clone really the best name? >>>> ^^^^^^ >>> Probably not, but I'm not very good at coming up with elegant names. >>> Your email should have included a new suggestion :-) >> Fair enough. ;) >> >>> - blk_rq_unprep_clone(struct request *clone) >>> * Frees cloned bios from the clone request. >> Why not blk_rq_free_clone? > > Because the 'clone' is not freed in this interface. > This interface frees only bios in the 'clone'. > Allocating/freeing the 'clone' are the caller's work, since > only the caller knows how to allocate/free it. > > 'prep' after 'alloc' and 'unprep' before 'free' is symmetric > and I feel a good candidate for my request-stacking driver, > so I chose it. > > Thanks, > Kiyoshi Ueda I'm not a native English speaker as well, so I'm fine with blk_rq_{prep,unprep}_clone. But maybe the English people don't like it? Perhaps blk_rq_{clone,declone} or blk_rq_{clone,declone}_bios (Both unclone and declone are found on the net but are not found in the free dictionary) Boaz -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hi Boaz, Jeff, Jens, Thank you for your ideas. It's time to decide now? Please see below. On 2009/06/15 18:30 +0900, Boaz Harrosh wrote: > > On 06/15/2009 06:31 AM, Kiyoshi Ueda wrote: >> >> On 06/12/2009 11:33 PM +0900, Jeff Moyer wrote: >>> >>> Jens Axboe <jens.axboe@oracle.com> writes: >>>> >>>> On Thu, Jun 11 2009, Jeff Moyer wrote: >>>>> >>>>> Jens Axboe <jens.axboe@oracle.com> writes: >>>>> >>>>> Is blk_rq_unprep_clone really the best name? >>>>> >>>>> ^^^^^^ >>>> >>>> Probably not, but I'm not very good at coming up with elegant names. >>>> >>>> Your email should have included a new suggestion :-) >>> >>> Fair enough. ;) >>> >>> >>>> >>>> - blk_rq_unprep_clone(struct request *clone) >>>> >>>> * Frees cloned bios from the clone request. >>> >>> Why not blk_rq_free_clone? >> >> Because the 'clone' is not freed in this interface. >> >> This interface frees only bios in the 'clone'. >> >> Allocating/freeing the 'clone' are the caller's work, since >> >> only the caller knows how to allocate/free it. >> >> >> >> 'prep' after 'alloc' and 'unprep' before 'free' is symmetric >> >> and I feel a good candidate for my request-stacking driver, >> >> so I chose it. > > > > I'm not a native English speaker as well, so I'm fine > > with blk_rq_{prep,unprep}_clone. But maybe the English > > people don't like it? > > > > Perhaps > > blk_rq_{clone,declone} or blk_rq_{clone,declone}_bios > > > > (Both unclone and declone are found on the net but are not > > found in the free dictionary) I had a feeling that blk_rq_{clone,declone} allocates/frees the clone request inside the interfaces like bio_clone(), so I didn't take such namings. And, the clone setup interface may not only make bio clones but also do something else (for other request members), so I didn't add any 'bio' namings to the interfaces. Jens, what do you prefer? Thanks, Kiyoshi Ueda -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 06/16/2009 06:02 AM, Kiyoshi Ueda wrote: > Hi Boaz, Jeff, Jens, >>> Perhaps >>> blk_rq_{clone,declone} or blk_rq_{clone,declone}_bios >>> >>> (Both unclone and declone are found on the net but are not >>> found in the free dictionary) > > I had a feeling that blk_rq_{clone,declone} allocates/frees > the clone request inside the interfaces like bio_clone(), so > I didn't take such namings. > And, the clone setup interface may not only make bio clones > but also do something else (for other request members), so > I didn't add any 'bio' namings to the interfaces. > I'm convinced blk_rq_prep_clone is good for what it actually does > Jens, what do you prefer? > > Thanks, > Kiyoshi Ueda Thanks Boaz -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Kiyoshi Ueda <k-ueda@ct.jp.nec.com> writes: > Hi Boaz, Jeff, Jens, > > Thank you for your ideas. > It's time to decide now? Please see below. > > On 2009/06/15 18:30 +0900, Boaz Harrosh wrote: >> > On 06/15/2009 06:31 AM, Kiyoshi Ueda wrote: >>> >> On 06/12/2009 11:33 PM +0900, Jeff Moyer wrote: >>>> >>> Jens Axboe <jens.axboe@oracle.com> writes: >>>>> >>>> On Thu, Jun 11 2009, Jeff Moyer wrote: >>>>>> >>>>> Jens Axboe <jens.axboe@oracle.com> writes: >>>>>> >>>>> Is blk_rq_unprep_clone really the best name? >>>>>> >>>>> ^^^^^^ >>>>> >>>> Probably not, but I'm not very good at coming up with elegant names. >>>>> >>>> Your email should have included a new suggestion :-) >>>> >>> Fair enough. ;) >>>> >>> >>>>> >>>> - blk_rq_unprep_clone(struct request *clone) >>>>> >>>> * Frees cloned bios from the clone request. >>>> >>> Why not blk_rq_free_clone? >>> >> Because the 'clone' is not freed in this interface. >>> >> This interface frees only bios in the 'clone'. >>> >> Allocating/freeing the 'clone' are the caller's work, since >>> >> only the caller knows how to allocate/free it. >>> >> >>> >> 'prep' after 'alloc' and 'unprep' before 'free' is symmetric >>> >> and I feel a good candidate for my request-stacking driver, >>> >> so I chose it. >> > >> > I'm not a native English speaker as well, so I'm fine >> > with blk_rq_{prep,unprep}_clone. But maybe the English >> > people don't like it? >> > >> > Perhaps >> > blk_rq_{clone,declone} or blk_rq_{clone,declone}_bios >> > >> > (Both unclone and declone are found on the net but are not >> > found in the free dictionary) > > I had a feeling that blk_rq_{clone,declone} allocates/frees > the clone request inside the interfaces like bio_clone(), so > I didn't take such namings. > And, the clone setup interface may not only make bio clones > but also do something else (for other request members), so > I didn't add any 'bio' namings to the interfaces. > > Jens, what do you prefer? I can live with it as it stands. prep/unprep at least has some symmetry. Cheers, Jeff -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Index: linux-2.6-block/block/blk-core.c =================================================================== --- linux-2.6-block.orig/block/blk-core.c +++ linux-2.6-block/block/blk-core.c @@ -2295,6 +2295,106 @@ int blk_lld_busy(struct request_queue *q } EXPORT_SYMBOL_GPL(blk_lld_busy); +/** + * blk_rq_unprep_clone - Helper function to free all bios in a cloned request + * @rq: the clone request to be cleaned up + * + * Description: + * Free all bios in @rq for a cloned request. + */ +void blk_rq_unprep_clone(struct request *rq) +{ + struct bio *bio; + + while ((bio = rq->bio) != NULL) { + rq->bio = bio->bi_next; + + bio_put(bio); + } +} +EXPORT_SYMBOL_GPL(blk_rq_unprep_clone); + +/* + * Copy attributes of the original request to the clone request. + * The actual data parts (e.g. ->cmd, ->buffer, ->sense) are not copied. + */ +static void __blk_rq_prep_clone(struct request *dst, struct request *src) +{ + dst->cpu = src->cpu; + dst->cmd_flags = (rq_data_dir(src) | REQ_NOMERGE); + dst->cmd_type = src->cmd_type; + dst->__sector = blk_rq_pos(src); + dst->__data_len = blk_rq_bytes(src); + dst->nr_phys_segments = src->nr_phys_segments; + dst->ioprio = src->ioprio; + dst->extra_len = src->extra_len; +} + +/** + * blk_rq_prep_clone - Helper function to setup clone request + * @rq: the request to be setup + * @rq_src: original request to be cloned + * @bs: bio_set that bios for clone are allocated from + * @gfp_mask: memory allocation mask for bio + * @bio_ctr: setup function to be called for each clone bio. + * Returns %0 for success, non %0 for failure. + * @data: private data to be passed to @bio_ctr + * + * Description: + * Clones bios in @rq_src to @rq, and copies attributes of @rq_src to @rq. + * The actual data parts of @rq_src (e.g. ->cmd, ->buffer, ->sense) + * are not copied, and copying such parts is the caller's responsibility. + * Also, pages which the original bios are pointing to are not copied + * and the cloned bios just point same pages. + * So cloned bios must be completed before original bios, which means + * the caller must complete @rq before @rq_src. + */ +int blk_rq_prep_clone(struct request *rq, struct request *rq_src, + struct bio_set *bs, gfp_t gfp_mask, + int (*bio_ctr)(struct bio *, struct bio *, void *), + void *data) +{ + struct bio *bio, *bio_src; + + if (!bs) + bs = fs_bio_set; + + blk_rq_init(NULL, rq); + + __rq_for_each_bio(bio_src, rq_src) { + bio = bio_alloc_bioset(gfp_mask, bio_src->bi_max_vecs, bs); + if (!bio) + goto free_and_out; + + __bio_clone(bio, bio_src); + + if (bio_integrity(bio_src) && + bio_integrity_clone(bio, bio_src, gfp_mask)) + goto free_and_out; + + if (bio_ctr && bio_ctr(bio, bio_src, data)) + goto free_and_out; + + if (rq->bio) { + rq->biotail->bi_next = bio; + rq->biotail = bio; + } else + rq->bio = rq->biotail = bio; + } + + __blk_rq_prep_clone(rq, rq_src); + + return 0; + +free_and_out: + if (bio) + bio_free(bio, bs); + blk_rq_unprep_clone(rq); + + return -ENOMEM; +} +EXPORT_SYMBOL_GPL(blk_rq_prep_clone); + int kblockd_schedule_work(struct request_queue *q, struct work_struct *work) { return queue_work(kblockd_workqueue, work); Index: linux-2.6-block/include/linux/blkdev.h =================================================================== --- linux-2.6-block.orig/include/linux/blkdev.h +++ linux-2.6-block/include/linux/blkdev.h @@ -765,6 +765,11 @@ extern void blk_insert_request(struct re extern void blk_requeue_request(struct request_queue *, struct request *); extern int blk_rq_check_limits(struct request_queue *q, struct request *rq); extern int blk_lld_busy(struct request_queue *q); +extern int blk_rq_prep_clone(struct request *rq, struct request *rq_src, + struct bio_set *bs, gfp_t gfp_mask, + int (*bio_ctr)(struct bio *, struct bio *, void *), + void *data); +extern void blk_rq_unprep_clone(struct request *rq); extern int blk_insert_cloned_request(struct request_queue *q, struct request *rq); extern void blk_plug_device(struct request_queue *);