FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 12-01-2007, 09:42 PM
Bartlomiej Zolnierkiewicz
 
Default blk_end_request: changing ide-cd (take 3)

Hi,

On Saturday 01 December 2007, Kiyoshi Ueda wrote:
> This patch converts ide-cd (cdrom_newpc_intr()) to use blk_end_request().
>
> ide-cd (cdrom_newpc_intr()) has some tricky behaviors below which
> need to use blk_end_request_callback().
> Needs to:
> 1. call post_transform_command() to modify request contents

Seems like post_transform_command() call can be removed (patch below).

> 2. wait completing request until DRQ_STAT is cleared

Would be great if somebody convert cdrom_newpc_intr() to use scatterlists
also for PIO transfers (ide_pio_sector() in ide-taskfile.c should serve
as a good starting base to see how to do PIO transfers using scatterlists)
so we could get rid of partial request completions in cdrom_newpc_intr()
and just fully complete request when the transfer is done. Shouldn't be
difficult but I guess that we can live with blk_end_request_callback() for
the time being...

> after end_that_request_first() and before end_that_request_last().
>
> As for the second one, ide-cd will wait for the interrupt from device.
> So blk_end_request_callback() has to return without completing request
> even if no leftover in the request.
> ide-cd uses a dummy callback function, which just returns value '1',
> to tell blk_end_request_callback() about that.
>
> Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>

[PATCH] ide-cd: remove dead post_transform_command()

post_transform_command() call in cdrom_newpc_intr() has no effect because
it is done after the request has already been fully completed (rq->bio and
rq->data are always NULL). It was verified to be true regardless whether
INQUIRY command is using DMA or PIO to transfer data (by using modified
Tejun Heo's test-shortsg.c utility and adding a few printk()-s to ide-cd).

This was uncovered thanks to the "blk_end_request: full I/O completion
handler (take 3)" patch series from Kiyoshi Ueda.

Cc: jens.axboe@oracle.com
Cc: bharrosh@panasas.com
Cc: Kiyoshi Ueda <k-ueda@ct.jp.nec.com
Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Tejun Heo <htejun@gmail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
Kiyoshi: please rebase your patch on top of this one (I'll send
it to Linus in the next IDE update), should make your patch a bit
simpler.

Tejun: you had really good timing with posting test-shortsg.c
(it saved me some time coding user-space SG_IO tester), thanks!

drivers/ide/ide-cd.c | 28 ----------------------------
1 file changed, 28 deletions(-)

Index: b/drivers/ide/ide-cd.c
================================================== =================
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1650,31 +1650,6 @@ static int cdrom_write_check_ireason(ide
return 1;
}

-static void post_transform_command(struct request *req)
-{
- u8 *c = req->cmd;
- char *ibuf;
-
- if (!blk_pc_request(req))
- return;
-
- if (req->bio)
- ibuf = bio_data(req->bio);
- else
- ibuf = req->data;
-
- if (!ibuf)
- return;
-
- /*
- * set ansi-revision and response data as atapi
- */
- if (c[0] == GPCMD_INQUIRY) {
- ibuf[2] |= 2;
- ibuf[3] = (ibuf[3] & 0xf0) | 2;
- }
-}
-
typedef void (xfer_func_t)(ide_drive_t *, void *, u32);

/*
@@ -1810,9 +1785,6 @@ static ide_startstop_t cdrom_newpc_intr(
return ide_started;

end_request:
- if (!rq->data_len)
- post_transform_command(rq);
-
spin_lock_irqsave(&ide_lock, flags);
blkdev_dequeue_request(rq);
end_that_request_last(rq, 1);

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 12-03-2007, 02:55 PM
Sergei Shtylyov
 
Default blk_end_request: changing ide-cd (take 3)

Bartlomiej Zolnierkiewicz wrote:


[PATCH] ide-cd: remove dead post_transform_command()



post_transform_command() call in cdrom_newpc_intr() has no effect because
it is done after the request has already been fully completed (rq->bio and
rq->data are always NULL). It was verified to be true regardless whether
INQUIRY command is using DMA or PIO to transfer data (by using modified
Tejun Heo's test-shortsg.c utility and adding a few printk()-s to ide-cd).



This was uncovered thanks to the "blk_end_request: full I/O completion
handler (take 3)" patch series from Kiyoshi Ueda.



Cc: jens.axboe@oracle.com
Cc: bharrosh@panasas.com
Cc: Kiyoshi Ueda <k-ueda@ct.jp.nec.com
Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Tejun Heo <htejun@gmail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>


Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

MBR, Sergei

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 12-03-2007, 09:52 PM
Kiyoshi Ueda
 
Default blk_end_request: changing ide-cd (take 3)

Hi Bartlomiej,

On Sat, 1 Dec 2007 23:42:51 +0100, Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> On Saturday 01 December 2007, Kiyoshi Ueda wrote:
> > This patch converts ide-cd (cdrom_newpc_intr()) to use blk_end_request().
> >
> > ide-cd (cdrom_newpc_intr()) has some tricky behaviors below which
> > need to use blk_end_request_callback().
> > Needs to:
> > 1. call post_transform_command() to modify request contents
>
> Seems like post_transform_command() call can be removed (patch below).
>
> > 2. wait completing request until DRQ_STAT is cleared
>
> Would be great if somebody convert cdrom_newpc_intr() to use scatterlists
> also for PIO transfers (ide_pio_sector() in ide-taskfile.c should serve
> as a good starting base to see how to do PIO transfers using scatterlists)
> so we could get rid of partial request completions in cdrom_newpc_intr()
> and just fully complete request when the transfer is done. Shouldn't be
> difficult but I guess that we can live with blk_end_request_callback() for
> the time being...
>
> > after end_that_request_first() and before end_that_request_last().
> >
> > As for the second one, ide-cd will wait for the interrupt from device.
> > So blk_end_request_callback() has to return without completing request
> > even if no leftover in the request.
> > ide-cd uses a dummy callback function, which just returns value '1',
> > to tell blk_end_request_callback() about that.
> >
> > Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
> > Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
>
> [PATCH] ide-cd: remove dead post_transform_command()
>
> post_transform_command() call in cdrom_newpc_intr() has no effect because
> it is done after the request has already been fully completed (rq->bio and
> rq->data are always NULL). It was verified to be true regardless whether
> INQUIRY command is using DMA or PIO to transfer data (by using modified
> Tejun Heo's test-shortsg.c utility and adding a few printk()-s to ide-cd).
>
> This was uncovered thanks to the "blk_end_request: full I/O completion
> handler (take 3)" patch series from Kiyoshi Ueda.
>
> Cc: jens.axboe@oracle.com
> Cc: bharrosh@panasas.com
> Cc: Kiyoshi Ueda <k-ueda@ct.jp.nec.com
> Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> Cc: Tejun Heo <htejun@gmail.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
> Kiyoshi: please rebase your patch on top of this one (I'll send
> it to Linus in the next IDE update), should make your patch a bit
> simpler.
>
> Tejun: you had really good timing with posting test-shortsg.c
> (it saved me some time coding user-space SG_IO tester), thanks!
>
> drivers/ide/ide-cd.c | 28 ----------------------------
> 1 file changed, 28 deletions(-)
>
> Index: b/drivers/ide/ide-cd.c
> ================================================== =================
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -1650,31 +1650,6 @@ static int cdrom_write_check_ireason(ide
> return 1;
> }
>
> -static void post_transform_command(struct request *req)
> -{
> - u8 *c = req->cmd;
> - char *ibuf;
> -
> - if (!blk_pc_request(req))
> - return;
> -
> - if (req->bio)
> - ibuf = bio_data(req->bio);
> - else
> - ibuf = req->data;
> -
> - if (!ibuf)
> - return;
> -
> - /*
> - * set ansi-revision and response data as atapi
> - */
> - if (c[0] == GPCMD_INQUIRY) {
> - ibuf[2] |= 2;
> - ibuf[3] = (ibuf[3] & 0xf0) | 2;
> - }
> -}
> -
> typedef void (xfer_func_t)(ide_drive_t *, void *, u32);
>
> /*
> @@ -1810,9 +1785,6 @@ static ide_startstop_t cdrom_newpc_intr(
> return ide_started;
>
> end_request:
> - if (!rq->data_len)
> - post_transform_command(rq);
> -
> spin_lock_irqsave(&ide_lock, flags);
> blkdev_dequeue_request(rq);
> end_that_request_last(rq, 1);

Thank you for the comments.
I rebased my patch on top of 2.6.24-rc3-mm2 + the patch to remove
post_transform_command().

As a result, one callback function for DMA mode has been removed.
What do you think about the patch below?



Subject: [PATCH 26/28] blk_end_request: changing ide-cd (take 3)


This patch converts ide-cd (cdrom_newpc_intr()) to use blk_end_request
interfaces.

In PIO mode, ide-cd (cdrom_newpc_intr()) needs to defer
end_that_request_last() until the device clears DRQ_STAT and raises
an interrupt after end_that_request_first().
So blk_end_request() has to return without completing request
even if no leftover in the request.

ide-cd uses blk_end_request_callback() and a dummy callback function,
which just returns value '1', to tell blk_end_request_callback()
about that.

Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
---
drivers/ide/ide-cd.c | 49 +++++++++++++++++++++++++++++++++++--------------
1 files changed, 35 insertions(+), 14 deletions(-)

Index: 2.6.24-rc3-mm2/drivers/ide/ide-cd.c
================================================== =================
--- 2.6.24-rc3-mm2.orig/drivers/ide/ide-cd.c
+++ 2.6.24-rc3-mm2/drivers/ide/ide-cd.c
@@ -1644,6 +1644,17 @@ static int cdrom_write_check_ireason(ide
return 1;
}

+/*
+ * Called from blk_end_request_callback() after the data of the request
+ * is completed and before the request is completed.
+ * By returning value '1', blk_end_request_callback() returns immediately
+ * without completing the request.
+ */
+static int cdrom_newpc_intr_dummy_cb(struct request *rq)
+{
+ return 1;
+}
+
typedef void (xfer_func_t)(ide_drive_t *, void *, u32);

/*
@@ -1682,9 +1693,13 @@ static ide_startstop_t cdrom_newpc_intr(
return ide_error(drive, "dma error", stat);
}

- end_that_request_chunk(rq, 1, rq->data_len);
- rq->data_len = 0;
- goto end_request;
+ spin_lock_irqsave(&ide_lock, flags);
+ if (__blk_end_request(rq, 1, rq->data_len))
+ BUG();
+ HWGROUP(drive)->rq = NULL;
+ spin_unlock_irqrestore(&ide_lock, flags);
+
+ return ide_stopped;
}

/*
@@ -1702,8 +1717,15 @@ static ide_startstop_t cdrom_newpc_intr(
/*
* If DRQ is clear, the command has completed.
*/
- if ((stat & DRQ_STAT) == 0)
- goto end_request;
+ if ((stat & DRQ_STAT) == 0) {
+ spin_lock_irqsave(&ide_lock, flags);
+ if (__blk_end_request(rq, 1, 0))
+ BUG();
+ HWGROUP(drive)->rq = NULL;
+ spin_unlock_irqrestore(&ide_lock, flags);
+
+ return ide_stopped;
+ }

/*
* check which way to transfer data
@@ -1756,7 +1778,14 @@ static ide_startstop_t cdrom_newpc_intr(
rq->data_len -= blen;

if (rq->bio)
- end_that_request_chunk(rq, 1, blen);
+ /*
+ * The request can't be completed until DRQ is cleared.
+ * So complete the data, but don't complete the request
+ * using the dummy function for the callback feature
+ * of blk_end_request_callback().
+ */
+ blk_end_request_callback(rq, 1, blen,
+ cdrom_newpc_intr_dummy_cb);
else
rq->data += blen;
}
@@ -1777,14 +1806,6 @@ static ide_startstop_t cdrom_newpc_intr(

ide_set_handler(drive, cdrom_newpc_intr, rq->timeout, NULL);
return ide_started;
-
-end_request:
- spin_lock_irqsave(&ide_lock, flags);
- blkdev_dequeue_request(rq);
- end_that_request_last(rq, 1);
- HWGROUP(drive)->rq = NULL;
- spin_unlock_irqrestore(&ide_lock, flags);
- return ide_stopped;
}

static ide_startstop_t cdrom_write_intr(ide_drive_t *drive)

Thanks,
Kiyoshi Ueda

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 12-04-2007, 12:46 PM
Bartlomiej Zolnierkiewicz
 
Default blk_end_request: changing ide-cd (take 3)

Hi,

On Monday 03 December 2007, Kiyoshi Ueda wrote:

[...]

> Thank you for the comments.
> I rebased my patch on top of 2.6.24-rc3-mm2 + the patch to remove
> post_transform_command().
>
> As a result, one callback function for DMA mode has been removed.
> What do you think about the patch below?
>
>
>
> Subject: [PATCH 26/28] blk_end_request: changing ide-cd (take 3)
>
>
> This patch converts ide-cd (cdrom_newpc_intr()) to use blk_end_request
> interfaces.
>
> In PIO mode, ide-cd (cdrom_newpc_intr()) needs to defer
> end_that_request_last() until the device clears DRQ_STAT and raises
> an interrupt after end_that_request_first().
> So blk_end_request() has to return without completing request
> even if no leftover in the request.
>
> ide-cd uses blk_end_request_callback() and a dummy callback function,
> which just returns value '1', to tell blk_end_request_callback()
> about that.
>
> Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>

Looks good, thanks!

Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

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

Thread Tools




All times are GMT. The time now is 11:02 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org