[PATCH 0/3] implement direct IO with integrity

[PATCH 0/3] implement direct IO with integrity

[Alexander V. Buev]

This series of patches makes possible to do direct block IO
with integrity payload using io uring kernel interface.
Userspace app can utilize READV/WRITEV operation with a new 
(unused before) flag in sqe struct to mark IO request as 
"request with integrity payload". 
When this flag is set, the last of provided iovecs
must contain pointer and length of this integrity payload.

Alexander V. Buev (3):
  block: bio-integrity: add PI iovec to bio
  block: io_uring: add IO_WITH_PI flag to SQE
  block: fops: handle IOCB_USE_PI in direct IO

 block/bio-integrity.c         | 124 +++++++++++++++++++++++++++++++++-
 block/fops.c                  |  71 +++++++++++++++++++
 fs/io_uring.c                 |  32 ++++++++-
 include/linux/bio.h           |   8 +++
 include/linux/fs.h            |   1 +
 include/uapi/linux/io_uring.h |   3 +
 6 files changed, 235 insertions(+), 4 deletions(-)

-- 
2.33.0

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

[Alexander V. Buev]

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);
+		}
 
 	__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;
+
+	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
+
+	if (unlikely(nr_vec_page > __MAX_ONSTACK_PI_PAGES)) {
+		pi_page = kcalloc(nr_vec_page, sizeof(struct pi_page *), GFP_NOIO);
+		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);
+
+		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);
+
+	// pin user data to pages
+	size = iov_iter_get_pages(&pi_iter, pi_page, LONG_MAX, nr_vec_page, &offset);
+	if (unlikely(size < 0)) {
+		pr_err("Failed to pin PI buffer to page");
+		ret = -EFAULT;
+		goto exit;
+	}
+
+	// calc count of pined pages
+	if (size > (PAGE_SIZE-offset)) {
+		size = DIV_ROUND_UP(size - (PAGE_SIZE-offset), PAGE_SIZE)+1;
+	} else
+		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;
+
+		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);
+
+	if (pi_page != pi_pages)
+		kfree(pi_page);
+
+	return ret;
+}
+EXPORT_SYMBOL(bio_integrity_add_pi_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 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

[PATCH 2/3] block: io_uring: add IO_WITH_PI flag to SQE

[Alexander V. Buev]

Add new IOSQE_IO_WITH_PI flag to sqe struct flags.
Add IOCB_USE_PI flag to kiocb struct flags.
Correct range checking at uring layer in case of
READV/WRITEV operations with IOCB_USE_PI.

Based on: https://patchwork.kernel.org/patch/11405557/

Signed-off-by: Alexander V. Buev <a.buev@yadro.com>
---
 fs/io_uring.c                 | 32 +++++++++++++++++++++++++++++---
 include/linux/fs.h            |  1 +
 include/uapi/linux/io_uring.h |  3 +++
 3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index bc18af5e0a93..bce8488fb849 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -105,7 +105,7 @@
 
 #define SQE_VALID_FLAGS	(IOSQE_FIXED_FILE|IOSQE_IO_DRAIN|IOSQE_IO_LINK|	\
 				IOSQE_IO_HARDLINK | IOSQE_ASYNC | \
-				IOSQE_BUFFER_SELECT)
+				IOSQE_BUFFER_SELECT | IOSQE_IO_WITH_PI)
 #define IO_REQ_CLEAN_FLAGS (REQ_F_BUFFER_SELECTED | REQ_F_NEED_CLEANUP | \
 				REQ_F_POLLED | REQ_F_INFLIGHT | REQ_F_CREDS)
 
@@ -726,6 +726,7 @@ enum {
 	REQ_F_HARDLINK_BIT	= IOSQE_IO_HARDLINK_BIT,
 	REQ_F_FORCE_ASYNC_BIT	= IOSQE_ASYNC_BIT,
 	REQ_F_BUFFER_SELECT_BIT	= IOSQE_BUFFER_SELECT_BIT,
+	REQ_F_IO_WITH_PI_BIT    = IOSQE_IO_WITH_PI_BIT,
 
 	/* first byte is taken by user flags, shift it to not overlap */
 	REQ_F_FAIL_BIT		= 8,
@@ -2850,6 +2851,13 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	if (unlikely(ret))
 		return ret;
 
+	if (sqe->flags & IOSQE_IO_WITH_PI) {
+		if (!(req->file->f_flags & O_DIRECT))
+			return -EINVAL;
+
+		kiocb->ki_flags |= IOCB_USE_PI;
+	}
+
 	/*
 	 * If the file is marked O_NONBLOCK, still allow retry for it if it
 	 * supports async. Otherwise it's impossible to use O_NONBLOCK files
@@ -3451,6 +3459,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
 	struct iov_iter_state __state, *state;
 	ssize_t ret, ret2;
+	size_t iov_data_size;
 
 	if (rw) {
 		iter = &rw->iter;
@@ -3483,7 +3492,15 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 		return ret ?: -EAGAIN;
 	}
 
-	ret = rw_verify_area(READ, req->file, io_kiocb_ppos(kiocb), req->result);
+	iov_data_size = req->result;
+	/* in case of PI present we must verify ranges only by data size */
+	if (req->opcode == IORING_OP_READV &&
+	     kiocb->ki_flags & IOCB_USE_PI &&
+	     iter->nr_segs >= 2) {
+		iov_data_size -= iter->iov[iter->nr_segs-1].iov_len;
+	}
+
+	ret = rw_verify_area(READ, req->file, io_kiocb_ppos(kiocb), iov_data_size);
 	if (unlikely(ret)) {
 		kfree(iovec);
 		return ret;
@@ -3586,6 +3603,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
 	struct iov_iter_state __state, *state;
 	ssize_t ret, ret2;
+	size_t iov_data_size;
 
 	if (rw) {
 		iter = &rw->iter;
@@ -3616,7 +3634,15 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 	    (req->flags & REQ_F_ISREG))
 		goto copy_iov;
 
-	ret = rw_verify_area(WRITE, req->file, io_kiocb_ppos(kiocb), req->result);
+	iov_data_size = req->result;
+	/* in case of PI present we must verify ranges only by data size */
+	if (req->opcode == IORING_OP_WRITEV &&
+	     kiocb->ki_flags & IOCB_USE_PI &&
+	     iter->nr_segs >= 2) {
+		iov_data_size -= iter->iov[iter->nr_segs-1].iov_len;
+	}
+
+	ret = rw_verify_area(WRITE, req->file, io_kiocb_ppos(kiocb), iov_data_size);
 	if (unlikely(ret))
 		goto out_free;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e7a633353fd2..874afc7bc28b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -321,6 +321,7 @@ enum rw_hint {
 #define IOCB_NOIO		(1 << 20)
 /* can use bio alloc cache */
 #define IOCB_ALLOC_CACHE	(1 << 21)
+#define IOCB_USE_PI		(1 << 22)
 
 struct kiocb {
 	struct file		*ki_filp;
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index b270a07b285e..6b2dd4449ea5 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -70,6 +70,7 @@ enum {
 	IOSQE_IO_HARDLINK_BIT,
 	IOSQE_ASYNC_BIT,
 	IOSQE_BUFFER_SELECT_BIT,
+	IOSQE_IO_WITH_PI_BIT,
 };
 
 /*
@@ -87,6 +88,8 @@ enum {
 #define IOSQE_ASYNC		(1U << IOSQE_ASYNC_BIT)
 /* select buffer from sqe->buf_group */
 #define IOSQE_BUFFER_SELECT	(1U << IOSQE_BUFFER_SELECT_BIT)
+/* perform IO with PI (protection information) */
+#define IOSQE_IO_WITH_PI	(1U << IOSQE_IO_WITH_PI_BIT)
 
 /*
  * io_uring_setup() flags
-- 
2.33.0

[PATCH 3/3] block: fops: handle IOCB_USE_PI in direct IO

[Alexander V. Buev]

Check that the size of integrity data correspond to device integrity
profile and data size. Split integrity data to the different bio's
in case of to big orginal bio (together with normal data).
Correct offset/size checking at blkdev layer in read/write_iter
functions.

Signed-off-by: Alexander V. Buev <a.buev@yadro.com>
---
 block/fops.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/block/fops.c b/block/fops.c
index 1e970c247e0e..74040314f5f3 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -197,12 +197,39 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
 	struct blk_plug plug;
 	struct blkdev_dio *dio;
 	struct bio *bio;
+	struct iovec *pi_iov, _pi_iov;
 	bool is_poll = (iocb->ki_flags & IOCB_HIPRI) != 0;
 	bool is_read = (iov_iter_rw(iter) == READ), is_sync;
 	loff_t pos = iocb->ki_pos;
 	blk_qc_t qc = BLK_QC_T_NONE;
 	int ret = 0;
 
+	if (iocb->ki_flags & IOCB_USE_PI) {
+		struct blk_integrity *bi = blk_get_integrity(bdev->bd_disk);
+		unsigned int intervals;
+
+		/* Last iovec contains protection information. */
+		if (!iter->nr_segs)
+			return -EINVAL;
+
+		iter->nr_segs--;
+		pi_iov = (struct iovec *)(iter->iov + iter->nr_segs);
+
+		/* TODO: seems iter is in charge of this check ? */
+		if (pi_iov->iov_len > iter->count)
+			return -EINVAL;
+
+		iter->count -= pi_iov->iov_len;
+
+		intervals = bio_integrity_intervals(bi, iter->count >> 9);
+		if (unlikely(intervals * bi->tuple_size > pi_iov->iov_len)) {
+			pr_err("Integrity & data size mismatch data=%lu integrity=%u intervals=%u tupple=%u",
+				iter->count, (unsigned int)pi_iov->iov_len,
+				intervals, bi->tuple_size);
+				return -EINVAL;
+		}
+	}
+
 	if ((pos | iov_iter_alignment(iter)) &
 	    (bdev_logical_block_size(bdev) - 1))
 		return -EINVAL;
@@ -258,6 +285,17 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
 		dio->size += bio->bi_iter.bi_size;
 		pos += bio->bi_iter.bi_size;
 
+		/* in case we can't add all data to one bio */
+		/* we must split integrity too */
+
+		if (iocb->ki_flags & IOCB_USE_PI) {
+			struct blk_integrity *bi = bdev_get_integrity(bio->bi_bdev);
+
+			_pi_iov.iov_base =  pi_iov->iov_base;
+			_pi_iov.iov_base += bio_integrity_bytes(bi, (dio->size-bio->bi_iter.bi_size) >> 9);
+			_pi_iov.iov_len  = bio_integrity_bytes(bi, bio->bi_iter.bi_size >> 9);
+		}
+
 		nr_pages = bio_iov_vecs_to_alloc(iter, BIO_MAX_VECS);
 		if (!nr_pages) {
 			bool polled = false;
@@ -267,6 +305,16 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
 				polled = true;
 			}
 
+			/* Add protection information to bio */
+			if (iocb->ki_flags & IOCB_USE_PI) {
+				ret = bio_integrity_add_pi_iovec(bio, &_pi_iov);
+				if (ret) {
+					bio->bi_status = BLK_STS_IOERR;
+					bio_endio(bio);
+					break;
+				}
+			}
+
 			qc = submit_bio(bio);
 
 			if (polled)
@@ -288,6 +336,16 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
 			atomic_inc(&dio->ref);
 		}
 
+
+		if (iocb->ki_flags & IOCB_USE_PI) {
+			ret = bio_integrity_add_pi_iovec(bio, &_pi_iov);
+			if (ret) {
+				bio->bi_status = BLK_STS_IOERR;
+				bio_endio(bio);
+				break;
+			}
+		}
+
 		submit_bio(bio);
 		bio = bio_alloc(GFP_KERNEL, nr_pages);
 	}
@@ -509,6 +567,12 @@ static ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if ((iocb->ki_flags & (IOCB_NOWAIT | IOCB_DIRECT)) == IOCB_NOWAIT)
 		return -EOPNOTSUPP;
 
+	if (iocb->ki_flags & IOCB_USE_PI) {
+		if (from->nr_segs < 2)
+			return -EINVAL;
+		size += from->iov[from->nr_segs-1].iov_len;
+	}
+
 	size -= iocb->ki_pos;
 	if (iov_iter_count(from) > size) {
 		shorted = iov_iter_count(from) - size;
@@ -537,6 +601,13 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 		return 0;
 
 	size -= pos;
+
+	if (iocb->ki_flags & IOCB_USE_PI) {
+		if (to->nr_segs < 2)
+			return -EINVAL;
+		size += to->iov[to->nr_segs-1].iov_len;
+	}
+
 	if (iov_iter_count(to) > size) {
 		shorted = iov_iter_count(to) - size;
 		iov_iter_truncate(to, size);
-- 
2.33.0

Re: [PATCH 0/3] implement direct IO with integrity

[Jens Axboe]

On 10/28/21 5:24 AM, Alexander V. Buev wrote:
> This series of patches makes possible to do direct block IO
> with integrity payload using io uring kernel interface.
> Userspace app can utilize READV/WRITEV operation with a new 
> (unused before) flag in sqe struct to mark IO request as 
> "request with integrity payload". 
> When this flag is set, the last of provided iovecs
> must contain pointer and length of this integrity payload.
> 
> Alexander V. Buev (3):
>   block: bio-integrity: add PI iovec to bio
>   block: io_uring: add IO_WITH_PI flag to SQE
>   block: fops: handle IOCB_USE_PI in direct IO
> 
>  block/bio-integrity.c         | 124 +++++++++++++++++++++++++++++++++-
>  block/fops.c                  |  71 +++++++++++++++++++
>  fs/io_uring.c                 |  32 ++++++++-
>  include/linux/bio.h           |   8 +++
>  include/linux/fs.h            |   1 +
>  include/uapi/linux/io_uring.h |   3 +
>  6 files changed, 235 insertions(+), 4 deletions(-)

A couple of suggestions on this:

1) Don't think we need an IOSQE flag, those are mostly reserved for
   modifiers that apply to (mostly) all kinds of requests
2) I think this would be cleaner as a separate command, rather than
   need odd adjustments and iov assumptions. That also gets it out
   of the fast path.

I'd add IORING_OP_READV_PI and IORING_OP_WRITEV_PI for this, I think
you'd end up with a much cleaner implementation that way.

-- 
Jens Axboe

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

[Christoph Hellwig]

On Thu, Oct 28, 2021 at 02:24:04PM +0300, Alexander V. Buev wrote:
>   * Written by: Martin K. Petersen <martin.petersen@oracle.com>
>   */
> -
>  #include <linux/blkdev.h>

Spurious whitespace change.

> +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);
> +	}

The bv declaration can move into the loop (or we can just nuke the
single use local variable entirely).

> +	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

Pleae avoid overly long line and //-style comments.

> +	size = iov_iter_get_pages(&pi_iter, pi_page, LONG_MAX, nr_vec_page, &offset);
> +	if (unlikely(size < 0)) {
> +		pr_err("Failed to pin PI buffer to page");
> +		ret = -EFAULT;
> +		goto exit;
> +	}

Instead of the local page this should use the same scheme as
__bio_iov_iter_get_pages.

> +
> +	// calc count of pined pages
> +	if (size > (PAGE_SIZE-offset)) {
> +		size = DIV_ROUND_UP(size - (PAGE_SIZE-offset), PAGE_SIZE)+1;
> +	} else

No need for braces around single line statements, please always
put whitespaces around your operators.

> +EXPORT_SYMBOL(bio_integrity_add_pi_iovec);

EXPORT_SYMBOL_GPL, please.

Re: [PATCH 3/3] block: fops: handle IOCB_USE_PI in direct IO

[Christoph Hellwig]

> +		struct blk_integrity *bi = blk_get_integrity(bdev->bd_disk);
> +		unsigned int intervals;
> +
> +		/* Last iovec contains protection information. */
> +		if (!iter->nr_segs)
> +			return -EINVAL;
> +
> +		iter->nr_segs--;
> +		pi_iov = (struct iovec *)(iter->iov + iter->nr_segs);

Please don't poke into the iov_iter abstractions.  This will break
with PI in anything but ITER_IOVEC buffers.

Re: [PATCH 0/3] implement direct IO with integrity

[Christoph Hellwig]

On Thu, Oct 28, 2021 at 09:13:07AM -0600, Jens Axboe wrote:
> A couple of suggestions on this:
> 
> 1) Don't think we need an IOSQE flag, those are mostly reserved for
>    modifiers that apply to (mostly) all kinds of requests
> 2) I think this would be cleaner as a separate command, rather than
>    need odd adjustments and iov assumptions. That also gets it out
>    of the fast path.
> 
> I'd add IORING_OP_READV_PI and IORING_OP_WRITEV_PI for this, I think
> you'd end up with a much cleaner implementation that way.

Agreed.  I also wonder if we could do saner paramter passing.
E.g. pass a separate pointer to the PI data if we find space for
that somewhere in the SQE.

Re: [PATCH 0/3] implement direct IO with integrity

[Jens Axboe]

On 10/28/21 9:18 AM, Christoph Hellwig wrote:
> On Thu, Oct 28, 2021 at 09:13:07AM -0600, Jens Axboe wrote:
>> A couple of suggestions on this:
>>
>> 1) Don't think we need an IOSQE flag, those are mostly reserved for
>>    modifiers that apply to (mostly) all kinds of requests
>> 2) I think this would be cleaner as a separate command, rather than
>>    need odd adjustments and iov assumptions. That also gets it out
>>    of the fast path.
>>
>> I'd add IORING_OP_READV_PI and IORING_OP_WRITEV_PI for this, I think
>> you'd end up with a much cleaner implementation that way.
> 
> Agreed.  I also wonder if we could do saner paramter passing.
> E.g. pass a separate pointer to the PI data if we find space for
> that somewhere in the SQE.

Yeah, the whole "put PI in the last iovec" makes the code really ugly
dealing with it. Would be a lot cleaner to separate the two. IMHO this
is largely a work-around that you'd apply to syscall interfaces that
only take the iovec, but we don't need to work around it here if we can
define a clean command upfront.

And if we don't need vectored requests for the data part, then even
better. That one might not be feasible, but figured I'd toss it out
there.

-- 
Jens Axboe

Re: [PATCH 0/3] implement direct IO with integrity

[Jens Axboe]

On 10/28/21 9:13 AM, Jens Axboe wrote:
> On 10/28/21 5:24 AM, Alexander V. Buev wrote:
>> This series of patches makes possible to do direct block IO
>> with integrity payload using io uring kernel interface.
>> Userspace app can utilize READV/WRITEV operation with a new 
>> (unused before) flag in sqe struct to mark IO request as 
>> "request with integrity payload". 
>> When this flag is set, the last of provided iovecs
>> must contain pointer and length of this integrity payload.
>>
>> Alexander V. Buev (3):
>>   block: bio-integrity: add PI iovec to bio
>>   block: io_uring: add IO_WITH_PI flag to SQE
>>   block: fops: handle IOCB_USE_PI in direct IO
>>
>>  block/bio-integrity.c         | 124 +++++++++++++++++++++++++++++++++-
>>  block/fops.c                  |  71 +++++++++++++++++++
>>  fs/io_uring.c                 |  32 ++++++++-
>>  include/linux/bio.h           |   8 +++
>>  include/linux/fs.h            |   1 +
>>  include/uapi/linux/io_uring.h |   3 +
>>  6 files changed, 235 insertions(+), 4 deletions(-)
> 
> A couple of suggestions on this:
> 
> 1) Don't think we need an IOSQE flag, those are mostly reserved for
>    modifiers that apply to (mostly) all kinds of requests
> 2) I think this would be cleaner as a separate command, rather than
>    need odd adjustments and iov assumptions. That also gets it out
>    of the fast path.
> 
> I'd add IORING_OP_READV_PI and IORING_OP_WRITEV_PI for this, I think
> you'd end up with a much cleaner implementation that way.

Oh, and please do CC io-uring on changes that touch it.

-- 
Jens Axboe

Re: [PATCH 0/3] implement direct IO with integrity

[Mikhail Malygin]

Thanks for the feedback, we’ll submit and updated version of the series.

The only question is regarding uapi: should we add a separate opcodes for read/write or use existing opcodes with the flag in the io_uring_sqe.rw_flags field?

The flag was discussed in the another submission, where it was considered to be a better approach over opcodes: https://patchwork.kernel.org/project/linux-block/patch/20200226083719.4389-2-bob.liu@oracle.com/

Thanks,
Mikhail

> On 28 Oct 2021, at 18:20, Jens Axboe <axboe@kernel.dk> wrote:
> 
> On 10/28/21 9:18 AM, Christoph Hellwig wrote:
>> On Thu, Oct 28, 2021 at 09:13:07AM -0600, Jens Axboe wrote:
>>> A couple of suggestions on this:
>>> 
>>> 1) Don't think we need an IOSQE flag, those are mostly reserved for
>>>   modifiers that apply to (mostly) all kinds of requests
>>> 2) I think this would be cleaner as a separate command, rather than
>>>   need odd adjustments and iov assumptions. That also gets it out
>>>   of the fast path.
>>> 
>>> I'd add IORING_OP_READV_PI and IORING_OP_WRITEV_PI for this, I think
>>> you'd end up with a much cleaner implementation that way.
>> 
>> Agreed.  I also wonder if we could do saner paramter passing.
>> E.g. pass a separate pointer to the PI data if we find space for
>> that somewhere in the SQE.
> 
> Yeah, the whole "put PI in the last iovec" makes the code really ugly
> dealing with it. Would be a lot cleaner to separate the two. IMHO this
> is largely a work-around that you'd apply to syscall interfaces that
> only take the iovec, but we don't need to work around it here if we can
> define a clean command upfront.
> 
> And if we don't need vectored requests for the data part, then even
> better. That one might not be feasible, but figured I'd toss it out
> there.
> 
> -- 
> Jens Axboe
> 

Re: [PATCH 0/3] implement direct IO with integrity

[Jens Axboe]

On 10/28/21 9:44 AM, Mikhail Malygin wrote:
> Thanks for the feedback, we’ll submit and updated version of the series.
> 
> The only question is regarding uapi: should we add a separate opcodes
> for read/write or use existing opcodes with the flag in the
> io_uring_sqe.rw_flags field?
> 
> The flag was discussed in the another submission, where it was
> considered to be a better approach over opcodes:
> https://patchwork.kernel.org/project/linux-block/patch/20200226083719.4389-2-bob.liu@oracle.com/

Separate opcodes is, at least to me, definitely the way to go. Just
looking at the code needing to tap into weird spots for PI is enough to
make that call. On top of that, it also allows you to cleanly define
this command and (hopefully?) avoid that weirdness with implied PI in
the last iovec.

-- 
Jens Axboe

Re: [PATCH 0/3] implement direct IO with integrity

[Pavel Begunkov]

On 10/28/21 16:50, Jens Axboe wrote:
> On 10/28/21 9:44 AM, Mikhail Malygin wrote:
>> Thanks for the feedback, we’ll submit and updated version of the series.
>>
>> The only question is regarding uapi: should we add a separate opcodes
>> for read/write or use existing opcodes with the flag in the
>> io_uring_sqe.rw_flags field?
>>
>> The flag was discussed in the another submission, where it was
>> considered to be a better approach over opcodes:
>> https://patchwork.kernel.org/project/linux-block/patch/20200226083719.4389-2-bob.liu@oracle.com/
> 
> Separate opcodes is, at least to me, definitely the way to go. Just
> looking at the code needing to tap into weird spots for PI is enough to
> make that call. On top of that, it also allows you to cleanly define
> this command and (hopefully?) avoid that weirdness with implied PI in
> the last iovec.

Reminds me struggles with writing encoded data to btrfs. I believe
Omar did go for RWF_ENCODED flag, right?

-- 
Pavel Begunkov

Re: [PATCH 0/3] implement direct IO with integrity

[Jens Axboe]

On 10/28/21 9:56 AM, Pavel Begunkov wrote:
> On 10/28/21 16:50, Jens Axboe wrote:
>> On 10/28/21 9:44 AM, Mikhail Malygin wrote:
>>> Thanks for the feedback, we’ll submit and updated version of the series.
>>>
>>> The only question is regarding uapi: should we add a separate opcodes
>>> for read/write or use existing opcodes with the flag in the
>>> io_uring_sqe.rw_flags field?
>>>
>>> The flag was discussed in the another submission, where it was
>>> considered to be a better approach over opcodes:
>>> https://patchwork.kernel.org/project/linux-block/patch/20200226083719.4389-2-bob.liu@oracle.com/
>>
>> Separate opcodes is, at least to me, definitely the way to go. Just
>> looking at the code needing to tap into weird spots for PI is enough to
>> make that call. On top of that, it also allows you to cleanly define
>> this command and (hopefully?) avoid that weirdness with implied PI in
>> the last iovec.
> 
> Reminds me struggles with writing encoded data to btrfs. I believe
> Omar did go for RWF_ENCODED flag, right?

Exactly the same problem, yes. It ends up being pretty miserable, and
there's no reason to go through that misery when we're not bound by only
passing in an iovec.

-- 
Jens Axboe

Re: [PATCH 0/3] implement direct IO with integrity

[Pavel Begunkov]

On 10/28/21 17:22, Jens Axboe wrote:
> On 10/28/21 9:56 AM, Pavel Begunkov wrote:
>> On 10/28/21 16:50, Jens Axboe wrote:
>>> On 10/28/21 9:44 AM, Mikhail Malygin wrote:
>>>> Thanks for the feedback, we’ll submit and updated version of the series.
>>>>
>>>> The only question is regarding uapi: should we add a separate opcodes
>>>> for read/write or use existing opcodes with the flag in the
>>>> io_uring_sqe.rw_flags field?
>>>>
>>>> The flag was discussed in the another submission, where it was
>>>> considered to be a better approach over opcodes:
>>>> https://patchwork.kernel.org/project/linux-block/patch/20200226083719.4389-2-bob.liu@oracle.com/
>>>
>>> Separate opcodes is, at least to me, definitely the way to go. Just
>>> looking at the code needing to tap into weird spots for PI is enough to
>>> make that call. On top of that, it also allows you to cleanly define
>>> this command and (hopefully?) avoid that weirdness with implied PI in
>>> the last iovec.
>>
>> Reminds me struggles with writing encoded data to btrfs. I believe
>> Omar did go for RWF_ENCODED flag, right?
> 
> Exactly the same problem, yes. It ends up being pretty miserable, and
> there's no reason to go through that misery when we're not bound by only
> passing in an iovec.

I agree that a new opcode is better, at least we can keep the overhead
out of the common path, but RWF_ENCODED was having similar problems
(e.g. passing metadata in iov), so that's interesting why RWF was chosen
in the end. Is it only to support readv/writev(2) or something else?

-- 
Pavel Begunkov

Re: [PATCH 0/3] implement direct IO with integrity

[Jens Axboe]

On 10/28/21 11:11 AM, Pavel Begunkov wrote:
> On 10/28/21 17:22, Jens Axboe wrote:
>> On 10/28/21 9:56 AM, Pavel Begunkov wrote:
>>> On 10/28/21 16:50, Jens Axboe wrote:
>>>> On 10/28/21 9:44 AM, Mikhail Malygin wrote:
>>>>> Thanks for the feedback, we’ll submit and updated version of the series.
>>>>>
>>>>> The only question is regarding uapi: should we add a separate opcodes
>>>>> for read/write or use existing opcodes with the flag in the
>>>>> io_uring_sqe.rw_flags field?
>>>>>
>>>>> The flag was discussed in the another submission, where it was
>>>>> considered to be a better approach over opcodes:
>>>>> https://patchwork.kernel.org/project/linux-block/patch/20200226083719.4389-2-bob.liu@oracle.com/
>>>>
>>>> Separate opcodes is, at least to me, definitely the way to go. Just
>>>> looking at the code needing to tap into weird spots for PI is enough to
>>>> make that call. On top of that, it also allows you to cleanly define
>>>> this command and (hopefully?) avoid that weirdness with implied PI in
>>>> the last iovec.
>>>
>>> Reminds me struggles with writing encoded data to btrfs. I believe
>>> Omar did go for RWF_ENCODED flag, right?
>>
>> Exactly the same problem, yes. It ends up being pretty miserable, and
>> there's no reason to go through that misery when we're not bound by only
>> passing in an iovec.
> 
> I agree that a new opcode is better, at least we can keep the overhead
> out of the common path, but RWF_ENCODED was having similar problems
> (e.g. passing metadata in iov), so that's interesting why RWF was chosen
> in the end. Is it only to support readv/writev(2) or something else?

To shoe horn it in via an existing interface, I don't think anybody
wanted to add yet another read/write API just to cater to that.

-- 
Jens Axboe

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

[Chaitanya Kulkarni]

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
> 

Re: [PATCH 0/3] implement direct IO with integrity

[Martin K. Petersen]

Jens,

> Yeah, the whole "put PI in the last iovec" makes the code really ugly
> dealing with it. Would be a lot cleaner to separate the two. IMHO this
> is largely a work-around that you'd apply to syscall interfaces that
> only take the iovec, but we don't need to work around it here if we
> can define a clean command upfront.

Yup, I agree.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Martin K. Petersen]

Alexander,

Thanks for working on this!

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

In my parallel attempt the flag was called BIP_USER_MAPPED to mirror
BIO_USER_MAPPED. But with the latter now gone it doesn't really
matter. BIP_RELEASE_PAGES is fine.

I find bio_integrity_payload_release_pages() a bit long. All the other
functions just use a bio_integrity_ prefix and take a bio. But no
biggie.

> +int bio_integrity_add_pi_iovec(struct bio *bio, struct iovec *pi_iov)

bio_integrity_add_iovec(), please. _pi is redundant.


> +	bip_set_seed(bip, bio->bi_iter.bi_sector);
> +
> +	if (bi->flags & BLK_INTEGRITY_IP_CHECKSUM)
> +		bip->bip_flags |= BIP_IP_CHECKSUM;

The last couple of months I have been working on a version of our DIX/PI
qualification tooling that does not depend on the DB I/O stack.

For the test tooling to work I need to be able to pass the seed and the
BIP_* flags as part of the command. The tooling needs to be able to
select the type of checksum and to be able to disable checking for
initiator and target on a per-I/O basis. So these would need to be
passed in.

Note that extended PI formats have been defined in NVMe. These allow for
larger CRCs and reference tags to be specified in addition to a storage
tag. So we'll need to be careful when defining the SQE fields here.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2577 bytes --]

Hi "Alexander,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.15-rc7]
[cannot apply to axboe-block/for-next hch-configfs/for-next next-20211028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Alexander-V-Buev/implement-direct-IO-with-integrity/20211028-193652
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1fc596a56b334f4d593a2b49e5ff55af6aaa0816
config: hexagon-randconfig-r045-20211028 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/582fcf396dced9b11bbf489cd5f64e213de2e4fd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Alexander-V-Buev/implement-direct-IO-with-integrity/20211028-193652
        git checkout 582fcf396dced9b11bbf489cd5f64e213de2e4fd
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> block/bio-integrity.c:94:6: warning: no previous prototype for function 'bio_integrity_payload_release_pages' [-Wmissing-prototypes]
   void bio_integrity_payload_release_pages(struct bio_integrity_payload *bip)
        ^
   block/bio-integrity.c:94:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void bio_integrity_payload_release_pages(struct bio_integrity_payload *bip)
   ^
   static 
   1 warning generated.


vim +/bio_integrity_payload_release_pages +94 block/bio-integrity.c

    93	
  > 94	void bio_integrity_payload_release_pages(struct bio_integrity_payload *bip)
    95	{
    96		unsigned short i;
    97		struct bio_vec *bv;
    98	
    99		for (i = 0; i < bip->bip_vcnt; ++i) {
   100			bv = bip->bip_vec + i;
   101			put_page(bv->bv_page);
   102		}
   103	}
   104	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37136 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3672 bytes --]

Hi "Alexander,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.15-rc7]
[cannot apply to axboe-block/for-next hch-configfs/for-next next-20211028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Alexander-V-Buev/implement-direct-IO-with-integrity/20211028-193652
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1fc596a56b334f4d593a2b49e5ff55af6aaa0816
config: h8300-randconfig-r016-20211028 (attached as .config)
compiler: h8300-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/582fcf396dced9b11bbf489cd5f64e213de2e4fd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Alexander-V-Buev/implement-direct-IO-with-integrity/20211028-193652
        git checkout 582fcf396dced9b11bbf489cd5f64e213de2e4fd
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=h8300 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/asm-generic/bug.h:5,
                    from arch/h8300/include/asm/bug.h:8,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:13,
                    from include/asm-generic/current.h:5,
                    from ./arch/h8300/include/generated/asm/current.h:1,
                    from include/linux/sched.h:12,
                    from include/linux/blkdev.h:5,
                    from block/bio-integrity.c:8:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   include/asm-generic/page.h:89:51: warning: ordered comparison of pointer with null pointer [-Wextra]
      89 | #define virt_addr_valid(kaddr)  (((void *)(kaddr) >= (void *)PAGE_OFFSET) && \
         |                                                   ^~
   include/linux/compiler.h:78:45: note: in definition of macro 'unlikely'
      78 | # define unlikely(x)    __builtin_expect(!!(x), 0)
         |                                             ^
   include/linux/scatterlist.h:143:9: note: in expansion of macro 'BUG_ON'
     143 |         BUG_ON(!virt_addr_valid(buf));
         |         ^~~~~~
   include/linux/scatterlist.h:143:17: note: in expansion of macro 'virt_addr_valid'
     143 |         BUG_ON(!virt_addr_valid(buf));
         |                 ^~~~~~~~~~~~~~~
   block/bio-integrity.c: At top level:
>> block/bio-integrity.c:94:6: warning: no previous prototype for 'bio_integrity_payload_release_pages' [-Wmissing-prototypes]
      94 | void bio_integrity_payload_release_pages(struct bio_integrity_payload *bip)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/bio_integrity_payload_release_pages +94 block/bio-integrity.c

    93	
  > 94	void bio_integrity_payload_release_pages(struct bio_integrity_payload *bip)
    95	{
    96		unsigned short i;
    97		struct bio_vec *bv;
    98	
    99		for (i = 0; i < bip->bip_vcnt; ++i) {
   100			bv = bip->bip_vec + i;
   101			put_page(bv->bv_page);
   102		}
   103	}
   104	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35098 bytes --]

Re: [PATCH 3/3] block: fops: handle IOCB_USE_PI in direct IO

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3546 bytes --]

Hi "Alexander,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.15-rc7]
[cannot apply to axboe-block/for-next hch-configfs/for-next next-20211028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Alexander-V-Buev/implement-direct-IO-with-integrity/20211028-193652
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1fc596a56b334f4d593a2b49e5ff55af6aaa0816
config: mips-randconfig-r014-20211028 (attached as .config)
compiler: mips-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/48606c737ef4eeab1fd098ed57d9966dd73b97c3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Alexander-V-Buev/implement-direct-IO-with-integrity/20211028-193652
        git checkout 48606c737ef4eeab1fd098ed57d9966dd73b97c3
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/asm-generic/bug.h:22,
                    from arch/mips/include/asm/bug.h:42,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/mm.h:9,
                    from block/fops.c:8:
   block/fops.c: In function '__blkdev_direct_IO':
>> include/linux/kern_levels.h:5:25: warning: format '%lu' expects argument of type 'long unsigned int', but argument 2 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
       5 | #define KERN_SOH        "\001"          /* ASCII Start Of Header */
         |                         ^~~~~~
   include/linux/printk.h:418:25: note: in definition of macro 'printk_index_wrap'
     418 |                 _p_func(_fmt, ##__VA_ARGS__);                           \
         |                         ^~~~
   include/linux/printk.h:489:9: note: in expansion of macro 'printk'
     489 |         printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~
   include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
      11 | #define KERN_ERR        KERN_SOH "3"    /* error conditions */
         |                         ^~~~~~~~
   include/linux/printk.h:489:16: note: in expansion of macro 'KERN_ERR'
     489 |         printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         |                ^~~~~~~~
   block/fops.c:226:25: note: in expansion of macro 'pr_err'
     226 |                         pr_err("Integrity & data size mismatch data=%lu integrity=%u intervals=%u tupple=%u",
         |                         ^~~~~~


vim +5 include/linux/kern_levels.h

314ba3520e513a Joe Perches 2012-07-30  4  
04d2c8c83d0e3a Joe Perches 2012-07-30 @5  #define KERN_SOH	"\001"		/* ASCII Start Of Header */
04d2c8c83d0e3a Joe Perches 2012-07-30  6  #define KERN_SOH_ASCII	'\001'
04d2c8c83d0e3a Joe Perches 2012-07-30  7  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26502 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2345 bytes --]

Hi "Alexander,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.15-rc7]
[cannot apply to axboe-block/for-next hch-configfs/for-next next-20211028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Alexander-V-Buev/implement-direct-IO-with-integrity/20211028-193652
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1fc596a56b334f4d593a2b49e5ff55af6aaa0816
config: arm-buildonly-randconfig-r006-20211028 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/582fcf396dced9b11bbf489cd5f64e213de2e4fd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Alexander-V-Buev/implement-direct-IO-with-integrity/20211028-193652
        git checkout 582fcf396dced9b11bbf489cd5f64e213de2e4fd
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> block/bio-integrity.c:94:6: error: no previous prototype for 'bio_integrity_payload_release_pages' [-Werror=missing-prototypes]
      94 | void bio_integrity_payload_release_pages(struct bio_integrity_payload *bip)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors


vim +/bio_integrity_payload_release_pages +94 block/bio-integrity.c

    93	
  > 94	void bio_integrity_payload_release_pages(struct bio_integrity_payload *bip)
    95	{
    96		unsigned short i;
    97		struct bio_vec *bv;
    98	
    99		for (i = 0; i < bip->bip_vcnt; ++i) {
   100			bv = bip->bip_vec + i;
   101			put_page(bv->bv_page);
   102		}
   103	}
   104	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35086 bytes --]

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

[Alexander V. Buev]

> 
> Alexander,
> 
> Thanks for working on this!
> 
> > +		if (bip->bip_flags & BIP_RELEASE_PAGES) {
> > +			bio_integrity_payload_release_pages(bip);
> > +		}
> 
> In my parallel attempt the flag was called BIP_USER_MAPPED to mirror
> BIO_USER_MAPPED. But with the latter now gone it doesn't really
> matter. BIP_RELEASE_PAGES is fine.
ok 

> I find bio_integrity_payload_release_pages() a bit long. All the other
> functions just use a bio_integrity_ prefix and take a bio. But no
> biggie.
> 
> > +int bio_integrity_add_pi_iovec(struct bio *bio, struct iovec *pi_iov)
> 
> bio_integrity_add_iovec(), please. _pi is redundant.

I definitely agree with this (As with other similar notes)
It's just my habit of naming functions by struct name.

> 
> > +	bip_set_seed(bip, bio->bi_iter.bi_sector);
> > +
> > +	if (bi->flags & BLK_INTEGRITY_IP_CHECKSUM)
> > +		bip->bip_flags |= BIP_IP_CHECKSUM;
> 
> The last couple of months I have been working on a version of our DIX/PI
> qualification tooling that does not depend on the DB I/O stack.
> 
> For the test tooling to work I need to be able to pass the seed and the
> BIP_* flags as part of the command. The tooling needs to be able to
> select the type of checksum and to be able to disable checking for
> initiator and target on a per-I/O basis. So these would need to be
> passed in.
> 
> Note that extended PI formats have been defined in NVMe. These allow for
> larger CRCs and reference tags to be specified in addition to a storage
> tag. So we'll need to be careful when defining the SQE fields here.
 
After discussion, I plan to start by adding a pointer to the PI data 
in SQE struct.
This struct now has some padding that can be used for this.
Size of PI data can be determined according to integrity profile of
device and size of normal data.

Also, as recommended, the new version will use opcodes instead of the flag.

Thanks everyone for the detailed feedback. It's important for me.
I will try to correct all the issue's in the next version.

But some later )


-- 
Alexander Buev