* [PATCH 0/2] bidi support @ 2007-05-05 9:02 FUJITA Tomonori 2007-05-05 9:24 ` FUJITA Tomonori 0 siblings, 1 reply; 4+ messages in thread From: FUJITA Tomonori @ 2007-05-05 9:02 UTC (permalink / raw) To: James.Bottomley, jens.axboe; +Cc: linux-scsi, bhalevy, hch, akpm, michaelc This patchset add bidi support for block pc requests. With Jens' linked request proposal, there aren't many changes to the block layer, just adding one pointer (next_rq) to the request structure. And there is no changes to the existing block-layer functions. A bidi request uses two request structures (one in request and one out request). The in request is linked and the out request is en-queued. We need some changes to the scsi mid-layer. A new structure, scsi_data_buffer is introduced to hold the in-buffer information. With James' comments via IRC, we should not make the scsi_cmnd structure fatter. So the scsi mid-layer uses req->next_rq->special pointer for a scsi_data_buffer structure. LLDs don't touch the second request (req->next_rq) so it's safe to use req->special. There is no change to the scsi_cmnd structure. The rest of changes include sgtable allocation and deallocation for scsi_data_buffer in the block pc request path. I created a new tree, including this patchset and bsg patches: http://git.kernel.org/?p=linux/kernel/git/tomo/linux-2.6-bidi.git;a=summary I modified and put Boaz's open-iscsi bidi patch in the tree so you can test bidi requests via bsg. I'll put the SMP pass through patches in the tree later. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 0/2] bidi support 2007-05-05 9:02 [PATCH 0/2] bidi support FUJITA Tomonori @ 2007-05-05 9:24 ` FUJITA Tomonori 2007-05-07 6:05 ` Benny Halevy 0 siblings, 1 reply; 4+ messages in thread From: FUJITA Tomonori @ 2007-05-05 9:24 UTC (permalink / raw) To: James.Bottomley; +Cc: jens.axboe, linux-scsi, bhalevy, hch, akpm, michaelc From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Subject: [PATCH 0/2] bidi support Date: Sat, 05 May 2007 18:02:29 +0900 > This patchset add bidi support for block pc requests. Oh, this patchset is against Linus' tree. The first patch (the block layer changes) can be applied against Jens' tree. The second patch (the scsi mid-layer changes) has one minor reject against the scsi-misc tree. The following patch is against the scsi-misc. --- >From 6a8c5375f1f6dbd574107920dd0a613527bd556b Mon Sep 17 00:00:00 2001 From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Date: Sat, 5 May 2007 18:11:42 +0900 Subject: [PATCH] add bidi support for block pc requests This adds bidi support for block pc requests. A bidi request uses req->next_rq pointer for an in request. This patch introduces a new structure, scsi_data_buffer to hold the data buffer information. To avoid make scsi_cmnd structure fatter, the scsi mid-layer uses cmnd->request->next_rq->special pointer for a scsi_data_buffer structure. LLDs don't touch the second request (req->next_rq) so it's safe to use req->special. scsi_blk_pc_done() always completes the whole command so scsi_end_request() simply completes the bidi chunk too. A helper function, scsi_bidi_data_buffer() is for LLDs to access to the scsi_data_buffer structure easily. Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> --- drivers/scsi/scsi_lib.c | 120 +++++++++++++++++++++++++++++++++++++++------ include/scsi/scsi_cmnd.h | 14 +++++ 2 files changed, 118 insertions(+), 16 deletions(-) diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index be8e655..8f7873a 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -66,6 +66,12 @@ #undef SP static void scsi_run_queue(struct request_queue *q); +struct scsi_data_buffer *scsi_bidi_data_buffer(struct scsi_cmnd *cmd) +{ + return blk_bidi_rq(cmd->request) ? cmd->request->next_rq->special : NULL; +} +EXPORT_SYMBOL(scsi_bidi_data_buffer); + /* * Function: scsi_unprep_request() * @@ -85,6 +91,7 @@ static void scsi_unprep_request(struct r req->cmd_flags &= ~REQ_DONTPREP; req->special = NULL; + kfree(scsi_bidi_data_buffer(cmd)); scsi_put_command(cmd); } @@ -657,6 +664,7 @@ static struct scsi_cmnd *scsi_end_reques request_queue_t *q = cmd->device->request_queue; struct request *req = cmd->request; unsigned long flags; + struct scsi_data_buffer *sdb = scsi_bidi_data_buffer(cmd); /* * If there are blocks left over at the end, set up the command @@ -685,6 +693,14 @@ static struct scsi_cmnd *scsi_end_reques } } + /* + * a REQ_BLOCK_PC command is always completed fully so just do + * end the bidi chunk. + */ + if (sdb) + end_that_request_chunk(req->next_rq, uptodate, + sdb->request_bufflen); + add_disk_randomness(req->rq_disk); spin_lock_irqsave(q->queue_lock, flags); @@ -701,34 +717,35 @@ static struct scsi_cmnd *scsi_end_reques return NULL; } -struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask) +static struct scatterlist *do_scsi_alloc_sgtable(unsigned short use_sg, + unsigned short *sglist_len, + gfp_t gfp_mask) { struct scsi_host_sg_pool *sgp; - struct scatterlist *sgl; - BUG_ON(!cmd->use_sg); + BUG_ON(!use_sg); - switch (cmd->use_sg) { + switch (use_sg) { case 1 ... 8: - cmd->sglist_len = 0; + *sglist_len = 0; break; case 9 ... 16: - cmd->sglist_len = 1; + *sglist_len = 1; break; case 17 ... 32: - cmd->sglist_len = 2; + *sglist_len = 2; break; #if (SCSI_MAX_PHYS_SEGMENTS > 32) case 33 ... 64: - cmd->sglist_len = 3; + *sglist_len = 3; break; #if (SCSI_MAX_PHYS_SEGMENTS > 64) case 65 ... 128: - cmd->sglist_len = 4; + *sglist_len = 4; break; #if (SCSI_MAX_PHYS_SEGMENTS > 128) case 129 ... 256: - cmd->sglist_len = 5; + *sglist_len = 5; break; #endif #endif @@ -737,11 +754,14 @@ #endif return NULL; } - sgp = scsi_sg_pools + cmd->sglist_len; - sgl = mempool_alloc(sgp->pool, gfp_mask); - return sgl; + sgp = scsi_sg_pools + *sglist_len; + return mempool_alloc(sgp->pool, gfp_mask); } +struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask) +{ + return do_scsi_alloc_sgtable(cmd->use_sg, &cmd->sglist_len, gfp_mask); +} EXPORT_SYMBOL(scsi_alloc_sgtable); void scsi_free_sgtable(struct scatterlist *sgl, int index) @@ -775,6 +795,8 @@ EXPORT_SYMBOL(scsi_free_sgtable); */ static void scsi_release_buffers(struct scsi_cmnd *cmd) { + struct scsi_data_buffer *sdb = scsi_bidi_data_buffer(cmd); + if (cmd->use_sg) scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len); @@ -784,6 +806,13 @@ static void scsi_release_buffers(struct */ cmd->request_buffer = NULL; cmd->request_bufflen = 0; + + if (sdb) { + if (sdb->use_sg) + scsi_free_sgtable(sdb->request_buffer, sdb->sglist_len); + sdb->request_buffer = NULL; + sdb->request_bufflen = 0; + } } /* @@ -834,6 +863,7 @@ void scsi_io_completion(struct scsi_cmnd } if (blk_pc_request(req)) { /* SG_IO ioctl from block level */ + struct scsi_data_buffer *sdb = scsi_bidi_data_buffer(cmd); req->errors = result; if (result) { clear_errors = 0; @@ -850,6 +880,8 @@ void scsi_io_completion(struct scsi_cmnd } } req->data_len = cmd->resid; + if (sdb) + req->next_rq->data_len = sdb->resid; } /* @@ -1075,6 +1107,38 @@ static struct scsi_cmnd *scsi_get_cmd_fr return cmd; } +static int scsi_data_buffer_init(struct scsi_cmnd *cmd) +{ + struct scatterlist *sgpnt; + struct scsi_data_buffer *sdb = scsi_bidi_data_buffer(cmd); + struct request *req = cmd->request; + int count; + + sdb->use_sg = req->next_rq->nr_phys_segments; + sgpnt = do_scsi_alloc_sgtable(sdb->use_sg, &sdb->sglist_len, + GFP_ATOMIC); + if (unlikely(!sgpnt)) { + scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len); + scsi_unprep_request(req); + return BLKPREP_DEFER; + } + + req->buffer = NULL; + sdb->request_buffer = (char *) sgpnt; + sdb->request_bufflen = req->next_rq->data_len; + + count = blk_rq_map_sg(req->q, req->next_rq, sgpnt); + if (likely(count <= sdb->use_sg)) { + sdb->use_sg = count; + return BLKPREP_OK; + } + + scsi_release_buffers(cmd); + scsi_put_command(cmd); + + return BLKPREP_KILL; +} + static void scsi_blk_pc_done(struct scsi_cmnd *cmd) { BUG_ON(!blk_pc_request(cmd->request)); @@ -1090,10 +1154,21 @@ static void scsi_blk_pc_done(struct scsi static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) { struct scsi_cmnd *cmd; + struct scsi_data_buffer *sdb = NULL; + + if (blk_bidi_rq(req)) { + sdb = kzalloc(sizeof(*sdb), GFP_ATOMIC); + if (unlikely(!sdb)) + return BLKPREP_DEFER; + req->next_rq->special = sdb; + } cmd = scsi_get_cmd_from_req(sdev, req); - if (unlikely(!cmd)) + if (unlikely(!cmd)) { + req->next_rq->special = NULL; + kfree(sdb); return BLKPREP_DEFER; + } /* * BLOCK_PC requests may transfer data, in which case they must @@ -1109,6 +1184,12 @@ static int scsi_setup_blk_pc_cmnd(struct ret = scsi_init_io(cmd); if (unlikely(ret)) return ret; + + if (sdb) { + ret = scsi_data_buffer_init(cmd); + if (ret != BLKPREP_OK) + return ret; + } } else { BUG_ON(req->data_len); BUG_ON(req->data); @@ -1122,13 +1203,15 @@ static int scsi_setup_blk_pc_cmnd(struct BUILD_BUG_ON(sizeof(req->cmd) > sizeof(cmd->cmnd)); memcpy(cmd->cmnd, req->cmd, sizeof(cmd->cmnd)); cmd->cmd_len = req->cmd_len; - if (!req->data_len) + if (sdb) + cmd->sc_data_direction = DMA_BIDIRECTIONAL; + else if (!req->data_len) cmd->sc_data_direction = DMA_NONE; else if (rq_data_dir(req) == WRITE) cmd->sc_data_direction = DMA_TO_DEVICE; else cmd->sc_data_direction = DMA_FROM_DEVICE; - + cmd->transfersize = req->data_len; cmd->allowed = req->retries; cmd->timeout_per_command = req->timeout; @@ -1178,6 +1261,11 @@ static int scsi_prep_fn(struct request_q struct scsi_device *sdev = q->queuedata; int ret = BLKPREP_OK; + if (WARN_ON(!blk_pc_request(req) && blk_bidi_rq(req))) { + ret = BLKPREP_KILL; + goto out; + } + /* * If the device is not in running state we will reject some * or all commands. diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index a2e0c10..597c48c 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -28,6 +28,18 @@ struct scsi_pointer { volatile int phase; }; +struct scsi_data_buffer { + unsigned short use_sg; /* Number of pieces of scatter-gather */ + unsigned short sglist_len; /* size of malloc'd scatter-gather list */ + void *request_buffer; /* Actual requested buffer */ + unsigned request_bufflen; /* Actual request size */ + /* + * Number of bytes requested to be transferred less actual + * number transferred (0 if not supported) + */ + int resid; +}; + struct scsi_cmnd { struct scsi_device *device; struct list_head list; /* scsi_cmnd participates in queue lists */ @@ -135,4 +147,6 @@ extern void scsi_kunmap_atomic_sg(void * extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t); extern void scsi_free_sgtable(struct scatterlist *, int); +extern struct scsi_data_buffer *scsi_bidi_data_buffer(struct scsi_cmnd *); + #endif /* _SCSI_SCSI_CMND_H */ -- 1.4.3.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 0/2] bidi support 2007-05-05 9:24 ` FUJITA Tomonori @ 2007-05-07 6:05 ` Benny Halevy 2007-05-07 7:02 ` FUJITA Tomonori 0 siblings, 1 reply; 4+ messages in thread From: Benny Halevy @ 2007-05-07 6:05 UTC (permalink / raw) To: FUJITA Tomonori Cc: James.Bottomley, jens.axboe, linux-scsi, hch, akpm, michaelc Tomo, Thanks for quickly crafting this patchset. Please see my comments in-line below. Putting aside the controversial design issues, we need to carefully compare our patches against yours to make sure no functional issues have been overlooked. Benny FUJITA Tomonori wrote: > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> > Subject: [PATCH 0/2] bidi support > Date: Sat, 05 May 2007 18:02:29 +0900 > >> This patchset add bidi support for block pc requests. > > Oh, this patchset is against Linus' tree. > > The first patch (the block layer changes) can be applied against Jens' > tree. > > The second patch (the scsi mid-layer changes) has one minor reject > against the scsi-misc tree. The following patch is against the > scsi-misc. > > --- > From 6a8c5375f1f6dbd574107920dd0a613527bd556b Mon Sep 17 00:00:00 2001 > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> > Date: Sat, 5 May 2007 18:11:42 +0900 > Subject: [PATCH] add bidi support for block pc requests > > This adds bidi support for block pc requests. > > A bidi request uses req->next_rq pointer for an in request. > > This patch introduces a new structure, scsi_data_buffer to hold the > data buffer information. To avoid make scsi_cmnd structure fatter, the > scsi mid-layer uses cmnd->request->next_rq->special pointer for > a scsi_data_buffer structure. LLDs don't touch the second request > (req->next_rq) so it's safe to use req->special. > > scsi_blk_pc_done() always completes the whole command so > scsi_end_request() simply completes the bidi chunk too. > > A helper function, scsi_bidi_data_buffer() is for LLDs to access to > the scsi_data_buffer structure easily. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> > --- > drivers/scsi/scsi_lib.c | 120 +++++++++++++++++++++++++++++++++++++++------ > include/scsi/scsi_cmnd.h | 14 +++++ > 2 files changed, 118 insertions(+), 16 deletions(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index be8e655..8f7873a 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -66,6 +66,12 @@ #undef SP > > static void scsi_run_queue(struct request_queue *q); > > +struct scsi_data_buffer *scsi_bidi_data_buffer(struct scsi_cmnd *cmd) > +{ > + return blk_bidi_rq(cmd->request) ? cmd->request->next_rq->special : NULL; > +} > +EXPORT_SYMBOL(scsi_bidi_data_buffer); > + > /* > * Function: scsi_unprep_request() > * > @@ -85,6 +91,7 @@ static void scsi_unprep_request(struct r > req->cmd_flags &= ~REQ_DONTPREP; > req->special = NULL; > > + kfree(scsi_bidi_data_buffer(cmd)); > scsi_put_command(cmd); > } > > @@ -657,6 +664,7 @@ static struct scsi_cmnd *scsi_end_reques > request_queue_t *q = cmd->device->request_queue; > struct request *req = cmd->request; > unsigned long flags; > + struct scsi_data_buffer *sdb = scsi_bidi_data_buffer(cmd); > > /* > * If there are blocks left over at the end, set up the command > @@ -685,6 +693,14 @@ static struct scsi_cmnd *scsi_end_reques > } > } > > + /* > + * a REQ_BLOCK_PC command is always completed fully so just do > + * end the bidi chunk. > + */ > + if (sdb) > + end_that_request_chunk(req->next_rq, uptodate, > + sdb->request_bufflen); I think I agree you shouldn't call end_that_request_last(req->next_rq) so sdb and req->next_rq should be freed here, no? > + > add_disk_randomness(req->rq_disk); > > spin_lock_irqsave(q->queue_lock, flags); > @@ -701,34 +717,35 @@ static struct scsi_cmnd *scsi_end_reques > return NULL; > } > > -struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask) > +static struct scatterlist *do_scsi_alloc_sgtable(unsigned short use_sg, > + unsigned short *sglist_len, > + gfp_t gfp_mask) > { > struct scsi_host_sg_pool *sgp; > - struct scatterlist *sgl; > > - BUG_ON(!cmd->use_sg); > + BUG_ON(!use_sg); > > - switch (cmd->use_sg) { > + switch (use_sg) { > case 1 ... 8: > - cmd->sglist_len = 0; > + *sglist_len = 0; > break; > case 9 ... 16: > - cmd->sglist_len = 1; > + *sglist_len = 1; > break; > case 17 ... 32: > - cmd->sglist_len = 2; > + *sglist_len = 2; > break; > #if (SCSI_MAX_PHYS_SEGMENTS > 32) > case 33 ... 64: > - cmd->sglist_len = 3; > + *sglist_len = 3; > break; > #if (SCSI_MAX_PHYS_SEGMENTS > 64) > case 65 ... 128: > - cmd->sglist_len = 4; > + *sglist_len = 4; > break; > #if (SCSI_MAX_PHYS_SEGMENTS > 128) > case 129 ... 256: > - cmd->sglist_len = 5; > + *sglist_len = 5; > break; > #endif > #endif > @@ -737,11 +754,14 @@ #endif > return NULL; > } > > - sgp = scsi_sg_pools + cmd->sglist_len; > - sgl = mempool_alloc(sgp->pool, gfp_mask); > - return sgl; > + sgp = scsi_sg_pools + *sglist_len; > + return mempool_alloc(sgp->pool, gfp_mask); > } > > +struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask) > +{ > + return do_scsi_alloc_sgtable(cmd->use_sg, &cmd->sglist_len, gfp_mask); > +} > EXPORT_SYMBOL(scsi_alloc_sgtable); > > void scsi_free_sgtable(struct scatterlist *sgl, int index) > @@ -775,6 +795,8 @@ EXPORT_SYMBOL(scsi_free_sgtable); > */ > static void scsi_release_buffers(struct scsi_cmnd *cmd) > { > + struct scsi_data_buffer *sdb = scsi_bidi_data_buffer(cmd); > + > if (cmd->use_sg) > scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len); > > @@ -784,6 +806,13 @@ static void scsi_release_buffers(struct > */ > cmd->request_buffer = NULL; > cmd->request_bufflen = 0; > + > + if (sdb) { > + if (sdb->use_sg) > + scsi_free_sgtable(sdb->request_buffer, sdb->sglist_len); > + sdb->request_buffer = NULL; > + sdb->request_bufflen = 0; > + } > } > > /* > @@ -834,6 +863,7 @@ void scsi_io_completion(struct scsi_cmnd > } > > if (blk_pc_request(req)) { /* SG_IO ioctl from block level */ > + struct scsi_data_buffer *sdb = scsi_bidi_data_buffer(cmd); > req->errors = result; > if (result) { > clear_errors = 0; > @@ -850,6 +880,8 @@ void scsi_io_completion(struct scsi_cmnd > } > } > req->data_len = cmd->resid; > + if (sdb) > + req->next_rq->data_len = sdb->resid; > } > > /* > @@ -1075,6 +1107,38 @@ static struct scsi_cmnd *scsi_get_cmd_fr > return cmd; > } > > +static int scsi_data_buffer_init(struct scsi_cmnd *cmd) > +{ > + struct scatterlist *sgpnt; > + struct scsi_data_buffer *sdb = scsi_bidi_data_buffer(cmd); > + struct request *req = cmd->request; > + int count; > + > + sdb->use_sg = req->next_rq->nr_phys_segments; > + sgpnt = do_scsi_alloc_sgtable(sdb->use_sg, &sdb->sglist_len, > + GFP_ATOMIC); > + if (unlikely(!sgpnt)) { > + scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len); > + scsi_unprep_request(req); > + return BLKPREP_DEFER; > + } > + > + req->buffer = NULL; req->next_rq->buffer = NULL; no? > + sdb->request_buffer = (char *) sgpnt; > + sdb->request_bufflen = req->next_rq->data_len; > + > + count = blk_rq_map_sg(req->q, req->next_rq, sgpnt); > + if (likely(count <= sdb->use_sg)) { > + sdb->use_sg = count; > + return BLKPREP_OK; > + } > + > + scsi_release_buffers(cmd); either kfree(sbd) and blk_put_request(req->next_rq) here, or should that be done in scsi_put_command? who does that on the error-free path? (see comment above in scsi_end_request) > + scsi_put_command(cmd); > + > + return BLKPREP_KILL; > +} > + > static void scsi_blk_pc_done(struct scsi_cmnd *cmd) > { > BUG_ON(!blk_pc_request(cmd->request)); > @@ -1090,10 +1154,21 @@ static void scsi_blk_pc_done(struct scsi > static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) > { > struct scsi_cmnd *cmd; > + struct scsi_data_buffer *sdb = NULL; > + > + if (blk_bidi_rq(req)) { > + sdb = kzalloc(sizeof(*sdb), GFP_ATOMIC); > + if (unlikely(!sdb)) > + return BLKPREP_DEFER; > + req->next_rq->special = sdb; > + } > > cmd = scsi_get_cmd_from_req(sdev, req); > - if (unlikely(!cmd)) > + if (unlikely(!cmd)) { > + req->next_rq->special = NULL; req->next_rq can be NULL > + kfree(sdb); > return BLKPREP_DEFER; > + } > > /* > * BLOCK_PC requests may transfer data, in which case they must > @@ -1109,6 +1184,12 @@ static int scsi_setup_blk_pc_cmnd(struct > ret = scsi_init_io(cmd); > if (unlikely(ret)) > return ret; > + > + if (sdb) { > + ret = scsi_data_buffer_init(cmd); > + if (ret != BLKPREP_OK) > + return ret; > + } > } else { > BUG_ON(req->data_len); > BUG_ON(req->data); > @@ -1122,13 +1203,15 @@ static int scsi_setup_blk_pc_cmnd(struct > BUILD_BUG_ON(sizeof(req->cmd) > sizeof(cmd->cmnd)); > memcpy(cmd->cmnd, req->cmd, sizeof(cmd->cmnd)); > cmd->cmd_len = req->cmd_len; > - if (!req->data_len) > + if (sdb) > + cmd->sc_data_direction = DMA_BIDIRECTIONAL; > + else if (!req->data_len) > cmd->sc_data_direction = DMA_NONE; > else if (rq_data_dir(req) == WRITE) > cmd->sc_data_direction = DMA_TO_DEVICE; > else > cmd->sc_data_direction = DMA_FROM_DEVICE; > - > + > cmd->transfersize = req->data_len; > cmd->allowed = req->retries; > cmd->timeout_per_command = req->timeout; > @@ -1178,6 +1261,11 @@ static int scsi_prep_fn(struct request_q > struct scsi_device *sdev = q->queuedata; > int ret = BLKPREP_OK; > > + if (WARN_ON(!blk_pc_request(req) && blk_bidi_rq(req))) { > + ret = BLKPREP_KILL; > + goto out; > + } > + > /* > * If the device is not in running state we will reject some > * or all commands. > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h > index a2e0c10..597c48c 100644 > --- a/include/scsi/scsi_cmnd.h > +++ b/include/scsi/scsi_cmnd.h > @@ -28,6 +28,18 @@ struct scsi_pointer { > volatile int phase; > }; > > +struct scsi_data_buffer { > + unsigned short use_sg; /* Number of pieces of scatter-gather */ > + unsigned short sglist_len; /* size of malloc'd scatter-gather list */ > + void *request_buffer; /* Actual requested buffer */ > + unsigned request_bufflen; /* Actual request size */ > + /* > + * Number of bytes requested to be transferred less actual > + * number transferred (0 if not supported) > + */ > + int resid; > +}; > + > struct scsi_cmnd { > struct scsi_device *device; > struct list_head list; /* scsi_cmnd participates in queue lists */ > @@ -135,4 +147,6 @@ extern void scsi_kunmap_atomic_sg(void * > extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t); > extern void scsi_free_sgtable(struct scatterlist *, int); > > +extern struct scsi_data_buffer *scsi_bidi_data_buffer(struct scsi_cmnd *); > + > #endif /* _SCSI_SCSI_CMND_H */ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 0/2] bidi support 2007-05-07 6:05 ` Benny Halevy @ 2007-05-07 7:02 ` FUJITA Tomonori 0 siblings, 0 replies; 4+ messages in thread From: FUJITA Tomonori @ 2007-05-07 7:02 UTC (permalink / raw) To: bhalevy Cc: fujita.tomonori, James.Bottomley, jens.axboe, linux-scsi, hch, akpm, michaelc From: Benny Halevy <bhalevy@panasas.com> Subject: Re: [PATCH 0/2] bidi support Date: Mon, 07 May 2007 09:05:37 +0300 > Tomo, > > Thanks for quickly crafting this patchset. > Please see my comments in-line below. > > Putting aside the controversial design issues, > we need to carefully compare our patches against yours > to make sure no functional issues have been overlooked. (snip) > > @@ -685,6 +693,14 @@ static struct scsi_cmnd *scsi_end_reques > > } > > } > > > > + /* > > + * a REQ_BLOCK_PC command is always completed fully so just do > > + * end the bidi chunk. > > + */ > > + if (sdb) > > + end_that_request_chunk(req->next_rq, uptodate, > > + sdb->request_bufflen); > > I think I agree you shouldn't call end_that_request_last(req->next_rq) > so sdb and req->next_rq should be freed here, no? I think that bidi requests are en-queued via blk_execute_rq (or blk_execute_rq_nowait). The caller frees req and req->next_rq. > > +static int scsi_data_buffer_init(struct scsi_cmnd *cmd) > > +{ > > + struct scatterlist *sgpnt; > > + struct scsi_data_buffer *sdb = scsi_bidi_data_buffer(cmd); > > + struct request *req = cmd->request; > > + int count; > > + > > + sdb->use_sg = req->next_rq->nr_phys_segments; > > + sgpnt = do_scsi_alloc_sgtable(sdb->use_sg, &sdb->sglist_len, > > + GFP_ATOMIC); > > + if (unlikely(!sgpnt)) { > > + scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len); > > + scsi_unprep_request(req); > > + return BLKPREP_DEFER; > > + } > > + > > + req->buffer = NULL; > > req->next_rq->buffer = NULL; > > no? Yes, but maybe we can remove this. > > + sdb->request_buffer = (char *) sgpnt; > > + sdb->request_bufflen = req->next_rq->data_len; > > + > > + count = blk_rq_map_sg(req->q, req->next_rq, sgpnt); > > + if (likely(count <= sdb->use_sg)) { > > + sdb->use_sg = count; > > + return BLKPREP_OK; > > + } > > + > > + scsi_release_buffers(cmd); > > either kfree(sbd) and blk_put_request(req->next_rq) here, or > should that be done in scsi_put_command? > who does that on the error-free path? (see comment above in scsi_end_request) Yeah, I think that scsi_release_buffers() should free sbd. > > + scsi_put_command(cmd); > > + > > + return BLKPREP_KILL; > > +} > > + > > static void scsi_blk_pc_done(struct scsi_cmnd *cmd) > > { > > BUG_ON(!blk_pc_request(cmd->request)); > > @@ -1090,10 +1154,21 @@ static void scsi_blk_pc_done(struct scsi > > static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) > > { > > struct scsi_cmnd *cmd; > > + struct scsi_data_buffer *sdb = NULL; > > + > > + if (blk_bidi_rq(req)) { > > + sdb = kzalloc(sizeof(*sdb), GFP_ATOMIC); > > + if (unlikely(!sdb)) > > + return BLKPREP_DEFER; > > + req->next_rq->special = sdb; > > + } > > > > cmd = scsi_get_cmd_from_req(sdev, req); > > - if (unlikely(!cmd)) > > + if (unlikely(!cmd)) { > > + req->next_rq->special = NULL; > > req->next_rq can be NULL Opps, thanks. Thanks, I'll fix them and update the git tree shortly. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-05-07 7:11 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-05-05 9:02 [PATCH 0/2] bidi support FUJITA Tomonori 2007-05-05 9:24 ` FUJITA Tomonori 2007-05-07 6:05 ` Benny Halevy 2007-05-07 7:02 ` FUJITA Tomonori
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.