| Message ID | 20190408104641.4905-4-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] block: rewrite blk_bvec_map_sg to avoid a nth_page call | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
On Mon, 2019-04-08 at 12:46 +0200, Christoph Hellwig wrote: > No caller uses bio_iov_iter_get_pages multiple times on a given bio, > and that funtionality isn't all that useful. Removing it will make > some future changes a little easier and also simplifies the function > a bit. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/bio.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/block/bio.c b/block/bio.c > index ad346082a971..2fa624db21c7 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -958,7 +958,10 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) > int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) > { > const bool is_bvec = iov_iter_is_bvec(iter); > - unsigned short orig_vcnt = bio->bi_vcnt; > + int ret = -EFAULT; Is the value -EFAULT used anywhere? In other words, can " = -EFAULT" be left out? Otherwise this patch looks good to me. Thanks, Bart.
On Mon, Apr 08, 2019 at 03:17:35PM -0700, Bart Van Assche wrote: > On Mon, 2019-04-08 at 12:46 +0200, Christoph Hellwig wrote: > > No caller uses bio_iov_iter_get_pages multiple times on a given bio, > > and that funtionality isn't all that useful. Removing it will make > > some future changes a little easier and also simplifies the function > > a bit. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > block/bio.c | 15 ++++++--------- > > 1 file changed, 6 insertions(+), 9 deletions(-) > > > > diff --git a/block/bio.c b/block/bio.c > > index ad346082a971..2fa624db21c7 100644 > > --- a/block/bio.c > > +++ b/block/bio.c > > @@ -958,7 +958,10 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) > > int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) > > { > > const bool is_bvec = iov_iter_is_bvec(iter); > > - unsigned short orig_vcnt = bio->bi_vcnt; > > + int ret = -EFAULT; > > Is the value -EFAULT used anywhere? In other words, can " = -EFAULT" be left > out? Otherwise this patch looks good to me. True, it could be dropped. The assignment is a leftover from an earlier version of the patch.
diff --git a/block/bio.c b/block/bio.c index ad346082a971..2fa624db21c7 100644 --- a/block/bio.c +++ b/block/bio.c @@ -958,7 +958,10 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) { const bool is_bvec = iov_iter_is_bvec(iter); - unsigned short orig_vcnt = bio->bi_vcnt; + int ret = -EFAULT; + + if (WARN_ON_ONCE(bio->bi_vcnt)) + return -EINVAL; /* * If this is a BVEC iter, then the pages are kernel pages. Don't @@ -968,19 +971,13 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) bio_set_flag(bio, BIO_NO_PAGE_REF); do { - int ret; - if (is_bvec) ret = __bio_iov_bvec_add_pages(bio, iter); else ret = __bio_iov_iter_get_pages(bio, iter); + } while (!ret && iov_iter_count(iter) && !bio_full(bio)); - if (unlikely(ret)) - return bio->bi_vcnt > orig_vcnt ? 0 : ret; - - } while (iov_iter_count(iter) && !bio_full(bio)); - - return 0; + return bio->bi_vcnt ? 0 : ret; } static void submit_bio_wait_endio(struct bio *bio)
No caller uses bio_iov_iter_get_pages multiple times on a given bio, and that funtionality isn't all that useful. Removing it will make some future changes a little easier and also simplifies the function a bit. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/bio.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)