* cleanup bio page releasing and fix a page leak
@ 2019-05-02 23:33 Christoph Hellwig
2019-05-02 23:33 ` [PATCH 1/8] block: move the BIO_NO_PAGE_REF check into bio_release_pages Christoph Hellwig
` (8 more replies)
0 siblings, 9 replies; 23+ messages in thread
From: Christoph Hellwig @ 2019-05-02 23:33 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
Hi Jens,
this series cleans up the various direct I/O and pass through
routines by switching them over to common bio helpers. For
the block device simple case this also fixes a page leak
if we were using bvec iters.
The last page just unconditionally applies the no page ref
behavior for bvec iters. I looked at all the callers, and
there is none that drops the pre-required references before
completing the request. Probably not suitable for so late
in the merge, but I wanted to get it out.
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 1/8] block: move the BIO_NO_PAGE_REF check into bio_release_pages 2019-05-02 23:33 cleanup bio page releasing and fix a page leak Christoph Hellwig @ 2019-05-02 23:33 ` Christoph Hellwig 2019-05-04 12:28 ` Johannes Thumshirn 2019-05-02 23:33 ` [PATCH 2/8] block: use bio_release_pages in bio_unmap_user Christoph Hellwig ` (7 subsequent siblings) 8 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2019-05-02 23:33 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block Move the BIO_NO_PAGE_REF check into bio_release_pages instead of duplicating it in both callers. Also make the function available outside of bio.c so that we can reuse it in other direct I/O implementations. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/bio.c | 11 ++++++----- include/linux/bio.h | 1 + 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/block/bio.c b/block/bio.c index 683cbb40f051..96ddffa49881 100644 --- a/block/bio.c +++ b/block/bio.c @@ -855,11 +855,14 @@ static void bio_get_pages(struct bio *bio) get_page(bvec->bv_page); } -static void bio_release_pages(struct bio *bio) +void bio_release_pages(struct bio *bio) { struct bvec_iter_all iter_all; struct bio_vec *bvec; + if (bio_flagged(bio, BIO_NO_PAGE_REF)) + return; + bio_for_each_segment_all(bvec, bio, iter_all) put_page(bvec->bv_page); } @@ -1692,8 +1695,7 @@ static void bio_dirty_fn(struct work_struct *work) next = bio->bi_private; bio_set_pages_dirty(bio); - if (!bio_flagged(bio, BIO_NO_PAGE_REF)) - bio_release_pages(bio); + bio_release_pages(bio); bio_put(bio); } } @@ -1709,8 +1711,7 @@ void bio_check_pages_dirty(struct bio *bio) goto defer; } - if (!bio_flagged(bio, BIO_NO_PAGE_REF)) - bio_release_pages(bio); + bio_release_pages(bio); bio_put(bio); return; defer: diff --git a/include/linux/bio.h b/include/linux/bio.h index ea73df36529a..d5699a54328e 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -427,6 +427,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); struct rq_map_data; extern struct bio *bio_map_user_iov(struct request_queue *, struct iov_iter *, gfp_t); -- 2.20.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/8] block: move the BIO_NO_PAGE_REF check into bio_release_pages 2019-05-02 23:33 ` [PATCH 1/8] block: move the BIO_NO_PAGE_REF check into bio_release_pages Christoph Hellwig @ 2019-05-04 12:28 ` Johannes Thumshirn 0 siblings, 0 replies; 23+ messages in thread From: Johannes Thumshirn @ 2019-05-04 12:28 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jens Axboe, linux-block Looks good, Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> -- Johannes Thumshirn SUSE Labs Filesystems jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/8] block: use bio_release_pages in bio_unmap_user 2019-05-02 23:33 cleanup bio page releasing and fix a page leak Christoph Hellwig 2019-05-02 23:33 ` [PATCH 1/8] block: move the BIO_NO_PAGE_REF check into bio_release_pages Christoph Hellwig @ 2019-05-02 23:33 ` Christoph Hellwig 2019-05-03 6:50 ` Nikolay Borisov 2019-05-02 23:33 ` [PATCH 3/8] block: use bio_release_pages in bio_map_user_iov Christoph Hellwig ` (6 subsequent siblings) 8 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2019-05-02 23:33 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block Use bio_release_pages and bio_set_pages_dirty instead of open coding them. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/bio.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/block/bio.c b/block/bio.c index 96ddffa49881..a6862b954350 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1445,24 +1445,6 @@ struct bio *bio_map_user_iov(struct request_queue *q, return ERR_PTR(ret); } -static void __bio_unmap_user(struct bio *bio) -{ - struct bio_vec *bvec; - struct bvec_iter_all iter_all; - - /* - * make sure we dirty pages we wrote to - */ - bio_for_each_segment_all(bvec, bio, iter_all) { - if (bio_data_dir(bio) == READ) - set_page_dirty_lock(bvec->bv_page); - - put_page(bvec->bv_page); - } - - bio_put(bio); -} - /** * bio_unmap_user - unmap a bio * @bio: the bio being unmapped @@ -1474,7 +1456,9 @@ static void __bio_unmap_user(struct bio *bio) */ void bio_unmap_user(struct bio *bio) { - __bio_unmap_user(bio); + bio_set_pages_dirty(bio); + bio_release_pages(bio); + bio_put(bio); bio_put(bio); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/8] block: use bio_release_pages in bio_unmap_user 2019-05-02 23:33 ` [PATCH 2/8] block: use bio_release_pages in bio_unmap_user Christoph Hellwig @ 2019-05-03 6:50 ` Nikolay Borisov 2019-05-03 7:20 ` Christoph Hellwig 0 siblings, 1 reply; 23+ messages in thread From: Nikolay Borisov @ 2019-05-03 6:50 UTC (permalink / raw) To: Christoph Hellwig, Jens Axboe; +Cc: linux-block On 3.05.19 г. 2:33 ч., Christoph Hellwig wrote: > Use bio_release_pages and bio_set_pages_dirty instead of open coding > them. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/bio.c | 22 +++------------------- > 1 file changed, 3 insertions(+), 19 deletions(-) > > diff --git a/block/bio.c b/block/bio.c > index 96ddffa49881..a6862b954350 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -1445,24 +1445,6 @@ struct bio *bio_map_user_iov(struct request_queue *q, > return ERR_PTR(ret); > } > > -static void __bio_unmap_user(struct bio *bio) > -{ > - struct bio_vec *bvec; > - struct bvec_iter_all iter_all; > - > - /* > - * make sure we dirty pages we wrote to > - */ > - bio_for_each_segment_all(bvec, bio, iter_all) { > - if (bio_data_dir(bio) == READ) > - set_page_dirty_lock(bvec->bv_page); > - > - put_page(bvec->bv_page); > - } > - > - bio_put(bio); > -} > - > /** > * bio_unmap_user - unmap a bio > * @bio: the bio being unmapped > @@ -1474,7 +1456,9 @@ static void __bio_unmap_user(struct bio *bio) > */ > void bio_unmap_user(struct bio *bio) > { > - __bio_unmap_user(bio); > + bio_set_pages_dirty(bio); Doesn't this need to be : if (bio_data_dir(bio) == READ) bio_set_pages_dirty() > + bio_release_pages(bio); > + bio_put(bio); > bio_put(bio); > } > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/8] block: use bio_release_pages in bio_unmap_user 2019-05-03 6:50 ` Nikolay Borisov @ 2019-05-03 7:20 ` Christoph Hellwig 0 siblings, 0 replies; 23+ messages in thread From: Christoph Hellwig @ 2019-05-03 7:20 UTC (permalink / raw) To: Nikolay Borisov; +Cc: Christoph Hellwig, Jens Axboe, linux-block On Fri, May 03, 2019 at 09:50:08AM +0300, Nikolay Borisov wrote: > > @@ -1474,7 +1456,9 @@ static void __bio_unmap_user(struct bio *bio) > > */ > > void bio_unmap_user(struct bio *bio) > > { > > - __bio_unmap_user(bio); > > + bio_set_pages_dirty(bio); > > Doesn't this need to be : > > if (bio_data_dir(bio) == READ) > bio_set_pages_dirty() Yes, indeed. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/8] block: use bio_release_pages in bio_map_user_iov 2019-05-02 23:33 cleanup bio page releasing and fix a page leak Christoph Hellwig 2019-05-02 23:33 ` [PATCH 1/8] block: move the BIO_NO_PAGE_REF check into bio_release_pages Christoph Hellwig 2019-05-02 23:33 ` [PATCH 2/8] block: use bio_release_pages in bio_unmap_user Christoph Hellwig @ 2019-05-02 23:33 ` Christoph Hellwig 2019-05-04 12:31 ` Johannes Thumshirn 2019-05-02 23:33 ` [PATCH 4/8] iomap: use bio_release_pages in iomap_dio_bio_end_io Christoph Hellwig ` (5 subsequent siblings) 8 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2019-05-02 23:33 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block Use bio_release_pages instead of open coding it. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/bio.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/block/bio.c b/block/bio.c index a6862b954350..3938e179a530 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1370,8 +1370,6 @@ struct bio *bio_map_user_iov(struct request_queue *q, int j; struct bio *bio; int ret; - struct bio_vec *bvec; - struct bvec_iter_all iter_all; if (!iov_iter_count(iter)) return ERR_PTR(-EINVAL); @@ -1438,9 +1436,7 @@ struct bio *bio_map_user_iov(struct request_queue *q, return bio; out_unmap: - bio_for_each_segment_all(bvec, bio, iter_all) { - put_page(bvec->bv_page); - } + bio_release_pages(bio); bio_put(bio); return ERR_PTR(ret); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 3/8] block: use bio_release_pages in bio_map_user_iov 2019-05-02 23:33 ` [PATCH 3/8] block: use bio_release_pages in bio_map_user_iov Christoph Hellwig @ 2019-05-04 12:31 ` Johannes Thumshirn 0 siblings, 0 replies; 23+ messages in thread From: Johannes Thumshirn @ 2019-05-04 12:31 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jens Axboe, linux-block Looks good, Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> -- Johannes Thumshirn SUSE Labs Filesystems jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/8] iomap: use bio_release_pages in iomap_dio_bio_end_io 2019-05-02 23:33 cleanup bio page releasing and fix a page leak Christoph Hellwig ` (2 preceding siblings ...) 2019-05-02 23:33 ` [PATCH 3/8] block: use bio_release_pages in bio_map_user_iov Christoph Hellwig @ 2019-05-02 23:33 ` Christoph Hellwig 2019-05-04 12:33 ` Johannes Thumshirn 2019-05-02 23:33 ` [PATCH 5/8] block_dev: use bio_release_pages in blkdev_bio_end_io Christoph Hellwig ` (4 subsequent siblings) 8 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2019-05-02 23:33 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block Use bio_release_pages instead of duplicating it. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/iomap.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/fs/iomap.c b/fs/iomap.c index 12a656271076..a7bff5b2e1e8 100644 --- a/fs/iomap.c +++ b/fs/iomap.c @@ -1588,13 +1588,7 @@ static void iomap_dio_bio_end_io(struct bio *bio) if (should_dirty) { bio_check_pages_dirty(bio); } else { - if (!bio_flagged(bio, BIO_NO_PAGE_REF)) { - struct bvec_iter_all iter_all; - struct bio_vec *bvec; - - bio_for_each_segment_all(bvec, bio, iter_all) - put_page(bvec->bv_page); - } + bio_release_pages(bio); bio_put(bio); } } -- 2.20.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 4/8] iomap: use bio_release_pages in iomap_dio_bio_end_io 2019-05-02 23:33 ` [PATCH 4/8] iomap: use bio_release_pages in iomap_dio_bio_end_io Christoph Hellwig @ 2019-05-04 12:33 ` Johannes Thumshirn 2019-05-04 12:35 ` Johannes Thumshirn 0 siblings, 1 reply; 23+ messages in thread From: Johannes Thumshirn @ 2019-05-04 12:33 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jens Axboe, linux-block On Thu, May 02, 2019 at 07:33:28PM -0400, Christoph Hellwig wrote: > @@ -1588,13 +1588,7 @@ static void iomap_dio_bio_end_io(struct bio *bio) > if (should_dirty) { > bio_check_pages_dirty(bio); > } else { > - if (!bio_flagged(bio, BIO_NO_PAGE_REF)) { > - struct bvec_iter_all iter_all; > - struct bio_vec *bvec; > - > - bio_for_each_segment_all(bvec, bio, iter_all) > - put_page(bvec->bv_page); > - } > + bio_release_pages(bio); Shouldn't this rather be: if (!bio_flagged(bio, BIO_NO_PAGE_REF)) bio_release_pages(bio); -- Johannes Thumshirn SUSE Labs Filesystems jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/8] iomap: use bio_release_pages in iomap_dio_bio_end_io 2019-05-04 12:33 ` Johannes Thumshirn @ 2019-05-04 12:35 ` Johannes Thumshirn 0 siblings, 0 replies; 23+ messages in thread From: Johannes Thumshirn @ 2019-05-04 12:35 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jens Axboe, linux-block On Sat, May 04, 2019 at 02:33:09PM +0200, Johannes Thumshirn wrote: > > Shouldn't this rather be: > > if (!bio_flagged(bio, BIO_NO_PAGE_REF)) > bio_release_pages(bio); OK I apparently can't remember between 3 emails, sorry Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> -- Johannes Thumshirn SUSE Labs Filesystems jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 5/8] block_dev: use bio_release_pages in blkdev_bio_end_io 2019-05-02 23:33 cleanup bio page releasing and fix a page leak Christoph Hellwig ` (3 preceding siblings ...) 2019-05-02 23:33 ` [PATCH 4/8] iomap: use bio_release_pages in iomap_dio_bio_end_io Christoph Hellwig @ 2019-05-02 23:33 ` Christoph Hellwig 2019-05-04 12:35 ` Johannes Thumshirn 2019-05-02 23:33 ` [PATCH 6/8] block_dev: use bio_release_pages in __blkdev_direct_IO_simple Christoph Hellwig ` (3 subsequent siblings) 8 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2019-05-02 23:33 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block Use bio_release_pages instead of duplicating it. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/block_dev.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/fs/block_dev.c b/fs/block_dev.c index 8abc6570d29f..a59ebea9d125 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -335,13 +335,7 @@ static void blkdev_bio_end_io(struct bio *bio) if (should_dirty) { bio_check_pages_dirty(bio); } else { - if (!bio_flagged(bio, BIO_NO_PAGE_REF)) { - struct bvec_iter_all iter_all; - struct bio_vec *bvec; - - bio_for_each_segment_all(bvec, bio, iter_all) - put_page(bvec->bv_page); - } + bio_release_pages(bio); bio_put(bio); } } -- 2.20.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 5/8] block_dev: use bio_release_pages in blkdev_bio_end_io 2019-05-02 23:33 ` [PATCH 5/8] block_dev: use bio_release_pages in blkdev_bio_end_io Christoph Hellwig @ 2019-05-04 12:35 ` Johannes Thumshirn 0 siblings, 0 replies; 23+ messages in thread From: Johannes Thumshirn @ 2019-05-04 12:35 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jens Axboe, linux-block Looks good, Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> -- Johannes Thumshirn SUSE Labs Filesystems jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 6/8] block_dev: use bio_release_pages in __blkdev_direct_IO_simple 2019-05-02 23:33 cleanup bio page releasing and fix a page leak Christoph Hellwig ` (4 preceding siblings ...) 2019-05-02 23:33 ` [PATCH 5/8] block_dev: use bio_release_pages in blkdev_bio_end_io Christoph Hellwig @ 2019-05-02 23:33 ` Christoph Hellwig 2019-05-04 17:38 ` Johannes Thumshirn 2019-05-02 23:33 ` [PATCH 7/8] direct-io: use bio_release_pages in dio_bio_complete Christoph Hellwig ` (2 subsequent siblings) 8 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2019-05-02 23:33 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block Use bio_release_pages instead of a manual put_pages loop that fails to take BIO_NO_PAGE_REF into account. Also use bio_set_pages_dirty for the rest of the former loop to reduce code duplication. Fixes: 399254aaf489 ("block: add BIO_NO_PAGE_REF flag") Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/block_dev.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/fs/block_dev.c b/fs/block_dev.c index a59ebea9d125..4fc2377884a3 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -204,13 +204,12 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter, { struct file *file = iocb->ki_filp; struct block_device *bdev = I_BDEV(bdev_file_inode(file)); - struct bio_vec inline_vecs[DIO_INLINE_BIO_VECS], *vecs, *bvec; + struct bio_vec inline_vecs[DIO_INLINE_BIO_VECS], *vecs; loff_t pos = iocb->ki_pos; bool should_dirty = false; struct bio bio; ssize_t ret; blk_qc_t qc; - struct bvec_iter_all iter_all; if ((pos | iov_iter_alignment(iter)) & (bdev_logical_block_size(bdev) - 1)) @@ -260,11 +259,9 @@ __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); - put_page(bvec->bv_page); - } + if (should_dirty) + bio_set_pages_dirty(&bio); + bio_release_pages(&bio); if (unlikely(bio.bi_status)) ret = blk_status_to_errno(bio.bi_status); -- 2.20.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 6/8] block_dev: use bio_release_pages in __blkdev_direct_IO_simple 2019-05-02 23:33 ` [PATCH 6/8] block_dev: use bio_release_pages in __blkdev_direct_IO_simple Christoph Hellwig @ 2019-05-04 17:38 ` Johannes Thumshirn 0 siblings, 0 replies; 23+ messages in thread From: Johannes Thumshirn @ 2019-05-04 17:38 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jens Axboe, linux-block Looks good, Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> -- Johannes Thumshirn SUSE Labs Filesystems jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 7/8] direct-io: use bio_release_pages in dio_bio_complete 2019-05-02 23:33 cleanup bio page releasing and fix a page leak Christoph Hellwig ` (5 preceding siblings ...) 2019-05-02 23:33 ` [PATCH 6/8] block_dev: use bio_release_pages in __blkdev_direct_IO_simple Christoph Hellwig @ 2019-05-02 23:33 ` Christoph Hellwig 2019-05-04 17:39 ` Johannes Thumshirn 2019-05-02 23:33 ` [PATCH 8/8] block: never take page references for ITER_BVEC Christoph Hellwig 2019-05-03 17:52 ` cleanup bio page releasing and fix a page leak Jens Axboe 8 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2019-05-02 23:33 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block Use bio_release_pages and bio_set_pages_dirty instead of open coding them. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/direct-io.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/fs/direct-io.c b/fs/direct-io.c index fbe885d68035..62d0594a4622 100644 --- a/fs/direct-io.c +++ b/fs/direct-io.c @@ -537,7 +537,6 @@ static struct bio *dio_await_one(struct dio *dio) */ static blk_status_t dio_bio_complete(struct dio *dio, struct bio *bio) { - struct bio_vec *bvec; blk_status_t err = bio->bi_status; if (err) { @@ -550,17 +549,9 @@ static blk_status_t dio_bio_complete(struct dio *dio, struct bio *bio) if (dio->is_async && dio->op == REQ_OP_READ && dio->should_dirty) { bio_check_pages_dirty(bio); /* transfers ownership */ } else { - struct bvec_iter_all iter_all; - - bio_for_each_segment_all(bvec, bio, iter_all) { - struct page *page = bvec->bv_page; - - if (dio->op == REQ_OP_READ && !PageCompound(page) && - dio->should_dirty) - set_page_dirty_lock(page); - put_page(page); - } - bio_put(bio); + if (dio->op == REQ_OP_READ && dio->should_dirty) + bio_set_pages_dirty(bio); + bio_release_pages(bio); } return err; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 7/8] direct-io: use bio_release_pages in dio_bio_complete 2019-05-02 23:33 ` [PATCH 7/8] direct-io: use bio_release_pages in dio_bio_complete Christoph Hellwig @ 2019-05-04 17:39 ` Johannes Thumshirn 0 siblings, 0 replies; 23+ messages in thread From: Johannes Thumshirn @ 2019-05-04 17:39 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jens Axboe, linux-block Looks good, Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> -- Johannes Thumshirn SUSE Labs Filesystems jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 8/8] block: never take page references for ITER_BVEC 2019-05-02 23:33 cleanup bio page releasing and fix a page leak Christoph Hellwig ` (6 preceding siblings ...) 2019-05-02 23:33 ` [PATCH 7/8] direct-io: use bio_release_pages in dio_bio_complete Christoph Hellwig @ 2019-05-02 23:33 ` Christoph Hellwig 2019-05-04 17:41 ` Johannes Thumshirn 2019-05-06 8:19 ` Ming Lei 2019-05-03 17:52 ` cleanup bio page releasing and fix a page leak Jens Axboe 8 siblings, 2 replies; 23+ messages in thread From: Christoph Hellwig @ 2019-05-02 23:33 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block If we pass pages through an iov_iter we always already have a reference in the caller. Thus remove the ITER_BVEC_FLAG_NO_REF and don't take reference to pages by default for bvec backed iov_iters. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/bio.c | 14 +------------- drivers/block/loop.c | 16 ++++------------ fs/io_uring.c | 3 --- include/linux/uio.h | 8 -------- 4 files changed, 5 insertions(+), 36 deletions(-) diff --git a/block/bio.c b/block/bio.c index 3938e179a530..e999d530d863 100644 --- a/block/bio.c +++ b/block/bio.c @@ -846,15 +846,6 @@ int bio_add_page(struct bio *bio, struct page *page, } EXPORT_SYMBOL(bio_add_page); -static void bio_get_pages(struct bio *bio) -{ - struct bvec_iter_all iter_all; - struct bio_vec *bvec; - - bio_for_each_segment_all(bvec, bio, iter_all) - get_page(bvec->bv_page); -} - void bio_release_pages(struct bio *bio) { struct bvec_iter_all iter_all; @@ -967,11 +958,8 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) ret = __bio_iov_iter_get_pages(bio, iter); } while (!ret && iov_iter_count(iter) && !bio_full(bio)); - if (iov_iter_bvec_no_ref(iter)) + if (is_bvec) bio_set_flag(bio, BIO_NO_PAGE_REF); - else if (is_bvec) - bio_get_pages(bio); - return bio->bi_vcnt ? 0 : ret; } diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 102d79575895..c20710e617c2 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -264,20 +264,12 @@ lo_do_transfer(struct loop_device *lo, int cmd, return ret; } -static inline void loop_iov_iter_bvec(struct iov_iter *i, - unsigned int direction, const struct bio_vec *bvec, - unsigned long nr_segs, size_t count) -{ - iov_iter_bvec(i, direction, bvec, nr_segs, count); - i->type |= ITER_BVEC_FLAG_NO_REF; -} - static int lo_write_bvec(struct file *file, struct bio_vec *bvec, loff_t *ppos) { struct iov_iter i; ssize_t bw; - loop_iov_iter_bvec(&i, WRITE, bvec, 1, bvec->bv_len); + iov_iter_bvec(&i, WRITE, bvec, 1, bvec->bv_len); file_start_write(file); bw = vfs_iter_write(file, &i, ppos, 0); @@ -355,7 +347,7 @@ static int lo_read_simple(struct loop_device *lo, struct request *rq, ssize_t len; rq_for_each_segment(bvec, rq, iter) { - loop_iov_iter_bvec(&i, READ, &bvec, 1, bvec.bv_len); + iov_iter_bvec(&i, READ, &bvec, 1, bvec.bv_len); len = vfs_iter_read(lo->lo_backing_file, &i, &pos, 0); if (len < 0) return len; @@ -396,7 +388,7 @@ static int lo_read_transfer(struct loop_device *lo, struct request *rq, b.bv_offset = 0; b.bv_len = bvec.bv_len; - loop_iov_iter_bvec(&i, READ, &b, 1, b.bv_len); + iov_iter_bvec(&i, READ, &b, 1, b.bv_len); len = vfs_iter_read(lo->lo_backing_file, &i, &pos, 0); if (len < 0) { ret = len; @@ -563,7 +555,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd, } atomic_set(&cmd->ref, 2); - loop_iov_iter_bvec(&iter, rw, bvec, nr_bvec, blk_rq_bytes(rq)); + iov_iter_bvec(&iter, rw, bvec, nr_bvec, blk_rq_bytes(rq)); iter.iov_offset = offset; cmd->iocb.ki_pos = pos; diff --git a/fs/io_uring.c b/fs/io_uring.c index f65f85d89217..f7eb63a5b3db 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -853,9 +853,6 @@ static int io_import_fixed(struct io_ring_ctx *ctx, int rw, iov_iter_bvec(iter, rw, imu->bvec, imu->nr_bvecs, offset + len); if (offset) iov_iter_advance(iter, offset); - - /* don't drop a reference to these pages */ - iter->type |= ITER_BVEC_FLAG_NO_REF; return 0; } diff --git a/include/linux/uio.h b/include/linux/uio.h index f184af1999a8..bace8fd40d0c 100644 --- a/include/linux/uio.h +++ b/include/linux/uio.h @@ -23,9 +23,6 @@ struct kvec { }; enum iter_type { - /* set if ITER_BVEC doesn't hold a bv_page ref */ - ITER_BVEC_FLAG_NO_REF = 2, - /* iter types */ ITER_IOVEC = 4, ITER_KVEC = 8, @@ -93,11 +90,6 @@ static inline unsigned char iov_iter_rw(const struct iov_iter *i) return i->type & (READ | WRITE); } -static inline bool iov_iter_bvec_no_ref(const struct iov_iter *i) -{ - return (i->type & ITER_BVEC_FLAG_NO_REF) != 0; -} - /* * Total number of bytes covered by an iovec. * -- 2.20.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 8/8] block: never take page references for ITER_BVEC 2019-05-02 23:33 ` [PATCH 8/8] block: never take page references for ITER_BVEC Christoph Hellwig @ 2019-05-04 17:41 ` Johannes Thumshirn 2019-05-06 8:19 ` Ming Lei 1 sibling, 0 replies; 23+ messages in thread From: Johannes Thumshirn @ 2019-05-04 17:41 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jens Axboe, linux-block Looks good, Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> -- Johannes Thumshirn SUSE Labs Filesystems jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 8/8] block: never take page references for ITER_BVEC 2019-05-02 23:33 ` [PATCH 8/8] block: never take page references for ITER_BVEC Christoph Hellwig 2019-05-04 17:41 ` Johannes Thumshirn @ 2019-05-06 8:19 ` Ming Lei 2019-05-06 13:30 ` Christoph Hellwig 1 sibling, 1 reply; 23+ messages in thread From: Ming Lei @ 2019-05-06 8:19 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jens Axboe, linux-block On Thu, May 02, 2019 at 07:33:32PM -0400, Christoph Hellwig wrote: > If we pass pages through an iov_iter we always already have a reference > in the caller. Thus remove the ITER_BVEC_FLAG_NO_REF and don't take > reference to pages by default for bvec backed iov_iters. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/bio.c | 14 +------------- > drivers/block/loop.c | 16 ++++------------ > fs/io_uring.c | 3 --- > include/linux/uio.h | 8 -------- > 4 files changed, 5 insertions(+), 36 deletions(-) > > diff --git a/block/bio.c b/block/bio.c > index 3938e179a530..e999d530d863 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -846,15 +846,6 @@ int bio_add_page(struct bio *bio, struct page *page, > } > EXPORT_SYMBOL(bio_add_page); > > -static void bio_get_pages(struct bio *bio) > -{ > - struct bvec_iter_all iter_all; > - struct bio_vec *bvec; > - > - bio_for_each_segment_all(bvec, bio, iter_all) > - get_page(bvec->bv_page); > -} > - > void bio_release_pages(struct bio *bio) > { > struct bvec_iter_all iter_all; > @@ -967,11 +958,8 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) > ret = __bio_iov_iter_get_pages(bio, iter); > } while (!ret && iov_iter_count(iter) && !bio_full(bio)); > > - if (iov_iter_bvec_no_ref(iter)) > + if (is_bvec) > bio_set_flag(bio, BIO_NO_PAGE_REF); > - else if (is_bvec) > - bio_get_pages(bio); > - > return bio->bi_vcnt ? 0 : ret; > } > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 102d79575895..c20710e617c2 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -264,20 +264,12 @@ lo_do_transfer(struct loop_device *lo, int cmd, > return ret; > } > > -static inline void loop_iov_iter_bvec(struct iov_iter *i, > - unsigned int direction, const struct bio_vec *bvec, > - unsigned long nr_segs, size_t count) > -{ > - iov_iter_bvec(i, direction, bvec, nr_segs, count); > - i->type |= ITER_BVEC_FLAG_NO_REF; > -} > - > static int lo_write_bvec(struct file *file, struct bio_vec *bvec, loff_t *ppos) > { > struct iov_iter i; > ssize_t bw; > > - loop_iov_iter_bvec(&i, WRITE, bvec, 1, bvec->bv_len); > + iov_iter_bvec(&i, WRITE, bvec, 1, bvec->bv_len); > > file_start_write(file); > bw = vfs_iter_write(file, &i, ppos, 0); > @@ -355,7 +347,7 @@ static int lo_read_simple(struct loop_device *lo, struct request *rq, > ssize_t len; > > rq_for_each_segment(bvec, rq, iter) { > - loop_iov_iter_bvec(&i, READ, &bvec, 1, bvec.bv_len); > + iov_iter_bvec(&i, READ, &bvec, 1, bvec.bv_len); > len = vfs_iter_read(lo->lo_backing_file, &i, &pos, 0); > if (len < 0) > return len; > @@ -396,7 +388,7 @@ static int lo_read_transfer(struct loop_device *lo, struct request *rq, > b.bv_offset = 0; > b.bv_len = bvec.bv_len; > > - loop_iov_iter_bvec(&i, READ, &b, 1, b.bv_len); > + iov_iter_bvec(&i, READ, &b, 1, b.bv_len); > len = vfs_iter_read(lo->lo_backing_file, &i, &pos, 0); > if (len < 0) { > ret = len; > @@ -563,7 +555,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd, > } > atomic_set(&cmd->ref, 2); > > - loop_iov_iter_bvec(&iter, rw, bvec, nr_bvec, blk_rq_bytes(rq)); > + iov_iter_bvec(&iter, rw, bvec, nr_bvec, blk_rq_bytes(rq)); > iter.iov_offset = offset; > > cmd->iocb.ki_pos = pos; > diff --git a/fs/io_uring.c b/fs/io_uring.c > index f65f85d89217..f7eb63a5b3db 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -853,9 +853,6 @@ static int io_import_fixed(struct io_ring_ctx *ctx, int rw, > iov_iter_bvec(iter, rw, imu->bvec, imu->nr_bvecs, offset + len); > if (offset) > iov_iter_advance(iter, offset); > - > - /* don't drop a reference to these pages */ > - iter->type |= ITER_BVEC_FLAG_NO_REF; > return 0; > } > > diff --git a/include/linux/uio.h b/include/linux/uio.h > index f184af1999a8..bace8fd40d0c 100644 > --- a/include/linux/uio.h > +++ b/include/linux/uio.h > @@ -23,9 +23,6 @@ struct kvec { > }; > > enum iter_type { > - /* set if ITER_BVEC doesn't hold a bv_page ref */ > - ITER_BVEC_FLAG_NO_REF = 2, > - > /* iter types */ > ITER_IOVEC = 4, > ITER_KVEC = 8, > @@ -93,11 +90,6 @@ static inline unsigned char iov_iter_rw(const struct iov_iter *i) > return i->type & (READ | WRITE); > } > > -static inline bool iov_iter_bvec_no_ref(const struct iov_iter *i) > -{ > - return (i->type & ITER_BVEC_FLAG_NO_REF) != 0; > -} > - > /* > * Total number of bytes covered by an iovec. > * I remember that this way is the initial version of Jens' patch, however kernel bug is triggered: https://lore.kernel.org/linux-block/20190226034613.GA676@sol.localdomain/ Or maybe I miss some recent changes, could you explain it a bit? Thanks, Ming ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 8/8] block: never take page references for ITER_BVEC 2019-05-06 8:19 ` Ming Lei @ 2019-05-06 13:30 ` Christoph Hellwig 2019-05-07 6:09 ` Christoph Hellwig 0 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2019-05-06 13:30 UTC (permalink / raw) To: Ming Lei; +Cc: Christoph Hellwig, Jens Axboe, linux-block On Mon, May 06, 2019 at 04:19:54PM +0800, Ming Lei wrote: > I remember that this way is the initial version of Jens' patch, however > kernel bug is triggered: > > https://lore.kernel.org/linux-block/20190226034613.GA676@sol.localdomain/ > > Or maybe I miss some recent changes, could you explain it a bit? I'm not even sure how the original would crash.. When I run the rest locally it gets and EBUSY from ioctl LOOP_SET_FD, so I'm not sure what it tends up testing in the end. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 8/8] block: never take page references for ITER_BVEC 2019-05-06 13:30 ` Christoph Hellwig @ 2019-05-07 6:09 ` Christoph Hellwig 0 siblings, 0 replies; 23+ messages in thread From: Christoph Hellwig @ 2019-05-07 6:09 UTC (permalink / raw) To: Ming Lei; +Cc: Christoph Hellwig, Jens Axboe, linux-block On Mon, May 06, 2019 at 03:30:27PM +0200, Christoph Hellwig wrote: > On Mon, May 06, 2019 at 04:19:54PM +0800, Ming Lei wrote: > > I remember that this way is the initial version of Jens' patch, however > > kernel bug is triggered: > > > > https://lore.kernel.org/linux-block/20190226034613.GA676@sol.localdomain/ > > > > Or maybe I miss some recent changes, could you explain it a bit? > > I'm not even sure how the original would crash.. When I run the > rest locally it gets and EBUSY from ioctl LOOP_SET_FD, so I'm > not sure what it tends up testing in the end. I also did another run with KASAN enabled and a clean environment, this gives no EBUSY and still works fine. I still don't understand what the original problem might have been here, though. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: cleanup bio page releasing and fix a page leak 2019-05-02 23:33 cleanup bio page releasing and fix a page leak Christoph Hellwig ` (7 preceding siblings ...) 2019-05-02 23:33 ` [PATCH 8/8] block: never take page references for ITER_BVEC Christoph Hellwig @ 2019-05-03 17:52 ` Jens Axboe 8 siblings, 0 replies; 23+ messages in thread From: Jens Axboe @ 2019-05-03 17:52 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-block On 5/2/19 5:33 PM, Christoph Hellwig wrote: > Hi Jens, > > this series cleans up the various direct I/O and pass through > routines by switching them over to common bio helpers. For > the block device simple case this also fixes a page leak > if we were using bvec iters. > > The last page just unconditionally applies the no page ref > behavior for bvec iters. I looked at all the callers, and > there is none that drops the pre-required references before > completing the request. Probably not suitable for so late > in the merge, but I wanted to get it out. I'm guessing you'll spin a v2 of this due to the issue that Nikolay pointed out. I'd suggest we then just queue it up for later in the merge window inclusion. -- Jens Axboe ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2019-05-07 6:09 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-02 23:33 cleanup bio page releasing and fix a page leak Christoph Hellwig 2019-05-02 23:33 ` [PATCH 1/8] block: move the BIO_NO_PAGE_REF check into bio_release_pages Christoph Hellwig 2019-05-04 12:28 ` Johannes Thumshirn 2019-05-02 23:33 ` [PATCH 2/8] block: use bio_release_pages in bio_unmap_user Christoph Hellwig 2019-05-03 6:50 ` Nikolay Borisov 2019-05-03 7:20 ` Christoph Hellwig 2019-05-02 23:33 ` [PATCH 3/8] block: use bio_release_pages in bio_map_user_iov Christoph Hellwig 2019-05-04 12:31 ` Johannes Thumshirn 2019-05-02 23:33 ` [PATCH 4/8] iomap: use bio_release_pages in iomap_dio_bio_end_io Christoph Hellwig 2019-05-04 12:33 ` Johannes Thumshirn 2019-05-04 12:35 ` Johannes Thumshirn 2019-05-02 23:33 ` [PATCH 5/8] block_dev: use bio_release_pages in blkdev_bio_end_io Christoph Hellwig 2019-05-04 12:35 ` Johannes Thumshirn 2019-05-02 23:33 ` [PATCH 6/8] block_dev: use bio_release_pages in __blkdev_direct_IO_simple Christoph Hellwig 2019-05-04 17:38 ` Johannes Thumshirn 2019-05-02 23:33 ` [PATCH 7/8] direct-io: use bio_release_pages in dio_bio_complete Christoph Hellwig 2019-05-04 17:39 ` Johannes Thumshirn 2019-05-02 23:33 ` [PATCH 8/8] block: never take page references for ITER_BVEC Christoph Hellwig 2019-05-04 17:41 ` Johannes Thumshirn 2019-05-06 8:19 ` Ming Lei 2019-05-06 13:30 ` Christoph Hellwig 2019-05-07 6:09 ` Christoph Hellwig 2019-05-03 17:52 ` cleanup bio page releasing and fix a page leak Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).