[PATCH 1/2] blkdev: Submit discard bio in batches in blkdev_issue_discard()

[PATCH 1/2] blkdev: Submit discard bio in batches in blkdev_issue_discard()

[Lukas Czerner]

Currently we are waiting for every submitted REQ_DISCARD bio separately,
but it can have unwanted consequences of repeatedly flushing the queue,
so we rather submit bios in batches and wait for the entire batch, hence
narrowing the window of other ios going in.

Use bio_batch_end_io() and struct bio_batch for that purpose, the same
is used by blkdev_issue_zeroout(). Also change bio_batch_end_io() so we
always set !BIO_UPTODATE in the case of error and remove the check for
bb, since we are the only user of this function and we always set this.

Remove bio_get()/bio_put() from the blkdev_issue_discard() since
bio_alloc() and bio_batch_end_io() is doing the same thing, hence it is
not needed anymore.

I have done simple dd testing with surprising results. The script I have
used is:

for i in $(seq 10); do
        echo $i
        dd if=/dev/sdb1 of=/dev/sdc1 bs=4k &
        sleep 5
done
/usr/bin/time -f %e ./blkdiscard /dev/sdc1

Running time of BLKDISCARD on the whole device:
with patch              without patch
0.95                    15.58

So we can see that in this artificial test the kernel with the patch
applied is approx 16x faster in discarding the device.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
CC: Dmitry Monakhov <dmonakhov@openvz.org>
CC: Jens Axboe <jens.axboe@oracle.com>
---
 block/blk-lib.c |   69 +++++++++++++++++++++++-------------------------------
 1 files changed, 29 insertions(+), 40 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 25de73e..9260cb0 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -9,17 +9,23 @@
 
 #include "blk.h"
 
-static void blkdev_discard_end_io(struct bio *bio, int err)
+struct bio_batch {
+	atomic_t		done;
+	unsigned long		flags;
+	struct completion	*wait;
+};
+
+static void bio_batch_end_io(struct bio *bio, int err)
 {
+	struct bio_batch *bb = bio->bi_private;
+
 	if (err) {
 		if (err == -EOPNOTSUPP)
-			set_bit(BIO_EOPNOTSUPP, &bio->bi_flags);
-		clear_bit(BIO_UPTODATE, &bio->bi_flags);
+			set_bit(BIO_EOPNOTSUPP, &bb->flags);
+		clear_bit(BIO_UPTODATE, &bb->flags);
 	}
-
-	if (bio->bi_private)
-		complete(bio->bi_private);
-
+	if (atomic_dec_and_test(&bb->done))
+		complete(bb->wait);
 	bio_put(bio);
 }
 
@@ -41,6 +47,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	struct request_queue *q = bdev_get_queue(bdev);
 	int type = REQ_WRITE | REQ_DISCARD;
 	unsigned int max_discard_sectors;
+	struct bio_batch bb;
 	struct bio *bio;
 	int ret = 0;
 
@@ -67,7 +74,11 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		type |= REQ_SECURE;
 	}
 
-	while (nr_sects && !ret) {
+	atomic_set(&bb.done, 1);
+	bb.flags = 1 << BIO_UPTODATE;
+	bb.wait = &wait;
+
+	while (nr_sects) {
 		bio = bio_alloc(gfp_mask, 1);
 		if (!bio) {
 			ret = -ENOMEM;
@@ -75,9 +86,9 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		}
 
 		bio->bi_sector = sector;
-		bio->bi_end_io = blkdev_discard_end_io;
+		bio->bi_end_io = bio_batch_end_io;
 		bio->bi_bdev = bdev;
-		bio->bi_private = &wait;
+		bio->bi_private = &bb;
 
 		if (nr_sects > max_discard_sectors) {
 			bio->bi_size = max_discard_sectors << 9;
@@ -88,45 +99,23 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 			nr_sects = 0;
 		}
 
-		bio_get(bio);
+		atomic_inc(&bb.done);
 		submit_bio(type, bio);
+	}
 
+	/* Wait for bios in-flight */
+	if (!atomic_dec_and_test(&bb.done))
 		wait_for_completion(&wait);
 
-		if (bio_flagged(bio, BIO_EOPNOTSUPP))
-			ret = -EOPNOTSUPP;
-		else if (!bio_flagged(bio, BIO_UPTODATE))
-			ret = -EIO;
-		bio_put(bio);
-	}
+	if (test_bit(BIO_EOPNOTSUPP, &bb.flags))
+		ret = -EOPNOTSUPP;
+	else if (!test_bit(BIO_UPTODATE, &bb.flags))
+		ret = -EIO;
 
 	return ret;
 }
 EXPORT_SYMBOL(blkdev_issue_discard);
 
-struct bio_batch
-{
-	atomic_t 		done;
-	unsigned long 		flags;
-	struct completion 	*wait;
-};
-
-static void bio_batch_end_io(struct bio *bio, int err)
-{
-	struct bio_batch *bb = bio->bi_private;
-
-	if (err) {
-		if (err == -EOPNOTSUPP)
-			set_bit(BIO_EOPNOTSUPP, &bb->flags);
-		else
-			clear_bit(BIO_UPTODATE, &bb->flags);
-	}
-	if (bb)
-		if (atomic_dec_and_test(&bb->done))
-			complete(bb->wait);
-	bio_put(bio);
-}
-
 /**
  * blkdev_issue_zeroout - generate number of zero filed write bios
  * @bdev:	blockdev to issue
-- 
1.7.4.4

[PATCH 2/2] blkdev: Simple cleanup in blkdev_issue_zeroout()

[Lukas Czerner]

In blkdev_issue_zeroout() we are submitting regular WRITE bios, so we do
not need to check for -EOPNOTSUPP specifically in case of error. Also
there is no need to have label submit: because there is no way to jump
out from the while cycle without an error and we really want to exit,
rather than try again.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
CC: Dmitry Monakhov <dmonakhov@openvz.org>
CC: Jens Axboe <jens.axboe@oracle.com>
---
 block/blk-lib.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 9260cb0..6165a15 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -140,7 +140,6 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 	bb.flags = 1 << BIO_UPTODATE;
 	bb.wait = &wait;
 
-submit:
 	ret = 0;
 	while (nr_sects != 0) {
 		bio = bio_alloc(gfp_mask,
@@ -179,16 +178,6 @@ submit:
 		/* One of bios in the batch was completed with error.*/
 		ret = -EIO;
 
-	if (ret)
-		goto out;
-
-	if (test_bit(BIO_EOPNOTSUPP, &bb.flags)) {
-		ret = -EOPNOTSUPP;
-		goto out;
-	}
-	if (nr_sects != 0)
-		goto submit;
-out:
 	return ret;
 }
 EXPORT_SYMBOL(blkdev_issue_zeroout);
-- 
1.7.4.4

Re: [PATCH 1/2] blkdev: Submit discard bio in batches in blkdev_issue_discard()

[Jeff Moyer]

Lukas Czerner <lczerner@redhat.com> writes:

> Currently we are waiting for every submitted REQ_DISCARD bio separately,
> but it can have unwanted consequences of repeatedly flushing the queue,
> so we rather submit bios in batches and wait for the entire batch, hence
> narrowing the window of other ios going in.
>
> Use bio_batch_end_io() and struct bio_batch for that purpose, the same
> is used by blkdev_issue_zeroout(). Also change bio_batch_end_io() so we
> always set !BIO_UPTODATE in the case of error and remove the check for
> bb, since we are the only user of this function and we always set this.
>
> Remove bio_get()/bio_put() from the blkdev_issue_discard() since
> bio_alloc() and bio_batch_end_io() is doing the same thing, hence it is
> not needed anymore.
>
> I have done simple dd testing with surprising results. The script I have
> used is:
>
> for i in $(seq 10); do
>         echo $i
>         dd if=/dev/sdb1 of=/dev/sdc1 bs=4k &
>         sleep 5
> done
> /usr/bin/time -f %e ./blkdiscard /dev/sdc1
>
> Running time of BLKDISCARD on the whole device:
> with patch              without patch
> 0.95                    15.58
>
> So we can see that in this artificial test the kernel with the patch
> applied is approx 16x faster in discarding the device.

I don't see any major problems here.  I would like you to test
explicitly for queue_flag_discard before submitting any bios, though.
Previously, after the first bio returned EOPNOTSUPP, you errored out.
Now, you submit all bios that will end with the same failure.  It's
definitely suboptimal, and you already have the q, so you might as well
test for this early on.

Also, as I noted to you on irc, another fix required in this area is
submitting discard requests (the final one in the batch) that do not
honor the discard granularity exported by the device.  That's not
directly related to this patch, though.

Cheers,
Jeff

Re: [PATCH 2/2] blkdev: Simple cleanup in blkdev_issue_zeroout()

[Jeff Moyer]

Lukas Czerner <lczerner@redhat.com> writes:

> In blkdev_issue_zeroout() we are submitting regular WRITE bios, so we do
> not need to check for -EOPNOTSUPP specifically in case of error. Also
> there is no need to have label submit: because there is no way to jump
> out from the while cycle without an error and we really want to exit,
> rather than try again.

Looks good.  While we're at it:

	while (nr_sects != 0) {
		sz = min((sector_t) PAGE_SIZE >> 9 , nr_sects);
		if (sz == 0)
			/* bio has maximum size possible */
			break;

how could sz be equal to zero there?

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Cheers,
Jeff

Re: [PATCH 1/2] blkdev: Submit discard bio in batches in blkdev_issue_discard()

[Lukas Czerner]

On Wed, 27 Apr 2011, Jeff Moyer wrote:

> Lukas Czerner <lczerner@redhat.com> writes:
> 
> > Currently we are waiting for every submitted REQ_DISCARD bio separately,
> > but it can have unwanted consequences of repeatedly flushing the queue,
> > so we rather submit bios in batches and wait for the entire batch, hence
> > narrowing the window of other ios going in.
> >
> > Use bio_batch_end_io() and struct bio_batch for that purpose, the same
> > is used by blkdev_issue_zeroout(). Also change bio_batch_end_io() so we
> > always set !BIO_UPTODATE in the case of error and remove the check for
> > bb, since we are the only user of this function and we always set this.
> >
> > Remove bio_get()/bio_put() from the blkdev_issue_discard() since
> > bio_alloc() and bio_batch_end_io() is doing the same thing, hence it is
> > not needed anymore.
> >
> > I have done simple dd testing with surprising results. The script I have
> > used is:
> >
> > for i in $(seq 10); do
> >         echo $i
> >         dd if=/dev/sdb1 of=/dev/sdc1 bs=4k &
> >         sleep 5
> > done
> > /usr/bin/time -f %e ./blkdiscard /dev/sdc1
> >
> > Running time of BLKDISCARD on the whole device:
> > with patch              without patch
> > 0.95                    15.58
> >
> > So we can see that in this artificial test the kernel with the patch
> > applied is approx 16x faster in discarding the device.
> 
> I don't see any major problems here.  I would like you to test
> explicitly for queue_flag_discard before submitting any bios, though.
> Previously, after the first bio returned EOPNOTSUPP, you errored out.
> Now, you submit all bios that will end with the same failure.  It's
> definitely suboptimal, and you already have the q, so you might as well
> test for this early on.
> 
> Also, as I noted to you on irc, another fix required in this area is
> submitting discard requests (the final one in the batch) that do not
> honor the discard granularity exported by the device.  That's not
> directly related to this patch, though.
> 
> Cheers,
> Jeff
> 

Thanks for review and comments Jeff, I'll resend patches after I retest
them.

-Lukas

Re: [PATCH 1/2] blkdev: Submit discard bio in batches in blkdev_issue_discard()

[Lukas Czerner]

On Thu, 28 Apr 2011, Lukas Czerner wrote:

> On Wed, 27 Apr 2011, Jeff Moyer wrote:
> 
> > Lukas Czerner <lczerner@redhat.com> writes:
> > 
> > > Currently we are waiting for every submitted REQ_DISCARD bio separately,
> > > but it can have unwanted consequences of repeatedly flushing the queue,
> > > so we rather submit bios in batches and wait for the entire batch, hence
> > > narrowing the window of other ios going in.
> > >
> > > Use bio_batch_end_io() and struct bio_batch for that purpose, the same
> > > is used by blkdev_issue_zeroout(). Also change bio_batch_end_io() so we
> > > always set !BIO_UPTODATE in the case of error and remove the check for
> > > bb, since we are the only user of this function and we always set this.
> > >
> > > Remove bio_get()/bio_put() from the blkdev_issue_discard() since
> > > bio_alloc() and bio_batch_end_io() is doing the same thing, hence it is
> > > not needed anymore.
> > >
> > > I have done simple dd testing with surprising results. The script I have
> > > used is:
> > >
> > > for i in $(seq 10); do
> > >         echo $i
> > >         dd if=/dev/sdb1 of=/dev/sdc1 bs=4k &
> > >         sleep 5
> > > done
> > > /usr/bin/time -f %e ./blkdiscard /dev/sdc1
> > >
> > > Running time of BLKDISCARD on the whole device:
> > > with patch              without patch
> > > 0.95                    15.58
> > >
> > > So we can see that in this artificial test the kernel with the patch
> > > applied is approx 16x faster in discarding the device.
> > 
> > I don't see any major problems here.  I would like you to test
> > explicitly for queue_flag_discard before submitting any bios, though.

Btw, this is already in there:

	if (!blk_queue_discard(q))
		return -EOPNOTSUPP;

at the top of the function.

> > Previously, after the first bio returned EOPNOTSUPP, you errored out.
> > Now, you submit all bios that will end with the same failure.  It's
> > definitely suboptimal, and you already have the q, so you might as well
> > test for this early on.
> > 
> > Also, as I noted to you on irc, another fix required in this area is
> > submitting discard requests (the final one in the batch) that do not
> > honor the discard granularity exported by the device.  That's not
> > directly related to this patch, though.
> > 
> > Cheers,
> > Jeff
> > 
> 
> Thanks for review and comments Jeff, I'll resend patches after I retest
> them.
> 
> -Lukas
> 

-- 

Re: [PATCH 1/2] blkdev: Submit discard bio in batches in blkdev_issue_discard()

[Jeff Moyer]

Lukas Czerner <lczerner@redhat.com> writes:

> On Thu, 28 Apr 2011, Lukas Czerner wrote:
>
>> On Wed, 27 Apr 2011, Jeff Moyer wrote:
>> 
>> > Lukas Czerner <lczerner@redhat.com> writes:
>> > 
>> > > Currently we are waiting for every submitted REQ_DISCARD bio separately,
>> > > but it can have unwanted consequences of repeatedly flushing the queue,
>> > > so we rather submit bios in batches and wait for the entire batch, hence
>> > > narrowing the window of other ios going in.
>> > >
>> > > Use bio_batch_end_io() and struct bio_batch for that purpose, the same
>> > > is used by blkdev_issue_zeroout(). Also change bio_batch_end_io() so we
>> > > always set !BIO_UPTODATE in the case of error and remove the check for
>> > > bb, since we are the only user of this function and we always set this.
>> > >
>> > > Remove bio_get()/bio_put() from the blkdev_issue_discard() since
>> > > bio_alloc() and bio_batch_end_io() is doing the same thing, hence it is
>> > > not needed anymore.
>> > >
>> > > I have done simple dd testing with surprising results. The script I have
>> > > used is:
>> > >
>> > > for i in $(seq 10); do
>> > >         echo $i
>> > >         dd if=/dev/sdb1 of=/dev/sdc1 bs=4k &
>> > >         sleep 5
>> > > done
>> > > /usr/bin/time -f %e ./blkdiscard /dev/sdc1
>> > >
>> > > Running time of BLKDISCARD on the whole device:
>> > > with patch              without patch
>> > > 0.95                    15.58
>> > >
>> > > So we can see that in this artificial test the kernel with the patch
>> > > applied is approx 16x faster in discarding the device.
>> > 
>> > I don't see any major problems here.  I would like you to test
>> > explicitly for queue_flag_discard before submitting any bios, though.
>
> Btw, this is already in there:
>
> 	if (!blk_queue_discard(q))
> 		return -EOPNOTSUPP;
>
> at the top of the function.

Boy, how did I miss that?  ;-)

Cheers,
Jeff