lkml.org 
[lkml]   [2023]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4 7/7] iov_iter, block: Make bio structs pin pages rather than ref'ing if appropriate
From
On 1/9/23 3:24?PM, David Howells wrote:
> Would you be okay with me flipping the logic of BIO_NO_PAGE_REF, so I end up
> with:
>
> static void bio_release_page(struct bio *bio, struct page *page)
> {
> if (bio_flagged(bio, BIO_PAGE_PINNED))
> unpin_user_page(page);
> if (bio_flagged(bio, BIO_PAGE_REFFED))
> put_page(page);
> }
>
> See attached patch.

I think it makes more sense to have NO_REF check, to be honest, as that
means the general path doesn't have to set that flag. But I don't feel
too strongly about that part.

> diff --git a/block/bio.c b/block/bio.c
> index 5f96fcae3f75..5b9f9fc62345 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -243,6 +243,11 @@ static void bio_free(struct bio *bio)
> * Users of this function have their own bio allocation. Subsequently,
> * they must remember to pair any call to bio_init() with bio_uninit()
> * when IO has completed, or when the bio is released.
> + *
> + * We set the initial assumption that pages attached to the bio will be
> + * released with put_page() by setting BIO_PAGE_REFFED, but this should be set
> + * to BIO_PAGE_PINNED if the page should be unpinned instead; if the pages
> + * should not be put or unpinned, these flags should be cleared.
> */
> void bio_init(struct bio *bio, struct block_device *bdev, struct bio_vec *table,
> unsigned short max_vecs, blk_opf_t opf)
> @@ -274,6 +279,7 @@ void bio_init(struct bio *bio, struct block_device *bdev, struct bio_vec *table,
> #ifdef CONFIG_BLK_DEV_INTEGRITY
> bio->bi_integrity = NULL;
> #endif
> + bio_set_flag(bio, BIO_PAGE_REFFED);

This is first set to zero, then you set the flag. Why not just
initialize it like that to begin with?

> @@ -302,6 +308,8 @@ void bio_reset(struct bio *bio, struct block_device *bdev, blk_opf_t opf)
> {
> bio_uninit(bio);
> memset(bio, 0, BIO_RESET_BYTES);
> + bio_set_flag(bio, BIO_PAGE_REFFED);
> + bio_clear_flag(bio, BIO_PAGE_PINNED);
> atomic_set(&bio->__bi_remaining, 1);
> bio->bi_bdev = bdev;
> if (bio->bi_bdev)

You just memset bi_flags here, surely we don't need to clear
BIO_PAGE_PINNED after that?

> @@ -814,6 +822,8 @@ static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp)
> bio_set_flag(bio, BIO_CLONED);
> bio->bi_ioprio = bio_src->bi_ioprio;
> bio->bi_iter = bio_src->bi_iter;
> + bio_clear_flag(bio, BIO_PAGE_REFFED);
> + bio_clear_flag(bio, BIO_PAGE_PINNED);

Maybe it would make sense to have a set/clear mask operation? Not
strictly required for this patch, but probably worth checking after the
fact.

> @@ -1273,12 +1295,20 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
> * result to ensure the bio's total size is correct. The remainder of
> * the iov data will be picked up in the next bio iteration.
> */
> - size = iov_iter_get_pages(iter, pages,
> - UINT_MAX - bio->bi_iter.bi_size,
> - nr_pages, &offset, gup_flags);
> + size = iov_iter_extract_pages(iter, &pages,
> + UINT_MAX - bio->bi_iter.bi_size,
> + nr_pages, gup_flags,
> + &offset, &cleanup_mode);
> if (unlikely(size <= 0))
> return size ? size : -EFAULT;
>
> + bio_clear_flag(bio, BIO_PAGE_REFFED);
> + bio_clear_flag(bio, BIO_PAGE_PINNED);
> + if (cleanup_mode & FOLL_GET)
> + bio_set_flag(bio, BIO_PAGE_REFFED);
> + if (cleanup_mode & FOLL_PIN)
> + bio_set_flag(bio, BIO_PAGE_PINNED);
> +
> nr_pages = DIV_ROUND_UP(offset + size, PAGE_SIZE);

The cleanup_mode pass-back isn't the prettiest thing in the world, and
that's a lot of arguments. Maybe it'd be slightly better if we just have
gup_flags be an output parameter too?

Also not great to first clear both flags, then set them with the two
added branches...

> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 22078a28d7cb..1c6f051f6ff2 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -482,7 +482,8 @@ void zero_fill_bio(struct bio *bio);
>
> static inline void bio_release_pages(struct bio *bio, bool mark_dirty)
> {
> - if (!bio_flagged(bio, BIO_NO_PAGE_REF))
> + if (bio_flagged(bio, BIO_PAGE_REFFED) ||
> + bio_flagged(bio, BIO_PAGE_PINNED))
> __bio_release_pages(bio, mark_dirty);
> }

Same here on a mask check, but perhaps it ends up generating the same
code?

--
Jens Axboe

\
 
 \ /
  Last update: 2023-03-26 23:34    [W:0.061 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site