Re: [PATCH 1/3] block: bio-integrity: add PI iovec to bio

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 10/28/21 4:24 AM, Alexander V. Buev wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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@yadro.com>
> ---
>   block/bio-integrity.c | 124 +++++++++++++++++++++++++++++++++++++++++-
>   include/linux/bio.h   |   8 +++
>   2 files changed, 131 insertions(+), 1 deletion(-)
> 
> diff --git a/block/bio-integrity.c b/block/bio-integrity.c
> index 6b47cddbbca1..3e12cfa806ff 100644
> --- a/block/bio-integrity.c
> +++ b/block/bio-integrity.c
> @@ -5,11 +5,11 @@
>    * Copyright (C) 2007, 2008, 2009 Oracle Corporation
>    * Written by: Martin K. Petersen <martin.petersen@oracle.com>
>    */
> -
>   #include <linux/blkdev.h>
>   #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 +91,17 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
>   }
>   EXPORT_SYMBOL(bio_integrity_alloc);
> 
> +void bio_integrity_payload_release_pages(struct bio_integrity_payload *bip)
> +{
> +       unsigned short i;
> +       struct bio_vec *bv;
> +
> +       for (i = 0; i < bip->bip_vcnt; ++i) {
> +               bv = bip->bip_vec + i;
> +               put_page(bv->bv_page);
> +       }
> +}
> +
>   /**
>    * bio_integrity_free - Free bio integrity payload
>    * @bio:       bio containing bip to be freed
> @@ -105,6 +116,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_payload_release_pages(bip);
> +               }
> 

Please add braces in above else makes code easier to read :-

        else {
                if (bip->bip_flags & BIP_RELEASE_PAGES) {
                        bio_integrity_payload_release_pages(bip);
                }
         }

also if you can get away with using following, (totally untested) :-

        else if (bip->bip_flags & BIP_RELEASE_PAGES)
                        bio_integrity_payload_release_pages(bip);

>          __bio_integrity_free(bs, bip);
>          bio->bi_integrity = NULL;
> @@ -377,6 +392,113 @@ void bio_integrity_advance(struct bio *bio, unsigned int bytes_done)
>          bvec_iter_advance(bip->bip_vec, &bip->bip_iter, bytes);
>   }
> 
> +#define __MAX_ONSTACK_PI_PAGES (8)
> +/**
> + * bio_integrity_add_pi_iovec - Add PI io vector
> + * @bio:       bio whose integrity vector to update
> + * @pi_iov:    iovec added to @bio's integrity
> + *
> + * Description: Pins pages for *pi_iov and appends them to @bio's integrity.
> + */
> +int bio_integrity_add_pi_iovec(struct bio *bio, struct iovec *pi_iov)
> +{
> +       struct blk_integrity *bi = bdev_get_integrity(bio->bi_bdev);
> +       struct bio_integrity_payload *bip;
> +       struct page *pi_pages[__MAX_ONSTACK_PI_PAGES];
> +       struct page **pi_page = pi_pages;
> +       struct iov_iter pi_iter;
> +       int nr_vec_page = 0;
> +       int ret = 0, intervals = 0;
> +       bool is_write = op_is_write(bio_op(bio));
> +       ssize_t size, pg_num;
> +       size_t offset;
> +       size_t len;
> +

nit:- see if above hunk can be written as, nothing technical purely
cosmetic :

#define MAX_ONSTACK_PI_PAGES (8)
/**
  * bio_integrity_add_pi_iovec - Add PI io vector
  * @bio:       bio whose integrity vector to update
  * @pi_iov:    iovec added to @bio's integrity
  *
  * Description: Pins pages for *pi_iov and appends them to @bio's 
integrity.
  */ 

int bio_integrity_add_pi_iovec(struct bio *bio, struct iovec *pi_iov)
{
        struct blk_integrity *bi = bdev_get_integrity(bio->bi_bdev);
        struct page *pi_pages[MAX_ONSTACK_PI_PAGES];
        bool is_write = op_is_write(bio_op(bio));
        struct bio_integrity_payload *bip;
        struct page **pi_page = pi_pages;
        int ret = 0, intervals = 0;
        struct iov_iter pi_iter;
        ssize_t size, pg_num;
        int nr_vec_page = 0;
        size_t offset;
        size_t len;


> +       if (unlikely(!bi)) {
> +               pr_err("The disk is not integrity capable");
> +               return -EINVAL;
> +       }
> +
> +       nr_vec_page = (pi_iov->iov_len + PAGE_SIZE - 1) >> PAGE_SHIFT;
> +       nr_vec_page += 1; // we need this die to data of size N pages can be pinned to N+1 page
> +

Please use /**/ kernel documentation style as above comment in important.

> +       if (unlikely(nr_vec_page > __MAX_ONSTACK_PI_PAGES)) {
> +               pi_page = kcalloc(nr_vec_page, sizeof(struct pi_page *), GFP_NOIO);

Overly long line above, < 80 char per line is preferred.

> +               if (!pi_page)
> +                       return -ENOMEM;
> +       }
> +
> +       intervals = bio_integrity_intervals(bi, bio_sectors(bio));
> +       if (unlikely(intervals * bi->tuple_size < pi_iov->iov_len)) {
> +               pr_err("Interval number is wrong, intervals=%d, bi->tuple_size=%d, pi_iov->iov_len=%u",
> +                       (int)intervals, (int)bi->tuple_size,
> +                       (unsigned int)pi_iov->iov_len);

intervals variable is already int why type cast ?
I've not checked but see if you can remove other casts as well...

> +
> +               ret = -EINVAL;
> +               goto exit;
> +       }
> +
> +       bip = bio_integrity_alloc(bio, GFP_NOIO, nr_vec_page);
> +       if (IS_ERR(bip)) {
> +               ret = PTR_ERR(bip);
> +               goto exit;
> +       }
> +
> +       bip->bip_iter.bi_size = pi_iov->iov_len;
> +       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;
> +
> +       iov_iter_init(&pi_iter, is_write ?  WRITE : READ, pi_iov, 1, pi_iov->iov_len);

initialize the is_write with WRITE or READ at the time of declaration 
and remove the conditional operator in the function call that is 
discouraged... something like following at the start of the function on
a new line :-

bool direction = op_is_write(bio_op(bio)) ? WRITE : READ;

So above iov_iter_init() call becomes :-

iov_iter_init(&pi_iter, direction , pi_iov, 1, pi_iov->iov_len);

> +
> +       // pin user data to pages

above comment is obvious maybe we can get rid of that one ?

> +       size = iov_iter_get_pages(&pi_iter, pi_page, LONG_MAX, nr_vec_page, &offset);

< 80 char per line are preferred..

> +       if (unlikely(size < 0)) {
> +               pr_err("Failed to pin PI buffer to page");
> +               ret = -EFAULT;
> +               goto exit;
> +       }
> +
> +       // calc count of pined pages

/**/ style comment please...

> +       if (size > (PAGE_SIZE-offset)) {
> +               size = DIV_ROUND_UP(size - (PAGE_SIZE-offset), PAGE_SIZE)+1;
> +       } else

no need for braces in the above if .. also this need spaces after
binary operators ...

> +               size = 1;
> +
> +       // fill bio integrity biovecs the given pages
> +       len = pi_iov->iov_len;
> +       for (pg_num = 0; pg_num < size; ++pg_num) {
> +               size_t sz;
> +

I'd rename sz variable to page_len as it is coupled with pi_page..

> +               offset = (pg_num)?0:offset;
> +               sz = PAGE_SIZE-offset;

nit:- spaces after binary operators something like :-

                offset = pg_num ? 0 : offset;
                sz = PAGE_SIZE - offset;

> +               if (sz > len)
> +                       sz = len;
> +               ret = bio_integrity_add_page(bio, pi_page[pg_num], sz, offset);
> +               if (unlikely(ret != sz)) {
> +                       ret = -ENOMEM;
> +                       goto exit;
> +               }
> +               len -= sz;
> +               bip->bip_flags |= BIP_RELEASE_PAGES;
> +       }
> +
> +       ret = 0;
> +
> +exit:
> +
> +       if (ret && bip->bip_flags & BIP_RELEASE_PAGES)
> +               bio_integrity_payload_release_pages(bip);
> +
every jump to exit label from this function initializes the ret variable 
to non 0 value -EINVAL, PTR_ERR(bip) from bio_integrity_alloc(),
-EFAULT, and -ENOMEM in the same order, we can get rid of the ret in the 
above if condition ?

> +       if (pi_page != pi_pages)
> +               kfree(pi_page);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL(bio_integrity_add_pi_iovec);

consider EXPORT_SYMBOL_GPL() unless there is a specific reason..

> +
>   /**
>    * 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 00952e92eae1..57a4dd0b81ff 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -319,6 +319,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 */
>   };
> 
>   /*
> @@ -706,6 +707,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_pi_iovec(struct bio *bio, struct iovec *pi_iov);
>   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);
> @@ -746,6 +748,12 @@ static inline void bio_integrity_advance(struct bio *bio,
>          return;
>   }
> 
> +static inline int bio_integrity_add_pi_iovec(struct bio *bio,
> +                                       struct iovec *pi_iov)
> +{
> +       return 0;
> +}
> +
>   static inline void bio_integrity_trim(struct bio *bio)
>   {
>          return;
> --
> 2.33.0
>