| Message ID | 20190626134928.7988-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/9] block: move the BIO_NO_PAGE_REF check into bio_release_pages | expand |
On 19-06-26 15:49:21, Christoph Hellwig wrote: > A lot of callers of bio_release_pages also want to mark the released > pages as dirty. Add a mark_dirty parameter to avoid a second > relatively expensive bio_for_each_segment_all loop. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/bio.c | 12 +++++++----- > include/linux/bio.h | 2 +- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/block/bio.c b/block/bio.c > index 9bc7d28ae997..7f3920b6baca 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -845,7 +845,7 @@ static void bio_get_pages(struct bio *bio) > get_page(bvec->bv_page); > } > > -void bio_release_pages(struct bio *bio) > +void bio_release_pages(struct bio *bio, bool mark_dirty) > { > struct bvec_iter_all iter_all; > struct bio_vec *bvec; > @@ -853,8 +853,11 @@ void bio_release_pages(struct bio *bio) > if (bio_flagged(bio, BIO_NO_PAGE_REF)) > return; > > - bio_for_each_segment_all(bvec, bio, iter_all) > + bio_for_each_segment_all(bvec, bio, iter_all) { > + if (mark_dirty && !PageCompound(bvec->bv_page)) > + set_page_dirty_lock(bvec->bv_page); Christoph, Could you please explain a bit why we should not make it dirty in case of compound page?
On 6/26/19, 1:52 PM, "linux-block-owner@vger.kernel.org on behalf of Minwoo Im" <linux-block-owner@vger.kernel.org on behalf of minwoo.im.dev@gmail.com> wrote:
On 19-06-26 15:49:21, Christoph Hellwig wrote:
> A lot of callers of bio_release_pages also want to mark the released
> pages as dirty. Add a mark_dirty parameter to avoid a second
> relatively expensive bio_for_each_segment_all loop.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> block/bio.c | 12 +++++++-----
> include/linux/bio.h | 2 +-
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 9bc7d28ae997..7f3920b6baca 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -845,7 +845,7 @@ static void bio_get_pages(struct bio *bio)
> get_page(bvec->bv_page);
> }
>
> -void bio_release_pages(struct bio *bio)
> +void bio_release_pages(struct bio *bio, bool mark_dirty)
> {
> struct bvec_iter_all iter_all;
> struct bio_vec *bvec;
> @@ -853,8 +853,11 @@ void bio_release_pages(struct bio *bio)
> if (bio_flagged(bio, BIO_NO_PAGE_REF))
> return;
>
> - bio_for_each_segment_all(bvec, bio, iter_all)
> + bio_for_each_segment_all(bvec, bio, iter_all) {
> + if (mark_dirty && !PageCompound(bvec->bv_page))
> + set_page_dirty_lock(bvec->bv_page);
Christoph,
Could you please explain a bit why we should not make it dirty in case
of compound page?
Maybe because of [PATCH 7/9] block_dev: use bio_release_pages in bio_unmap_user :-
@@ -259,13 +258,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
}
__set_current_state(TASK_RUNNING);
- bio_for_each_segment_all(bvec, &bio, iter_all) {
- if (should_dirty && !PageCompound(bvec->bv_page))
- set_page_dirty_lock(bvec->bv_page);
- if (!bio_flagged(&bio, BIO_NO_PAGE_REF))
- put_page(bvec->bv_page);
- }
-
+ bio_release_pages(&bio, should_dirty);
I'll let Christoph confirm that.
On Thu, Jun 27, 2019 at 05:52:47AM +0900, Minwoo Im wrote: > Could you please explain a bit why we should not make it dirty in case > of compound page? PageCompount is only true for the tail pages, and marking them dirty doesn't work. On the other hand marking the head page dirty covers the whole compount, so we are fine.
On 19-06-27 10:27:59, Christoph Hellwig wrote: > On Thu, Jun 27, 2019 at 05:52:47AM +0900, Minwoo Im wrote: > > Could you please explain a bit why we should not make it dirty in case > > of compound page? > > PageCompount is only true for the tail pages, and marking them dirty > doesn't work. On the other hand marking the head page dirty covers > the whole compount, so we are fine. Christoph, Thanks for letting me know this.
On 19-06-27 00:21:27, Chaitanya Kulkarni wrote: > > > On 6/26/19, 1:52 PM, "linux-block-owner@vger.kernel.org on behalf of Minwoo Im" <linux-block-owner@vger.kernel.org on behalf of minwoo.im.dev@gmail.com> wrote: > > On 19-06-26 15:49:21, Christoph Hellwig wrote: > > A lot of callers of bio_release_pages also want to mark the released > > pages as dirty. Add a mark_dirty parameter to avoid a second > > relatively expensive bio_for_each_segment_all loop. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > block/bio.c | 12 +++++++----- > > include/linux/bio.h | 2 +- > > 2 files changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/block/bio.c b/block/bio.c > > index 9bc7d28ae997..7f3920b6baca 100644 > > --- a/block/bio.c > > +++ b/block/bio.c > > @@ -845,7 +845,7 @@ static void bio_get_pages(struct bio *bio) > > get_page(bvec->bv_page); > > } > > > > -void bio_release_pages(struct bio *bio) > > +void bio_release_pages(struct bio *bio, bool mark_dirty) > > { > > struct bvec_iter_all iter_all; > > struct bio_vec *bvec; > > @@ -853,8 +853,11 @@ void bio_release_pages(struct bio *bio) > > if (bio_flagged(bio, BIO_NO_PAGE_REF)) > > return; > > > > - bio_for_each_segment_all(bvec, bio, iter_all) > > + bio_for_each_segment_all(bvec, bio, iter_all) { > > + if (mark_dirty && !PageCompound(bvec->bv_page)) > > + set_page_dirty_lock(bvec->bv_page); > > Christoph, > > Could you please explain a bit why we should not make it dirty in case > of compound page? > > Maybe because of [PATCH 7/9] block_dev: use bio_release_pages in bio_unmap_user :- > > @@ -259,13 +258,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter, > } > __set_current_state(TASK_RUNNING); > - bio_for_each_segment_all(bvec, &bio, iter_all) { > - if (should_dirty && !PageCompound(bvec->bv_page)) > - set_page_dirty_lock(bvec->bv_page); > - if (!bio_flagged(&bio, BIO_NO_PAGE_REF)) > - put_page(bvec->bv_page); > - } > - > + bio_release_pages(&bio, should_dirty); > > I'll let Christoph confirm that. Chaitanya, Thanks for sharing this. Actually I have seen that commit log but didn't catch up why the compound pages are so. Now I understood what Christoph has mentioned. Thanks for your help, again.
diff --git a/block/bio.c b/block/bio.c index 9bc7d28ae997..7f3920b6baca 100644 --- a/block/bio.c +++ b/block/bio.c @@ -845,7 +845,7 @@ static void bio_get_pages(struct bio *bio) get_page(bvec->bv_page); } -void bio_release_pages(struct bio *bio) +void bio_release_pages(struct bio *bio, bool mark_dirty) { struct bvec_iter_all iter_all; struct bio_vec *bvec; @@ -853,8 +853,11 @@ void bio_release_pages(struct bio *bio) if (bio_flagged(bio, BIO_NO_PAGE_REF)) return; - bio_for_each_segment_all(bvec, bio, iter_all) + bio_for_each_segment_all(bvec, bio, iter_all) { + if (mark_dirty && !PageCompound(bvec->bv_page)) + set_page_dirty_lock(bvec->bv_page); put_page(bvec->bv_page); + } } static int __bio_iov_bvec_add_pages(struct bio *bio, struct iov_iter *iter) @@ -1683,8 +1686,7 @@ static void bio_dirty_fn(struct work_struct *work) while ((bio = next) != NULL) { next = bio->bi_private; - bio_set_pages_dirty(bio); - bio_release_pages(bio); + bio_release_pages(bio, true); bio_put(bio); } } @@ -1700,7 +1702,7 @@ void bio_check_pages_dirty(struct bio *bio) goto defer; } - bio_release_pages(bio); + bio_release_pages(bio, false); bio_put(bio); return; defer: diff --git a/include/linux/bio.h b/include/linux/bio.h index 87e6c8637bce..9dca313d948a 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -426,7 +426,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page *page, void __bio_add_page(struct bio *bio, struct page *page, unsigned int len, unsigned int off); int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter); -void bio_release_pages(struct bio *bio); +void bio_release_pages(struct bio *bio, bool mark_dirty); struct rq_map_data; extern struct bio *bio_map_user_iov(struct request_queue *, struct iov_iter *, gfp_t);
A lot of callers of bio_release_pages also want to mark the released pages as dirty. Add a mark_dirty parameter to avoid a second relatively expensive bio_for_each_segment_all loop. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/bio.c | 12 +++++++----- include/linux/bio.h | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-)