* 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
* [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
* [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
* [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
* [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
* [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
* [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
* [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 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
* 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
* 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
* 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
* 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
* 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
* 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
* 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
* 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
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).