On 2/10/22 5:08 AM, Alexander V. Buev wrote:
> Added functions to attach user PI iovec pages to bio
> and release this pages via bio_integrity_free.
>
consider :-
Added functions to attach user PI iovec pages to bio and release this
pages via bio_integrity_free.
> Signed-off-by: Alexander V. Buev <a.buev@xxxxxxxxx>
> ---
> block/bio-integrity.c | 151 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/bio.h | 8 +++
> 2 files changed, 159 insertions(+)
>
> diff --git a/block/bio-integrity.c b/block/bio-integrity.c
> index 0827b19820c5..8e57aea9c9eb 100644
> --- a/block/bio-integrity.c
> +++ b/block/bio-integrity.c
> @@ -10,6 +10,7 @@
> #include <linux/mempool.h>
> #include <linux/export.h>
> #include <linux/bio.h>
> +#include <linux/uio.h>
> #include <linux/workqueue.h>
> #include <linux/slab.h>
> #include "blk.h"
> @@ -91,6 +92,19 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
> }
> EXPORT_SYMBOL(bio_integrity_alloc);
>
> +void bio_integrity_release_pages(struct bio *bio)
> +{
> + struct bio_integrity_payload *bip = bio_integrity(bio);
> + unsigned short i;
> +
> + for (i = 0; i < bip->bip_vcnt; ++i) {
++i is common in kernel code..
> + struct bio_vec *bv;
> +
> + bv = bip->bip_vec + i;
is it possible to bip->bip_vec++ instead of above ?
> + put_page(bv->bv_page);
> + }
> +}
> +
> /**
> * bio_integrity_free - Free bio integrity payload
> * @bio: bio containing bip to be freed
> @@ -105,6 +119,10 @@ void bio_integrity_free(struct bio *bio)
>
> if (bip->bip_flags & BIP_BLOCK_INTEGRITY)
> kfree(bvec_virt(bip->bip_vec));
> + else {
> + if (bip->bip_flags & BIP_RELEASE_PAGES)
> + bio_integrity_release_pages(bio);
> + }
>
why not use if() ... else if () {} ?
> __bio_integrity_free(bs, bip);
> bio->bi_integrity = NULL;
> @@ -377,6 +395,139 @@ void bio_integrity_advance(struct bio *bio, unsigned int bytes_done)
> bvec_iter_advance(bip->bip_vec, &bip->bip_iter, bytes);
> }
>
> +static inline
> +struct page **__bio_integrity_temp_pages(struct bio *bio, unsigned int nr_needed_page)
> +{
> + struct page **pages = 0;
use of NULL please and reverse tree order declaration ..
> + unsigned int nr_avail_page;
> + struct bio_integrity_payload *bip = bio_integrity(bio);
> +
> + if (bip->bip_max_vcnt > nr_needed_page) {
> + nr_avail_page = (bip->bip_max_vcnt - nr_needed_page) *
> + sizeof(struct bio_vec)/sizeof(struct page *);
> + } else
> + nr_avail_page = 0;
> +
I'd initialize nr_avail_pages at the time of declaration and remove else
part.
> + if (nr_avail_page >= nr_needed_page)
> + pages = (struct page **) (bip->bip_vec + nr_needed_page);
> + else {
> + if (bio->bi_max_vecs - bio->bi_vcnt) {
> + nr_avail_page = (bio->bi_max_vecs - bio->bi_vcnt) *
> + sizeof(struct bio_vec)/sizeof(struct page *);
> + if (nr_avail_page >= nr_needed_page)
> + pages = (struct page **) (bio->bi_io_vec + bio->bi_vcnt);
> + }
> + }
> +
How about following much clear and removes need for else part ?
if (nr_avail_page >= nr_needed_page)
return (struct page **) (bip->bip_vec +
nr_needed_page);
if (bio->bi_max_vecs - bio->bi_vcnt) {
nr_avail_page = (bio->bi_max_vecs - bio->bi_vcnt) *
sizeof(struct bio_vec)/sizeof(struct page *);
if (nr_avail_page >= nr_needed_page)
return (struct page **) (bio->bi_io_vec +
bio->bi_vcnt);
}
return NULL;
I think if you use above then we don't have to need pages variable on
the stack in the fast path.
> + return pages;
> +}
> +
> +/**
> + * bio_integrity_add_iovec - Add PI io vector
> + * @bio: bio whose integrity vector to update
> + * @pi_iter: iov_iter pointed to data added to @bio's integrity
> + *
> + * Description: Pins pages for *pi_iov and appends them to @bio's integrity.
> + */
> +int bio_integrity_add_iovec(struct bio *bio, struct iov_iter *pi_iter)
> +{
> + struct blk_integrity *bi = bdev_get_integrity(bio->bi_bdev);
> + struct bio_integrity_payload *bip;
> + struct page **pi_page = 0, **bio_page;
> + unsigned int nr_vec_page, intervals;
> + int ret;
> + ssize_t size;
> + size_t offset, len, pg_num, page_count;
> +
> + if (unlikely(!bi && bi->tuple_size && bi->interval_exp)) {
> + pr_err("Device is not integrity capable");
> + return -EINVAL;
> + }
> +
> + intervals = bio_integrity_intervals(bi, bio_sectors(bio));
> + if (unlikely(intervals * bi->tuple_size < pi_iter->count)) {
> + pr_err("Intervals number is wrong, intervals=%u, tuple_size=%u, pi_len=%zu",
> + intervals, bi->tuple_size, pi_iter->count);
> + return -EINVAL;
> + }
> +
> + nr_vec_page = (pi_iter->count + PAGE_SIZE - 1) >> PAGE_SHIFT;
> + /* data of size N pages can be pinned to N+1 page */
> + nr_vec_page += 1;
> +
> + bip = bio_integrity_alloc(bio, GFP_NOIO, nr_vec_page);
> + if (IS_ERR(bip))
> + return PTR_ERR(bip);
> +
> + /* get space for page pointers array */
> + bio_page = __bio_integrity_temp_pages(bio, nr_vec_page);
> +
> + if (likely(bio_page))
> + pi_page = bio_page;
> + else {
> + pi_page = kcalloc(nr_vec_page, sizeof(struct pi_page *), GFP_NOIO);
overlay long line above..
> + if (!pi_page) {
> + ret = -ENOMEM;
> + goto error;
> + }
> + }
> +
> + bip->bip_iter.bi_size = pi_iter->count;
> + bip->bio_iter = bio->bi_iter;
> + bip_set_seed(bip, bio->bi_iter.bi_sector);
> +
> + if (bi->flags & BLK_INTEGRITY_IP_CHECKSUM)
> + bip->bip_flags |= BIP_IP_CHECKSUM;
> +
> + size = iov_iter_get_pages(pi_iter, pi_page, LONG_MAX, nr_vec_page, &offset);
same as above comment...
> + if (unlikely(size <= 0)) {
> + pr_err("Failed to pin PI buffer to page (%zi)", size);
> + ret = (size) ? size : -EFAULT;
> + goto error;
> + }
> +
> + /* calc count of pined pages */
> + if (size > (PAGE_SIZE - offset))
> + page_count = DIV_ROUND_UP(size - (PAGE_SIZE - offset), PAGE_SIZE) + 1;
> + else
> + page_count = 1;
> +
> + /* fill bio integrity biovecs the given pages */
> + len = pi_iter->count;
> + for (pg_num = 0; pg_num < page_count; ++pg_num) {
> + size_t page_len;
> +
> + page_len = PAGE_SIZE - offset;
> + if (page_len > len)
> + page_len = len;
> + ret = bio_integrity_add_page(bio, pi_page[pg_num], page_len, offset);
> + if (unlikely(ret != page_len)) {
> + ret = -ENOMEM;
> + goto error;
> + }
> + len -= page_len;
> + offset = 0;
> + bip->bip_flags |= BIP_RELEASE_PAGES;
> + }
> +
> + iov_iter_advance(pi_iter, size);
> +
> + if (pi_page != bio_page)
> + kfree(pi_page);
> +
> + return 0;
> +
> +error:
> + if (bio_integrity(bio))
> + bio_integrity_free(bio);
> +
> + if (pi_page && pi_page != bio_page)
> + kfree(pi_page);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(bio_integrity_add_iovec);
> +
> /**
> * bio_integrity_trim - Trim integrity vector
> * @bio: bio whose integrity vector to update
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 117d7f248ac9..ce008eeeb160 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -317,6 +317,7 @@ enum bip_flags {
> BIP_CTRL_NOCHECK = 1 << 2, /* disable HBA integrity checking */
> BIP_DISK_NOCHECK = 1 << 3, /* disable disk integrity checking */
> BIP_IP_CHECKSUM = 1 << 4, /* IP checksum */
> + BIP_RELEASE_PAGES = 1 << 5, /* release pages after io completion */
> };
>
> /*
> @@ -707,6 +708,7 @@ extern struct bio_integrity_payload *bio_integrity_alloc(struct bio *, gfp_t, un
> extern int bio_integrity_add_page(struct bio *, struct page *, unsigned int, unsigned int);
> extern bool bio_integrity_prep(struct bio *);
> extern void bio_integrity_advance(struct bio *, unsigned int);
> +extern int bio_integrity_add_iovec(struct bio *bio, struct iov_iter *iter);
can we drop extern keyword ?
> extern void bio_integrity_trim(struct bio *);
> extern int bio_integrity_clone(struct bio *, struct bio *, gfp_t);
> extern int bioset_integrity_create(struct bio_set *, int);
> @@ -747,6 +749,12 @@ static inline void bio_integrity_advance(struct bio *bio,
> return;
> }
>
> +static inline int bio_integrity_add_iovec(struct bio *bio,
> + struct iov_iter *pi_iter)
> +{
> + return 0;
> +}
> +
> static inline void bio_integrity_trim(struct bio *bio)
> {
> return;
>
