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 09-07-2012, 10:59 PM
Kent Overstreet
 
Default block: Refactor blk_update_request()

Converts it to use bio_advance(), simplifying it quite a bit in the
process.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
---
block/blk-core.c | 84 +++++++++++---------------------------------------------
1 file changed, 16 insertions(+), 68 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 82aab28..94cbdcc 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -153,25 +153,19 @@ EXPORT_SYMBOL(blk_rq_init);
static void req_bio_endio(struct request *rq, struct bio *bio,
unsigned int nbytes, int error)
{
+ /*
+ * XXX: bio_endio() does this. only need this because of the weird
+ * flush seq thing.
+ */
if (error)
clear_bit(BIO_UPTODATE, &bio->bi_flags);
else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
error = -EIO;

- if (unlikely(nbytes > bio->bi_size)) {
- printk(KERN_ERR "%s: want %u bytes done, %u left
",
- __func__, nbytes, bio->bi_size);
- nbytes = bio->bi_size;
- }
-
if (unlikely(rq->cmd_flags & REQ_QUIET))
set_bit(BIO_QUIET, &bio->bi_flags);

- bio->bi_size -= nbytes;
- bio->bi_sector += (nbytes >> 9);
-
- if (bio_integrity(bio))
- bio_integrity_advance(bio, nbytes);
+ bio_advance(bio, nbytes);

/* don't actually finish bio if it's part of flush sequence */
if (bio->bi_size == 0 && !(rq->cmd_flags & REQ_FLUSH_SEQ))
@@ -2216,8 +2210,7 @@ EXPORT_SYMBOL(blk_fetch_request);
**/
bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
{
- int total_bytes, bio_nbytes, next_idx = 0;
- struct bio *bio;
+ int total_bytes;

if (!req->bio)
return false;
@@ -2261,56 +2254,21 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)

blk_account_io_completion(req, nr_bytes);

- total_bytes = bio_nbytes = 0;
- while ((bio = req->bio) != NULL) {
- int nbytes;
+ total_bytes = 0;
+ while (req->bio) {
+ struct bio *bio = req->bio;
+ unsigned bio_bytes = min(bio->bi_size, nr_bytes);

- if (nr_bytes >= bio->bi_size) {
+ if (bio_bytes == bio->bi_size)
req->bio = bio->bi_next;
- nbytes = bio->bi_size;
- req_bio_endio(req, bio, nbytes, error);
- next_idx = 0;
- bio_nbytes = 0;
- } else {
- int idx = bio->bi_idx + next_idx;
-
- if (unlikely(idx >= bio->bi_vcnt)) {
- blk_dump_rq_flags(req, "__end_that");
- printk(KERN_ERR "%s: bio idx %d >= vcnt %d
",
- __func__, idx, bio->bi_vcnt);
- break;
- }
-
- nbytes = bio_iovec_idx(bio, idx)->bv_len;
- BIO_BUG_ON(nbytes > bio->bi_size);
-
- /*
- * not a complete bvec done
- */
- if (unlikely(nbytes > nr_bytes)) {
- bio_nbytes += nr_bytes;
- total_bytes += nr_bytes;
- break;
- }

- /*
- * advance to the next vector
- */
- next_idx++;
- bio_nbytes += nbytes;
- }
+ req_bio_endio(req, bio, bio_bytes, error);

- total_bytes += nbytes;
- nr_bytes -= nbytes;
+ total_bytes += bio_bytes;
+ nr_bytes -= bio_bytes;

- bio = req->bio;
- if (bio) {
- /*
- * end more in this run, or just return 'not-done'
- */
- if (unlikely(nr_bytes <= 0))
- break;
- }
+ if (!nr_bytes)
+ break;
}

/*
@@ -2326,16 +2284,6 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
return false;
}

- /*
- * if the request wasn't completed, update state
- */
- if (bio_nbytes) {
- req_bio_endio(req, bio, bio_nbytes, error);
- bio->bi_idx += next_idx;
- bio_iovec(bio)->bv_offset += nr_bytes;
- bio_iovec(bio)->bv_len -= nr_bytes;
- }
-
req->__data_len -= total_bytes;
req->buffer = bio_data(req->bio);

--
1.7.12

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-11-2012, 12:22 AM
Kent Overstreet
 
Default block: Refactor blk_update_request()

Converts it to use bio_advance(), simplifying it quite a bit in the
process.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
---
block/blk-core.c | 84 +++++++++++---------------------------------------------
1 file changed, 16 insertions(+), 68 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2d739ca..55c833c9 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -153,25 +153,19 @@ EXPORT_SYMBOL(blk_rq_init);
static void req_bio_endio(struct request *rq, struct bio *bio,
unsigned int nbytes, int error)
{
+ /*
+ * XXX: bio_endio() does this. only need this because of the weird
+ * flush seq thing.
+ */
if (error)
clear_bit(BIO_UPTODATE, &bio->bi_flags);
else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
error = -EIO;

- if (unlikely(nbytes > bio->bi_size)) {
- printk(KERN_ERR "%s: want %u bytes done, %u left
",
- __func__, nbytes, bio->bi_size);
- nbytes = bio->bi_size;
- }
-
if (unlikely(rq->cmd_flags & REQ_QUIET))
set_bit(BIO_QUIET, &bio->bi_flags);

- bio->bi_size -= nbytes;
- bio->bi_sector += (nbytes >> 9);
-
- if (bio_integrity(bio))
- bio_integrity_advance(bio, nbytes);
+ bio_advance(bio, nbytes);

/* don't actually finish bio if it's part of flush sequence */
if (bio->bi_size == 0 && !(rq->cmd_flags & REQ_FLUSH_SEQ))
@@ -2214,8 +2208,7 @@ EXPORT_SYMBOL(blk_fetch_request);
**/
bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
{
- int total_bytes, bio_nbytes, next_idx = 0;
- struct bio *bio;
+ int total_bytes;

if (!req->bio)
return false;
@@ -2259,56 +2252,21 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)

blk_account_io_completion(req, nr_bytes);

- total_bytes = bio_nbytes = 0;
- while ((bio = req->bio) != NULL) {
- int nbytes;
+ total_bytes = 0;
+ while (req->bio) {
+ struct bio *bio = req->bio;
+ unsigned bio_bytes = min(bio->bi_size, nr_bytes);

- if (nr_bytes >= bio->bi_size) {
+ if (bio_bytes == bio->bi_size)
req->bio = bio->bi_next;
- nbytes = bio->bi_size;
- req_bio_endio(req, bio, nbytes, error);
- next_idx = 0;
- bio_nbytes = 0;
- } else {
- int idx = bio->bi_idx + next_idx;
-
- if (unlikely(idx >= bio->bi_vcnt)) {
- blk_dump_rq_flags(req, "__end_that");
- printk(KERN_ERR "%s: bio idx %d >= vcnt %d
",
- __func__, idx, bio->bi_vcnt);
- break;
- }
-
- nbytes = bio_iovec_idx(bio, idx)->bv_len;
- BIO_BUG_ON(nbytes > bio->bi_size);
-
- /*
- * not a complete bvec done
- */
- if (unlikely(nbytes > nr_bytes)) {
- bio_nbytes += nr_bytes;
- total_bytes += nr_bytes;
- break;
- }

- /*
- * advance to the next vector
- */
- next_idx++;
- bio_nbytes += nbytes;
- }
+ req_bio_endio(req, bio, bio_bytes, error);

- total_bytes += nbytes;
- nr_bytes -= nbytes;
+ total_bytes += bio_bytes;
+ nr_bytes -= bio_bytes;

- bio = req->bio;
- if (bio) {
- /*
- * end more in this run, or just return 'not-done'
- */
- if (unlikely(nr_bytes <= 0))
- break;
- }
+ if (!nr_bytes)
+ break;
}

/*
@@ -2324,16 +2282,6 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
return false;
}

- /*
- * if the request wasn't completed, update state
- */
- if (bio_nbytes) {
- req_bio_endio(req, bio, bio_nbytes, error);
- bio->bi_idx += next_idx;
- bio_iovec(bio)->bv_offset += nr_bytes;
- bio_iovec(bio)->bv_len -= nr_bytes;
- }
-
req->__data_len -= total_bytes;
req->buffer = bio_data(req->bio);

--
1.7.12

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-20-2012, 11:20 PM
Tejun Heo
 
Default block: Refactor blk_update_request()

Hello,

On Mon, Sep 10, 2012 at 05:22:14PM -0700, Kent Overstreet wrote:
> static void req_bio_endio(struct request *rq, struct bio *bio,
> unsigned int nbytes, int error)
> {
> + /*
> + * XXX: bio_endio() does this. only need this because of the weird
> + * flush seq thing.
> + */
> if (error)
> clear_bit(BIO_UPTODATE, &bio->bi_flags);
> else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
> error = -EIO;

Isn't this also necessary to record errors on partial completions?

Other than that, I definitely like this. It would be nice to note
that the custom partial bio advancing in blk_update_request() is
replaced with multiple calls to req_bio_endio(). I don't think it has
any meaningful performance implications. It's just nice to future
readers of the commit.

Also, it would be really nice if you can verify this actually works
with partial blk_update_request(). sector update bug in the previous
patch scares me a bit. Implementing some debug hacks in the
completion path might be the easiest way to verify. A subtle bug here
could be pretty painful.

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-20-2012, 11:36 PM
Kent Overstreet
 
Default block: Refactor blk_update_request()

On Thu, Sep 20, 2012 at 04:20:00PM -0700, Tejun Heo wrote:
> Hello,
>
> On Mon, Sep 10, 2012 at 05:22:14PM -0700, Kent Overstreet wrote:
> > static void req_bio_endio(struct request *rq, struct bio *bio,
> > unsigned int nbytes, int error)
> > {
> > + /*
> > + * XXX: bio_endio() does this. only need this because of the weird
> > + * flush seq thing.
> > + */
> > if (error)
> > clear_bit(BIO_UPTODATE, &bio->bi_flags);
> > else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
> > error = -EIO;
>
> Isn't this also necessary to record errors on partial completions?

Ah yeah, you're right. Meant to delete that comment anyways.

> Other than that, I definitely like this. It would be nice to note
> that the custom partial bio advancing in blk_update_request() is
> replaced with multiple calls to req_bio_endio(). I don't think it has
> any meaningful performance implications. It's just nice to future
> readers of the commit.

The number of calls to req_bio_endio() isn't changing...
blk_update_request() called it for partial completions before. It's just
where the bio itself is updated that's getting shuffled around.

Or did you mean that bio_advance() is getting called on every bio
instead of the custom advancing in blk_update_request() before? That is
different, yeah - it's now always looping over the iovec, not just for
partial completions.

Yeah, I will note that in the commit message, in case Jens sees a
performance regression from it

> Also, it would be really nice if you can verify this actually works
> with partial blk_update_request(). sector update bug in the previous
> patch scares me a bit. Implementing some debug hacks in the
> completion path might be the easiest way to verify. A subtle bug here
> could be pretty painful.

Any suggestions on how to trigger partial updates?

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-20-2012, 11:41 PM
Tejun Heo
 
Default block: Refactor blk_update_request()

Hey,

On Thu, Sep 20, 2012 at 04:36:32PM -0700, Kent Overstreet wrote:
> > Other than that, I definitely like this. It would be nice to note
> > that the custom partial bio advancing in blk_update_request() is
> > replaced with multiple calls to req_bio_endio(). I don't think it has
> > any meaningful performance implications. It's just nice to future
> > readers of the commit.
>
> The number of calls to req_bio_endio() isn't changing...
> blk_update_request() called it for partial completions before. It's just
> where the bio itself is updated that's getting shuffled around.
>
> Or did you mean that bio_advance() is getting called on every bio
> instead of the custom advancing in blk_update_request() before? That is
> different, yeah - it's now always looping over the iovec, not just for
> partial completions.
>
> Yeah, I will note that in the commit message, in case Jens sees a
> performance regression from it

I don't think there's any performance implication. It's just nice to
explain how the complexity went away. If for nothing else, to point
out how silly the original code was.

> > Also, it would be really nice if you can verify this actually works
> > with partial blk_update_request(). sector update bug in the previous
> > patch scares me a bit. Implementing some debug hacks in the
> > completion path might be the easiest way to verify. A subtle bug here
> > could be pretty painful.
>
> Any suggestions on how to trigger partial updates?

ide along with many legacy drivers do it. Any SCSI driver including
libata only does full completion. I don't know. Even just trying to
call the function and comparing before & after with the original code
would be good. I'd like to see at least some form of verification
because the manifested bugs could be extremely nasty and difficult to
track down.

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-20-2012, 11:50 PM
Kent Overstreet
 
Default block: Refactor blk_update_request()

On Thu, Sep 20, 2012 at 04:41:33PM -0700, Tejun Heo wrote:
> Hey,
>
> On Thu, Sep 20, 2012 at 04:36:32PM -0700, Kent Overstreet wrote:
> > > Other than that, I definitely like this. It would be nice to note
> > > that the custom partial bio advancing in blk_update_request() is
> > > replaced with multiple calls to req_bio_endio(). I don't think it has
> > > any meaningful performance implications. It's just nice to future
> > > readers of the commit.
> >
> > The number of calls to req_bio_endio() isn't changing...
> > blk_update_request() called it for partial completions before. It's just
> > where the bio itself is updated that's getting shuffled around.
> >
> > Or did you mean that bio_advance() is getting called on every bio
> > instead of the custom advancing in blk_update_request() before? That is
> > different, yeah - it's now always looping over the iovec, not just for
> > partial completions.
> >
> > Yeah, I will note that in the commit message, in case Jens sees a
> > performance regression from it
>
> I don't think there's any performance implication. It's just nice to
> explain how the complexity went away. If for nothing else, to point
> out how silly the original code was.

New patch below - that commit message have what you're after?

> > > Also, it would be really nice if you can verify this actually works
> > > with partial blk_update_request(). sector update bug in the previous
> > > patch scares me a bit. Implementing some debug hacks in the
> > > completion path might be the easiest way to verify. A subtle bug here
> > > could be pretty painful.
> >
> > Any suggestions on how to trigger partial updates?
>
> ide along with many legacy drivers do it. Any SCSI driver including
> libata only does full completion. I don't know. Even just trying to
> call the function and comparing before & after with the original code
> would be good. I'd like to see at least some form of verification
> because the manifested bugs could be extremely nasty and difficult to
> track down.

Multiple partial completions should have the same semantics as a single
full completion, so maybe I'll try rigging up some test code that wraps
blk_update_request(), turning full completions into partial completions,
and verifies stuff...


commit fef0ddc82214f87de71ec6fb051eb28a6de0be74
Author: Kent Overstreet <koverstreet@google.com>
Date: Thu Sep 20 16:38:30 2012 -0700

block: Refactor blk_update_request()

Converts it to use bio_advance(), simplifying it quite a bit in the
process.

Note that req_bio_endio() now always calls bio_advance() - which means
it always loops over the biovec, not just on partial completions. Don't
expect it to affect performance, but worth noting.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>

diff --git a/block/blk-core.c b/block/blk-core.c
index 2d739ca..9f8cb16 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -158,20 +158,10 @@ static void req_bio_endio(struct request *rq, struct bio *bio,
else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
error = -EIO;

- if (unlikely(nbytes > bio->bi_size)) {
- printk(KERN_ERR "%s: want %u bytes done, %u left
",
- __func__, nbytes, bio->bi_size);
- nbytes = bio->bi_size;
- }
-
if (unlikely(rq->cmd_flags & REQ_QUIET))
set_bit(BIO_QUIET, &bio->bi_flags);

- bio->bi_size -= nbytes;
- bio->bi_sector += (nbytes >> 9);
-
- if (bio_integrity(bio))
- bio_integrity_advance(bio, nbytes);
+ bio_advance(bio, nbytes);

/* don't actually finish bio if it's part of flush sequence */
if (bio->bi_size == 0 && !(rq->cmd_flags & REQ_FLUSH_SEQ))
@@ -2214,8 +2204,7 @@ EXPORT_SYMBOL(blk_fetch_request);
**/
bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
{
- int total_bytes, bio_nbytes, next_idx = 0;
- struct bio *bio;
+ int total_bytes;

if (!req->bio)
return false;
@@ -2259,56 +2248,21 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)

blk_account_io_completion(req, nr_bytes);

- total_bytes = bio_nbytes = 0;
- while ((bio = req->bio) != NULL) {
- int nbytes;
+ total_bytes = 0;
+ while (req->bio) {
+ struct bio *bio = req->bio;
+ unsigned bio_bytes = min(bio->bi_size, nr_bytes);

- if (nr_bytes >= bio->bi_size) {
+ if (bio_bytes == bio->bi_size)
req->bio = bio->bi_next;
- nbytes = bio->bi_size;
- req_bio_endio(req, bio, nbytes, error);
- next_idx = 0;
- bio_nbytes = 0;
- } else {
- int idx = bio->bi_idx + next_idx;
-
- if (unlikely(idx >= bio->bi_vcnt)) {
- blk_dump_rq_flags(req, "__end_that");
- printk(KERN_ERR "%s: bio idx %d >= vcnt %d
",
- __func__, idx, bio->bi_vcnt);
- break;
- }
-
- nbytes = bio_iovec_idx(bio, idx)->bv_len;
- BIO_BUG_ON(nbytes > bio->bi_size);
-
- /*
- * not a complete bvec done
- */
- if (unlikely(nbytes > nr_bytes)) {
- bio_nbytes += nr_bytes;
- total_bytes += nr_bytes;
- break;
- }

- /*
- * advance to the next vector
- */
- next_idx++;
- bio_nbytes += nbytes;
- }
+ req_bio_endio(req, bio, bio_bytes, error);

- total_bytes += nbytes;
- nr_bytes -= nbytes;
+ total_bytes += bio_bytes;
+ nr_bytes -= bio_bytes;

- bio = req->bio;
- if (bio) {
- /*
- * end more in this run, or just return 'not-done'
- */
- if (unlikely(nr_bytes <= 0))
- break;
- }
+ if (!nr_bytes)
+ break;
}

/*
@@ -2324,16 +2278,6 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
return false;
}

- /*
- * if the request wasn't completed, update state
- */
- if (bio_nbytes) {
- req_bio_endio(req, bio, bio_nbytes, error);
- bio->bi_idx += next_idx;
- bio_iovec(bio)->bv_offset += nr_bytes;
- bio_iovec(bio)->bv_len -= nr_bytes;
- }
-
req->__data_len -= total_bytes;
req->buffer = bio_data(req->bio);


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-24-2012, 10:34 PM
Kent Overstreet
 
Default block: Refactor blk_update_request()

Converts it to use bio_advance(), simplifying it quite a bit in the
process.

Note that req_bio_endio() now always calls bio_advance() - which means
it always loops over the biovec, not just on partial completions. Don't
expect it to affect performance, but worth noting.

Tested it by forcing partial updates, and dumping before and after on
various bio/bvec fields when doing a partial update.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
---
block/blk-core.c | 80 +++++++++-----------------------------------------------
1 file changed, 12 insertions(+), 68 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index a17869f..a8a1a9e 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -158,20 +158,10 @@ static void req_bio_endio(struct request *rq, struct bio *bio,
else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
error = -EIO;

- if (unlikely(nbytes > bio->bi_size)) {
- printk(KERN_ERR "%s: want %u bytes done, %u left
",
- __func__, nbytes, bio->bi_size);
- nbytes = bio->bi_size;
- }
-
if (unlikely(rq->cmd_flags & REQ_QUIET))
set_bit(BIO_QUIET, &bio->bi_flags);

- bio->bi_size -= nbytes;
- bio->bi_sector += (nbytes >> 9);
-
- if (bio_integrity(bio))
- bio_integrity_advance(bio, nbytes);
+ bio_advance(bio, nbytes);

/* don't actually finish bio if it's part of flush sequence */
if (bio->bi_size == 0 && !(rq->cmd_flags & REQ_FLUSH_SEQ))
@@ -2219,8 +2209,7 @@ EXPORT_SYMBOL(blk_fetch_request);
**/
bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
{
- int total_bytes, bio_nbytes, next_idx = 0;
- struct bio *bio;
+ int total_bytes;

if (!req->bio)
return false;
@@ -2264,56 +2253,21 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)

blk_account_io_completion(req, nr_bytes);

- total_bytes = bio_nbytes = 0;
- while ((bio = req->bio) != NULL) {
- int nbytes;
+ total_bytes = 0;
+ while (req->bio) {
+ struct bio *bio = req->bio;
+ unsigned bio_bytes = min(bio->bi_size, nr_bytes);

- if (nr_bytes >= bio->bi_size) {
+ if (bio_bytes == bio->bi_size)
req->bio = bio->bi_next;
- nbytes = bio->bi_size;
- req_bio_endio(req, bio, nbytes, error);
- next_idx = 0;
- bio_nbytes = 0;
- } else {
- int idx = bio->bi_idx + next_idx;
-
- if (unlikely(idx >= bio->bi_vcnt)) {
- blk_dump_rq_flags(req, "__end_that");
- printk(KERN_ERR "%s: bio idx %d >= vcnt %d
",
- __func__, idx, bio->bi_vcnt);
- break;
- }
-
- nbytes = bio_iovec_idx(bio, idx)->bv_len;
- BIO_BUG_ON(nbytes > bio->bi_size);
-
- /*
- * not a complete bvec done
- */
- if (unlikely(nbytes > nr_bytes)) {
- bio_nbytes += nr_bytes;
- total_bytes += nr_bytes;
- break;
- }

- /*
- * advance to the next vector
- */
- next_idx++;
- bio_nbytes += nbytes;
- }
+ req_bio_endio(req, bio, bio_bytes, error);

- total_bytes += nbytes;
- nr_bytes -= nbytes;
+ total_bytes += bio_bytes;
+ nr_bytes -= bio_bytes;

- bio = req->bio;
- if (bio) {
- /*
- * end more in this run, or just return 'not-done'
- */
- if (unlikely(nr_bytes <= 0))
- break;
- }
+ if (!nr_bytes)
+ break;
}

/*
@@ -2329,16 +2283,6 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
return false;
}

- /*
- * if the request wasn't completed, update state
- */
- if (bio_nbytes) {
- req_bio_endio(req, bio, bio_nbytes, error);
- bio->bi_idx += next_idx;
- bio_iovec(bio)->bv_offset += nr_bytes;
- bio_iovec(bio)->bv_len -= nr_bytes;
- }
-
req->__data_len -= total_bytes;
req->buffer = bio_data(req->bio);

--
1.7.12

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 10-02-2012, 06:43 PM
Vivek Goyal
 
Default block: Refactor blk_update_request()

On Mon, Sep 24, 2012 at 03:34:44PM -0700, Kent Overstreet wrote:
> Converts it to use bio_advance(), simplifying it quite a bit in the
> process.
>
> Note that req_bio_endio() now always calls bio_advance() - which means
> it always loops over the biovec, not just on partial completions. Don't
> expect it to affect performance, but worth noting.
>
> Tested it by forcing partial updates, and dumping before and after on
> various bio/bvec fields when doing a partial update.
>
> Signed-off-by: Kent Overstreet <koverstreet@google.com>
> CC: Jens Axboe <axboe@kernel.dk>
> ---
> block/blk-core.c | 80 +++++++++-----------------------------------------------
> 1 file changed, 12 insertions(+), 68 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index a17869f..a8a1a9e 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -158,20 +158,10 @@ static void req_bio_endio(struct request *rq, struct bio *bio,
> else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
> error = -EIO;
>
> - if (unlikely(nbytes > bio->bi_size)) {
> - printk(KERN_ERR "%s: want %u bytes done, %u left
",
> - __func__, nbytes, bio->bi_size);
> - nbytes = bio->bi_size;
> - }
> -

You are dropping this warning because nobody is calling req_bio_endio()
with bytes greater than bio size in current code?

Otherwise this looks like a good simplification of blk_update_request()
code.

Thanks
Vivek

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 10-02-2012, 06:59 PM
Vivek Goyal
 
Default block: Refactor blk_update_request()

On Mon, Sep 24, 2012 at 03:34:44PM -0700, Kent Overstreet wrote:
> Converts it to use bio_advance(), simplifying it quite a bit in the
> process.
>
> Note that req_bio_endio() now always calls bio_advance() - which means
> it always loops over the biovec, not just on partial completions. Don't
> expect it to affect performance, but worth noting.
>
> Tested it by forcing partial updates, and dumping before and after on
> various bio/bvec fields when doing a partial update.

Kent,

In general, have you tested all these changes with FLUSH and DISCARD
bios/requests. Some of the code paths tend to break down because of these
special bios don't have payload.

Thanks
Vivek

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 10-02-2012, 08:09 PM
Kent Overstreet
 
Default block: Refactor blk_update_request()

On Tue, Oct 02, 2012 at 02:59:55PM -0400, Vivek Goyal wrote:
> On Mon, Sep 24, 2012 at 03:34:44PM -0700, Kent Overstreet wrote:
> > Converts it to use bio_advance(), simplifying it quite a bit in the
> > process.
> >
> > Note that req_bio_endio() now always calls bio_advance() - which means
> > it always loops over the biovec, not just on partial completions. Don't
> > expect it to affect performance, but worth noting.
> >
> > Tested it by forcing partial updates, and dumping before and after on
> > various bio/bvec fields when doing a partial update.
>
> Kent,
>
> In general, have you tested all these changes with FLUSH and DISCARD
> bios/requests. Some of the code paths tend to break down because of these
> special bios don't have payload.

Believe so, I should double check - also, I changed the bio_advance()
patch since the one I posted to make it handle DISCARD and WRITE_SAME
requests more explicitly.

(At this level, discards do (sometimes) have payloads, it's just always
a single page)

--
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 01:24 PM.

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