Re: [PATCH 15/18] z2ram: dequeue in-flight request

Sergei Shtylyov
2009-05-16T19:58:56Z

Hello, I wrote:

Oops, sent this message only to Tejun before, so have to repost now...

>> z2ram processes requests one-by-one synchronously and can be easily
>> converted to dequeueing model. Convert it.
>>
>> [ Impact: dequeue in-flight request ]
>>
>> Signed-off-by: Tejun Heo <tj@kernel.org>
>> ---
>> drivers/block/z2ram.c | 19 +++++++++++++++----
>> 1 files changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/block/z2ram.c b/drivers/block/z2ram.c
>> index 6a13838..c909c1a 100644
>> --- a/drivers/block/z2ram.c
>> +++ b/drivers/block/z2ram.c
>> @@ -70,15 +70,21 @@ static struct gendisk *z2ram_gendisk;
>> static void do_z2_request(struct request_queue *q)
>> {
>> struct request *req;
>> - while ((req = elv_next_request(q)) != NULL) {
>> +
>> + req = elv_next_request(q);
>> + if (req)
>> + blkdev_dequeue_request(req);
>> +
>> + while (req) {
>> unsigned long start = blk_rq_pos(req) << 9;
>> unsigned long len = blk_rq_cur_bytes(req);
>> + int err = 0;
>>
>> if (start + len > z2ram_size) {
>> printk( KERN_ERR DEVICE_NAME ": bad access: block=%lu,
>> count=%u\n",
>> blk_rq_pos(req), blk_rq_cur_sectors(req));
>> - __blk_end_request_cur(req, -EIO);
>> - continue;
>> + err = -EIO;
>> + goto done;
>> }
>> while (len) {
>> unsigned long addr = start & Z2RAM_CHUNKMASK;
>> @@ -93,7 +99,12 @@ static void do_z2_request(struct request_queue *q)
>> start += size;
>> len -= size;
>> }
>> - __blk_end_request_cur(req, 0);
>> + done:
>> + if (!__blk_end_request_cur(req, err)) {
>> + req = elv_next_request(q);
>> + if (req)
>> + blkdev_dequeue_request(req);
>> + }
>> }
>> }
>>
>
> I'm sure this can be made more compact and without duplication (in
> many other cases as well):
>
> @@ -70,15 +70,21 @@ static struct gendisk *z2ram_gendisk;
>
> static void do_z2_request(struct request_queue *q)
> {
> struct request *req;
> +
> while ((req = elv_next_request(q)) != NULL) {
> unsigned long start = blk_rq_pos(req) << 9;
> unsigned long len = blk_rq_cur_bytes(req);

Oops, missed:

+ int err;
+
>
> + blkdev_dequeue_request(req);
> +

+again:

> if (start + len > z2ram_size) {
> printk( KERN_ERR DEVICE_NAME ": bad access: block=%lu,
> count=%u\n",
> blk_rq_pos(req), blk_rq_cur_sectors(req));
> - __blk_end_request_cur(req, -EIO);
> - continue;
> + err = -EIO;
> + goto done;
> }
> while (len) {
> unsigned long addr = start & Z2RAM_CHUNKMASK;
> @@ -93,7 +99,12 @@ static void do_z2_request(struct request_queue *q)
> start += size;
> len -= size;
> }
> - __blk_end_request_cur(req, 0);
+ err = 0;
> + done:
> + if (__blk_end_request_cur(req, err));
> + break;

Oops, should've been:

+ if (__blk_end_request_cur(req, err))
+ goto again;

> }
> }

It can also be:

@@ -70,15 +70,21 @@ static struct gendisk *z2ram_gendisk;
static void do_z2_request(struct request_queue *q)
{
- struct request *req;
- while ((req = elv_next_request(q)) != NULL) {
+ while (1) {
+ struct request *req = elv_next_request(q);
+ unsigned long start, len;
+ int err;
+
+ if (req == NULL)
+ break;
+
+ blkdev_dequeue_request(req);
+
+again:
+ start = blk_rq_pos(req) << 9;
+ len = blk_rq_cur_bytes(req);

if (start + len > z2ram_size) {
printk( KERN_ERR DEVICE_NAME ": bad access: block=%lu,
count=%u\n",
blk_rq_pos(req), blk_rq_cur_sectors(req));
- __blk_end_request_cur(req, -EIO);
- continue;
+ err = -EIO;
+ goto done;
}
while (len) {
unsigned long addr = start & Z2RAM_CHUNKMASK;
@@ -93,7 +99,12 @@ static void do_z2_request(struct request_queue *q)
start += size;
len -= size;
}
- __blk_end_request_cur(req, 0);
+ err = 0;
+ done:
+ if (__blk_end_request_cur(req, err))
+ goto again;
}
}

if you want to get rid of the assignement in the *while* statement...

MBR, Sergei