Resend: [PATCH] blkdev: fix blkdev_issue_zeroout return value

Resend: [PATCH] blkdev: fix blkdev_issue_zeroout return value

[Dmitry Monakhov]

[-- Attachment #1: Type: text/plain, Size: 240 bytes --]

Hi Jens,
Seems that my first mail was missed somewhere.  
I've found couple of trivial issues in blkdev_issue_zeroout()
implementation. Unfortunately I've miss during initial testing phase
because always called it with BARRIER|WAIT flags.


[-- Attachment #2: patch-1 --]
[-- Type: text/plain, Size: 1841 bytes --]

>From 5eb4d762ad8fe146ee638fb1b2d7730db3e3ca4b Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <dmonakhov@openvz.org>
Date: Tue, 27 Jul 2010 17:12:27 +0400
Subject: [PATCH] blkdev: fix blkdev_issue_zeroout return value

- If function called without barrier option retvalue is incorrect
- Add io_schedule() between loops


 diff --git a/block/blk-lib.c b/block/blk-lib.c
index d0216b9..a8d6a21 100644

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 block/blk-lib.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index d0216b9..a8d6a21 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -157,7 +157,7 @@ static void bio_batch_end_io(struct bio *bio, int err)
 int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 			sector_t nr_sects, gfp_t gfp_mask, unsigned long flags)
 {
-	int ret = 0;
+	int ret;
 	struct bio *bio;
 	struct bio_batch bb;
 	unsigned int sz, issued = 0;
@@ -175,11 +175,14 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 			return ret;
 	}
 submit:
+	ret = 0;
 	while (nr_sects != 0) {
 		bio = bio_alloc(gfp_mask,
 				min(nr_sects, (sector_t)BIO_MAX_PAGES));
-		if (!bio)
+		if (!bio) {
+			ret = -ENOMEM;
 			break;
+		}
 
 		bio->bi_sector = sector;
 		bio->bi_bdev   = bdev;
@@ -198,6 +201,7 @@ submit:
 			if (ret < (sz << 9))
 				break;
 		}
+		ret = 0;
 		issued++;
 		submit_bio(WRITE, bio);
 	}
@@ -218,15 +222,18 @@ submit:
 		/* One of bios in the batch was completed with error.*/
 		ret = -EIO;
 
-	if (ret)
+	if (ret && ret != -ENOMEM)
 		goto out;
 
 	if (test_bit(BIO_EOPNOTSUPP, &bb.flags)) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
-	if (nr_sects != 0)
+	if (nr_sects != 0) {
+		if (ret == -ENOMEM)
+			io_schedule();
 		goto submit;
+	}
 out:
 	return ret;
 }
-- 
1.6.6.1

Re: Resend: [PATCH] blkdev: fix blkdev_issue_zeroout return value

[Jens Axboe]

On 2010-08-06 12:42, Dmitry Monakhov wrote:
> Hi Jens,
> Seems that my first mail was missed somewhere.  
> I've found couple of trivial issues in blkdev_issue_zeroout()
> implementation. Unfortunately I've miss during initial testing phase
> because always called it with BARRIER|WAIT flags.
> 

OK, I will add this so it can go out with the next pull request.

-- 
Jens Axboe

Re: Resend: [PATCH] blkdev: fix blkdev_issue_zeroout return value

[Jens Axboe]

On 2010-08-06 12:42, Dmitry Monakhov wrote:
> Hi Jens,
> Seems that my first mail was missed somewhere.  
> I've found couple of trivial issues in blkdev_issue_zeroout()
> implementation. Unfortunately I've miss during initial testing phase
> because always called it with BARRIER|WAIT flags.

BTW, this:

@@ -218,15 +222,18 @@ submit:
 		/* One of bios in the batch was completed with error.*/
 		ret = -EIO;
 
-	if (ret)
+	if (ret && ret != -ENOMEM)
 		goto out;
 
 	if (test_bit(BIO_EOPNOTSUPP, &bb.flags)) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
-	if (nr_sects != 0)
+	if (nr_sects != 0) {
+		if (ret == -ENOMEM)
+			io_schedule();
 		goto submit;
+	}
 out:
 	return ret;
 }

is broken. Either the caller sets __GFP_WAIT and then bio_alloc() will
not fail, or GFP_ATOMIC is used knowing that the call can fail and
return ENOMEM. Don't code in retry logic like this.

-- 
Jens Axboe

Re: Resend: [PATCH] blkdev: fix blkdev_issue_zeroout return value

[Dmitry Monakhov]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

Jens Axboe <axboe@kernel.dk> writes:

> On 2010-08-06 12:42, Dmitry Monakhov wrote:
>> Hi Jens,
>> Seems that my first mail was missed somewhere.  
>> I've found couple of trivial issues in blkdev_issue_zeroout()
>> implementation. Unfortunately I've miss during initial testing phase
>> because always called it with BARRIER|WAIT flags.
>
> BTW, this:
>
> @@ -218,15 +222,18 @@ submit:
>  		/* One of bios in the batch was completed with error.*/
>  		ret = -EIO;
>  
> -	if (ret)
> +	if (ret && ret != -ENOMEM)
>  		goto out;
>  
>  	if (test_bit(BIO_EOPNOTSUPP, &bb.flags)) {
>  		ret = -EOPNOTSUPP;
>  		goto out;
>  	}
> -	if (nr_sects != 0)
> +	if (nr_sects != 0) {
> +		if (ret == -ENOMEM)
> +			io_schedule();
>  		goto submit;
> +	}
>  out:
>  	return ret;
>  }
>
> is broken. Either the caller sets __GFP_WAIT and then bio_alloc() will
> not fail, or GFP_ATOMIC is used knowing that the call can fail and
> return ENOMEM. Don't code in retry logic like this.
Ok, my fault and in fact i've done in explicitly. I just thought
that blk-layer is some times an exception from general GFP_ATOMIC rule
because in some places in blk-layer we stick to GFP_NOFAIL semantics
regardless to actual gfp flags.

New version attached.

[-- Attachment #2: 0001-blkdev-fix-blkdev_issue_zeroout-return-value.patch --]
[-- Type: text/plain, Size: 1345 bytes --]

>From 32236cd453f070807a91631a2de12478f872edd3 Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <dmonakhov@openvz.org>
Date: Fri, 6 Aug 2010 15:06:01 +0400
Subject: [PATCH] blkdev: fix blkdev_issue_zeroout return value

- If function called without barrier option retvalue is incorrect

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 block/blk-lib.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 5d793e1..c1fc55a 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -145,7 +145,7 @@ static void bio_batch_end_io(struct bio *bio, int err)
 int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 			sector_t nr_sects, gfp_t gfp_mask, unsigned long flags)
 {
-	int ret = 0;
+	int ret;
 	struct bio *bio;
 	struct bio_batch bb;
 	unsigned int sz, issued = 0;
@@ -163,11 +163,14 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 			return ret;
 	}
 submit:
+	ret = 0;
 	while (nr_sects != 0) {
 		bio = bio_alloc(gfp_mask,
 				min(nr_sects, (sector_t)BIO_MAX_PAGES));
-		if (!bio)
+		if (!bio) {
+			ret = -ENOMEM;
 			break;
+		}
 
 		bio->bi_sector = sector;
 		bio->bi_bdev   = bdev;
@@ -186,6 +189,7 @@ submit:
 			if (ret < (sz << 9))
 				break;
 		}
+		ret = 0;
 		issued++;
 		submit_bio(WRITE, bio);
 	}
-- 
1.6.6.1

Re: Resend: [PATCH] blkdev: fix blkdev_issue_zeroout return value

[Jens Axboe]

On 2010-08-06 13:15, Dmitry Monakhov wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> On 2010-08-06 12:42, Dmitry Monakhov wrote:
>>> Hi Jens,
>>> Seems that my first mail was missed somewhere.  
>>> I've found couple of trivial issues in blkdev_issue_zeroout()
>>> implementation. Unfortunately I've miss during initial testing phase
>>> because always called it with BARRIER|WAIT flags.
>>
>> BTW, this:
>>
>> @@ -218,15 +222,18 @@ submit:
>>  		/* One of bios in the batch was completed with error.*/
>>  		ret = -EIO;
>>  
>> -	if (ret)
>> +	if (ret && ret != -ENOMEM)
>>  		goto out;
>>  
>>  	if (test_bit(BIO_EOPNOTSUPP, &bb.flags)) {
>>  		ret = -EOPNOTSUPP;
>>  		goto out;
>>  	}
>> -	if (nr_sects != 0)
>> +	if (nr_sects != 0) {
>> +		if (ret == -ENOMEM)
>> +			io_schedule();
>>  		goto submit;
>> +	}
>>  out:
>>  	return ret;
>>  }
>>
>> is broken. Either the caller sets __GFP_WAIT and then bio_alloc() will
>> not fail, or GFP_ATOMIC is used knowing that the call can fail and
>> return ENOMEM. Don't code in retry logic like this.
> Ok, my fault and in fact i've done in explicitly. I just thought
> that blk-layer is some times an exception from general GFP_ATOMIC rule
> because in some places in blk-layer we stick to GFP_NOFAIL semantics
> regardless to actual gfp flags.
> 
> New version attached.

Thanks that looks better, now really added.

-- 
Jens Axboe