| Message ID | 20240823103811.2421-1-anuj20.g@samsung.com |
|---|---|
| Headers | show |
| Series | Read/Write with meta/integrity | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Fri, Aug 23, 2024 at 04:08:03PM +0530, Anuj Gupta wrote: > Copy back the bounce buffer to user-space in entirety when the parent > bio completes. This looks odd to me. The usual way to handle iterating the entire submitter controlled data is to just iterate over the bvec array, as done by bio_for_each_segment_all/bio_for_each_bvec_all for the bio data. I think you want to do the same here, probably with a similar bip_for_each_bvec_all or similar helper. That way you don't need to stash away the iter. Currently we have the field for that, but I really want to split up struct bio_integrity_payload into what is actually needed for the payload and stuff only needed for the block layer autogenerated PI (bip_bio/bio_iter/bip_work).
On Fri, Aug 23, 2024 at 04:08:05PM +0530, Anuj Gupta wrote: > +struct uio_meta { > + meta_flags_t flags; > + u16 app_tag; > + struct iov_iter iter; > +}; Odd formatting here - the aligning tab goes before the field name, not the name of the structure type.
This patch came in twice, once with a "block" and once with a "block,nvme" prefix. One should be fine, and I think just block is the right one. How do we communicate what flags can be combined to the upper layer and userspace given the SCSI limitations here? > --- a/include/linux/bio-integrity.h > +++ b/include/linux/bio-integrity.h > @@ -11,6 +11,9 @@ enum bip_flags { > BIP_DISK_NOCHECK = 1 << 3, /* disable disk integrity checking */ > BIP_IP_CHECKSUM = 1 << 4, /* IP checksum */ > BIP_COPY_USER = 1 << 5, /* Kernel bounce buffer in use */ > + BIP_CHECK_GUARD = 1 << 6, > + BIP_CHECK_REFTAG = 1 << 7, > + BIP_CHECK_APPTAG = 1 << 8, Maybe describe the flags here like the other ones?
On Fri, Aug 23, 2024 at 04:08:09PM +0530, Anuj Gupta wrote: > From: Kanchan Joshi <joshi.k@samsung.com> > > If iocb contains the meta, extract that and prepare the bip. If an iocb contains metadata, ... > --- a/block/fops.c > +++ b/block/fops.c > @@ -154,6 +154,9 @@ static void blkdev_bio_end_io(struct bio *bio) > } > } > > + if (bio_integrity(bio) && (dio->iocb->ki_flags & IOCB_HAS_META)) > + bio_integrity_unmap_user(bio); How could bio_integrity() be true here without the iocb flag? > + if (!is_sync && unlikely(iocb->ki_flags & IOCB_HAS_META)) { unlikely is actively harmful here, as the code is likely if you use the feature.. > + ret = bio_integrity_map_iter(bio, iocb->private); > + if (unlikely(ret)) { > + bio_release_pages(bio, false); > + bio_clear_flag(bio, BIO_REFFED); > + bio_put(bio); > + blk_finish_plug(&plug); > + return ret; > + } This duplicates the error handling done just above. Please add a goto label to de-duplicate it. > + if (unlikely(iocb->ki_flags & IOCB_HAS_META)) { > + ret = bio_integrity_map_iter(bio, iocb->private); > + WRITE_ONCE(iocb->private, NULL); > + if (unlikely(ret)) { > + bio_put(bio); > + return ret; This probably also wants an out_bio_put label even if the duplication is minimal so far. You probably also want a WARN_ON for the iocb meta flag in __blkdev_direct_IO_simple so that we don't get caught off guard if someone adds a synchronous path using PI. > diff --git a/block/t10-pi.c b/block/t10-pi.c > index e7052a728966..cb7bc4a88380 100644 > --- a/block/t10-pi.c > +++ b/block/t10-pi.c > @@ -139,6 +139,8 @@ static void t10_pi_type1_prepare(struct request *rq) > /* Already remapped? */ > if (bip->bip_flags & BIP_MAPPED_INTEGRITY) > break; > + if (bip->bip_flags & BIP_INTEGRITY_USER) > + break; This is wrong. When submitting metadata on a partition the ref tag does need to be remapped. Please also add a tests that tests submitting metadata on a partition so that we have a regression test for this. > + BIP_INTEGRITY_USER = 1 << 9, /* Integrity payload is user > + * address > + */ .. and with the above fix this flag should not be needed. > }; > > struct bio_integrity_payload { > @@ -24,6 +27,7 @@ struct bio_integrity_payload { > unsigned short bip_vcnt; /* # of integrity bio_vecs */ > unsigned short bip_max_vcnt; /* integrity bio_vec slots */ > unsigned short bip_flags; /* control flags */ > + u16 app_tag; Please document the field even if it seems obvious.
> Add support for sending user-meta buffer. Set tags to be checked > using flags specified by user/block-layer user and underlying DIF/DIX > configuration. Introduce BLK_INTEGRITY_APP_TAG to specify apptag. > This provides a way for upper layers to specify apptag checking. We'll also need that flag for nvme, don't we? It should also be added in a block layer patch and not as part of a driver patch. > +/* > + * Can't check reftag alone or apptag alone > + */ > +static bool sd_prot_flags_valid(struct scsi_cmnd *scmd) > +{ > + struct request *rq = scsi_cmd_to_rq(scmd); > + struct bio *bio = rq->bio; > + > + if (bio_integrity_flagged(bio, BIP_CHECK_REFTAG) && > + !bio_integrity_flagged(bio, BIP_CHECK_APPTAG)) > + return false; > + if (!bio_integrity_flagged(bio, BIP_CHECK_REFTAG) && > + bio_integrity_flagged(bio, BIP_CHECK_APPTAG)) > + return false; > + return true; > +} We'll need to advertise this limitations to the application or in-kernel user somehow.. > + if ((bio_integrity_flagged(bio, BIP_CTRL_NOCHECK) == false) && > + (!dix || bio_integrity_flagged(bio, BIP_CHECK_REFTAG))) Incorrect formatting. This is better: if (!bio_integrity_flagged(bio, BIP_CTRL_NOCHECK) && (!dix || bio_integrity_flagged(bio, BIP_CHECK_REFTAG)))
On Sat, Aug 24, 2024 at 10:31:16AM +0200, Christoph Hellwig wrote: > On Fri, Aug 23, 2024 at 04:08:03PM +0530, Anuj Gupta wrote: > > Copy back the bounce buffer to user-space in entirety when the parent > > bio completes. > > This looks odd to me. The usual way to handle iterating the entire > submitter controlled data is to just iterate over the bvec array, as > done by bio_for_each_segment_all/bio_for_each_bvec_all for the bio > data. I think you want to do the same here, probably with a > similar bip_for_each_bvec_all or similar helper. That way you don't > need to stash away the iter. Currently we have the field for that, > but I really want to split up struct bio_integrity_payload into > what is actually needed for the payload and stuff only needed for > the block layer autogenerated PI (bip_bio/bio_iter/bip_work). I can add it [*], to iterate over the entire bvec array. But the original bio_iter still needs to be stored during submission, to calculate the number of bytes in the original integrity/metadata iter (as it could have gotten split, and I don't have original integrity iter stored anywhere). Do you have a different opinion? [*] diff --git a/block/bio-integrity.c b/block/bio-integrity.c index ff7de4fe74c4..f1690c644e70 100644 --- a/block/bio-integrity.c +++ b/block/bio-integrity.c @@ -125,11 +125,23 @@ static void bio_integrity_uncopy_user(struct bio_integrity_payload *bip) struct bio_vec *copy = &bip->bip_vec[1]; size_t bytes = bio_iter_integrity_bytes(bi, bip->bio_iter); struct iov_iter iter; - int ret; + struct bio_vec *bvec; + struct bvec_iter_all iter_all; iov_iter_bvec(&iter, ITER_DEST, copy, nr_vecs, bytes); - ret = copy_to_iter(bvec_virt(bip->bip_vec), bytes, &iter); - WARN_ON_ONCE(ret != bytes); + bip_for_each_segment_all(bvec, bip, iter_all) { + ssize_t ret; + + ret = copy_page_to_iter(bvec->bv_page, + bvec->bv_offset, + bvec->bv_len, + &iter); + + if (!iov_iter_count(&iter)) + break; + + WARN_ON_ONCE(ret < bvec->bv_len); + } bio_integrity_unpin_bvec(copy, nr_vecs, true); } diff --git a/include/linux/bio-integrity.h b/include/linux/bio-integrity.h index 22ff2ae16444..3132ef6f27e0 100644 --- a/include/linux/bio-integrity.h +++ b/include/linux/bio-integrity.h @@ -46,6 +46,19 @@ struct uio_meta { struct iov_iter iter; }; +static inline bool bip_next_segment(const struct bio_integrity_payload *bip, + struct bvec_iter_all *iter) +{ + if (iter->idx >= bip->bip_vcnt) + return false; + + bvec_advance(&bip->bip_vec[iter->idx], iter); + return true; +} + +#define bip_for_each_segment_all(bvl, bip, iter) \ + for (bvl = bvec_init_iter_all(&iter); bip_next_segment((bip), &iter); ) + #define BIP_CLONE_FLAGS (BIP_MAPPED_INTEGRITY | BIP_CTRL_NOCHECK | \ BIP_DISK_NOCHECK | BIP_IP_CHECKSUM | \ BIP_CHECK_GUARD | BIP_CHECK_REFTAG | \
On 8/24/2024 2:05 PM, Christoph Hellwig wrote: > How do we communicate what flags can be combined to the upper layer > and userspace given the SCSI limitations here? Will adding a new read-only sysfs attribute (under the group [*]) that publishes all valid combinations be fine? With Guard/Reftag/Apptag, we get 6 combinations. For NVMe, all can be valid. For SCSI, maximum 4 can be valid. And we factor the pi-type in while listing what all is valid. For example: 010 or 001 is not valid for SCSI and should not be shown by this. [*] const struct attribute_group blk_integrity_attr_group = { .name = "integrity", .attrs = integrity_attrs, };
Anuj, > Introduce BIP_CLONE_FLAGS describing integrity flags that should be > inherited in the cloned bip from the parent. Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Kanchan, > With Guard/Reftag/Apptag, we get 6 combinations. For NVMe, all can be > valid. For SCSI, maximum 4 can be valid. And we factor the pi-type in > while listing what all is valid. For example: 010 or 001 is not valid > for SCSI and should not be shown by this. I thought we had tentatively agreed to let the block layer integrity flags only describe what the controller should do? And then let sd.c decide what to do about RDPROTECT/WRPROTECT (since host-to-target is a different protection envelope anyway). That is kind of how it works already.
On Wed, Aug 28, 2024 at 04:48:06PM +0530, Anuj Gupta wrote: > I can add it [*], to iterate over the entire bvec array. But the original > bio_iter still needs to be stored during submission, to calculate the > number of bytes in the original integrity/metadata iter (as it could have > gotten split, and I don't have original integrity iter stored anywhere). > Do you have a different opinion? Just like for the bio data, the original submitter should never use the iter, but the _all iter.
On Wed, Aug 28, 2024 at 07:12:22PM +0530, Kanchan Joshi wrote: > On 8/24/2024 2:05 PM, Christoph Hellwig wrote: > > How do we communicate what flags can be combined to the upper layer > > and userspace given the SCSI limitations here? > > Will adding a new read-only sysfs attribute (under the group [*]) that > publishes all valid combinations be fine? > That's not a good application interface. Finding the sysfs file is already a pain for direct users of the block device, and almost impossible when going through a file system.
On Wed, Aug 28, 2024 at 11:16:53PM -0400, Martin K. Petersen wrote: > I thought we had tentatively agreed to let the block layer integrity > flags only describe what the controller should do? And then let sd.c > decide what to do about RDPROTECT/WRPROTECT (since host-to-target is a > different protection envelope anyway). That is kind of how it works > already. Yes, that should easier to manage.
On Thu, Aug 29, 2024 at 8:47 AM Martin K. Petersen <martin.petersen@oracle.com> wrote: > > > Kanchan, > > > With Guard/Reftag/Apptag, we get 6 combinations. For NVMe, all can be > > valid. For SCSI, maximum 4 can be valid. And we factor the pi-type in > > while listing what all is valid. For example: 010 or 001 is not valid > > for SCSI and should not be shown by this. > > I thought we had tentatively agreed to let the block layer integrity > flags only describe what the controller should do? And then let sd.c > decide what to do about RDPROTECT/WRPROTECT (since host-to-target is a > different protection envelope anyway). That is kind of how it works > already. > Do you see that this patch (and this set of flags) are fine? If not, which specific flags do you suggest should be introduced?
Martin, Christoph On 29/08/24 06:59PM, Anuj gupta wrote: >On Thu, Aug 29, 2024 at 8:47 AM Martin K. Petersen ><martin.petersen@oracle.com> wrote: >> >> >> Kanchan, >> >> > With Guard/Reftag/Apptag, we get 6 combinations. For NVMe, all can be >> > valid. For SCSI, maximum 4 can be valid. And we factor the pi-type in >> > while listing what all is valid. For example: 010 or 001 is not valid >> > for SCSI and should not be shown by this. >> >> I thought we had tentatively agreed to let the block layer integrity >> flags only describe what the controller should do? And then let sd.c >> decide what to do about RDPROTECT/WRPROTECT (since host-to-target is a >> different protection envelope anyway). That is kind of how it works >> already. >> >Do you see that this patch (and this set of flags) are fine? >If not, which specific flags do you suggest should be introduced? While other things are sorted for next iteration, it's not fully clear what are we missing for this part. Can you comment on the above?
Anuj, > Do you see that this patch (and this set of flags) are fine? > If not, which specific flags do you suggest should be introduced? The three exposed BIP flags are fine. We'll deal with the target flag peculiarities in the SCSI disk driver.
This adds a new io_uring interface to exchange meta along with read/write. Interface: Meta information is represented using a newly introduced 'struct io_uring_meta'. Application sets up a SQE128 ring, and prepares io_uring_meta within second SQE. Application populates 'struct io_uring_meta' fields as below: * meta_type: describes type of meta that is passed. Currently one type "Integrity" is supported. * meta_flags: these are meta-type specific flags. Three flags are exposed for integrity type, namely INTEGRITY_CHK_GUARD/APPTAG/REFTAG. * meta_len: length of the meta buffer * meta_addr: address of the meta buffer * app_tag: optional application-specific 16b value; this goes along with INTEGRITY_CHK_APPTAG flag. Block path (direct IO) , NVMe and SCSI driver are modified to support this. The first three patches are required to make the user metadata split work correctly. Patch 4,5 are prep patches. Patch 6 adds the io_uring support. Patch 7 gives us unified interface for user and kernel generated integrity. Patch 8 adds the support for block direct IO, patch 9 for NVMe, and patch 10 for SCSI. Some of the design choices came from this discussion [2]. Example program on how to use the interface is appended below [3] Tree: https://github.com/SamsungDS/linux/tree/feat/pi_us_v3 Testing: has been done by modifying fio to use this interface. https://github.sec.samsung.net/DS8-MemoryOpenSource/fio/tree/feat/test-meta-v4 Changes since v2: https://lore.kernel.org/linux-block/20240626100700.3629-1-anuj20.g@samsung.com/ - io_uring error handling styling (Gabriel) - add documented helper to get metadata bytes from data iter (hch) - during clone specify "what flags to clone" rather than "what not to clone" (hch) - Move uio_meta defination to bio-integrity.h (hch) - Rename apptag field to app_tag (hch) - Change datatype of flags field in uio_meta to bitwise (hch) - Don't introduce BIP_USER_CHK_FOO flags (hch, martin) - Driver should rely on block layer flags instead of seeing if it is user-passthrough (hch) - update the scsi code for handling user-meta (hch, martin) Changes since v1: https://lore.kernel.org/linux-block/20240425183943.6319-1-joshi.k@samsung.com/ - Do not use new opcode for meta, and also add the provision to introduce new meta types beyond integrity (Pavel) - Stuff IOCB_HAS_META check in need_complete_io (Jens) - Split meta handling in NVMe into a separate handler (Keith) - Add meta handling for __blkdev_direct_IO too (Keith) - Don't inherit BIP_COPY_USER flag for cloned bio's (Christoph) - Better commit descriptions (Christoph) Changes since RFC: - modify io_uring plumbing based on recent async handling state changes - fixes/enhancements to correctly handle the split for meta buffer - add flags to specify guard/reftag/apptag checks - add support to send apptag [1] https://lore.kernel.org/linux-block/719d2e-b0e6-663c-ec38-acf939e4a04b@redhat.com/ [2] https://lore.kernel.org/linux-block/20240705083205.2111277-1-hch@lst.de/ [3] #define _GNU_SOURCE #include <stdio.h> #include <stdlib.h> #include <string.h> #include <linux/io_uring.h> #include <linux/types.h> #include "liburing.h" /* write data/meta. read both. compare. send apptag too. * prerequisite: * unprotected xfer: format namespace with 4KB + 8b, pi_type = 0 * protected xfer: format namespace with 4KB + 8b, pi_type = 1 */ #define DATA_LEN 4096 #define META_LEN 8 struct t10_pi_tuple { __be16 guard; __be16 apptag; __be32 reftag; }; int main(int argc, char *argv[]) { struct io_uring ring; struct io_uring_sqe *sqe = NULL; struct io_uring_cqe *cqe = NULL; void *wdb,*rdb; char wmb[META_LEN], rmb[META_LEN]; char *data_str = "data buffer"; char *meta_str = "meta"; int fd, ret, blksize; struct stat fstat; unsigned long long offset = DATA_LEN; struct t10_pi_tuple *pi; struct io_uring_meta *md; if (argc != 2) { fprintf(stderr, "Usage: %s <block-device>", argv[0]); return 1; }; if (stat(argv[1], &fstat) == 0) { blksize = (int)fstat.st_blksize; } else { perror("stat"); return 1; } if (posix_memalign(&wdb, blksize, DATA_LEN)) { perror("posix_memalign failed"); return 1; } if (posix_memalign(&rdb, blksize, DATA_LEN)) { perror("posix_memalign failed"); return 1; } strcpy(wdb, data_str); strcpy(wmb, meta_str); fd = open(argv[1], O_RDWR | O_DIRECT); if (fd < 0) { printf("Error in opening device\n"); return 0; } ret = io_uring_queue_init(8, &ring, IORING_SETUP_SQE128); if (ret) { fprintf(stderr, "ring setup failed: %d\n", ret); return 1; } /* write data + meta-buffer to device */ sqe = io_uring_get_sqe(&ring); if (!sqe) { fprintf(stderr, "get sqe failed\n"); return 1; } io_uring_prep_write(sqe, fd, wdb, DATA_LEN, offset); md = (struct io_uring_meta *) sqe->big_sqe_cmd; md->meta_type = META_TYPE_INTEGRITY; md->meta_addr = (__u64)wmb; md->meta_len = META_LEN; /* flags to ask for guard/reftag/apptag*/ md->meta_flags = INTEGRITY_CHK_APPTAG; md->app_tag = 0x1234; pi = (struct t10_pi_tuple *)wmb; pi->apptag = 0x3412; ret = io_uring_submit(&ring); if (ret <= 0) { fprintf(stderr, "sqe submit failed: %d\n", ret); return 1; } ret = io_uring_wait_cqe(&ring, &cqe); if (!cqe) { fprintf(stderr, "cqe is NULL :%d\n", ret); return 1; } if (cqe->res < 0) { fprintf(stderr, "write cqe failure: %d", cqe->res); return 1; } io_uring_cqe_seen(&ring, cqe); /* read data + meta-buffer back from device */ sqe = io_uring_get_sqe(&ring); if (!sqe) { fprintf(stderr, "get sqe failed\n"); return 1; } io_uring_prep_read(sqe, fd, rdb, DATA_LEN, offset); md = (struct io_uring_meta *) sqe->big_sqe_cmd; md->meta_type = META_TYPE_INTEGRITY; md->meta_addr = (__u64)rmb; md->meta_len = META_LEN; md->meta_flags = INTEGRITY_CHK_APPTAG; md->app_tag = 0x1234; ret = io_uring_submit(&ring); if (ret <= 0) { fprintf(stderr, "sqe submit failed: %d\n", ret); return 1; } ret = io_uring_wait_cqe(&ring, &cqe); if (!cqe) { fprintf(stderr, "cqe is NULL :%d\n", ret); return 1; } if (cqe->res < 0) { fprintf(stderr, "read cqe failure: %d", cqe->res); return 1; } io_uring_cqe_seen(&ring, cqe); if (strncmp(wmb, rmb, META_LEN)) printf("Failure: meta mismatch!, wmb=%s, rmb=%s\n", wmb, rmb); if (strncmp(wdb, rdb, DATA_LEN)) printf("Failure: data mismatch!\n"); io_uring_queue_exit(&ring); free(rdb); free(wdb); return 0; } Anuj Gupta (7): block: define set of integrity flags to be inherited by cloned bip block: introduce a helper to determine metadata bytes from data iter block: handle split correctly for user meta bounce buffer block: modify bio_integrity_map_user to accept iov_iter as argument io_uring/rw: add support to send meta along with read/write block,nvme: introduce BIP_CHECK_GUARD/REFTAG/APPTAG bip_flags scsi: add support for user-meta interface Kanchan Joshi (3): block: define meta io descriptor block: add support to pass user meta buffer nvme: add handling for app_tag block/bio-integrity.c | 71 ++++++++++++++++++++++++++++++----- block/fops.c | 25 ++++++++++++ block/t10-pi.c | 6 +++ drivers/nvme/host/core.c | 24 +++++++----- drivers/nvme/host/ioctl.c | 11 +++++- drivers/scsi/sd.c | 25 +++++++++++- drivers/scsi/sd_dif.c | 2 +- include/linux/bio-integrity.h | 33 ++++++++++++++-- include/linux/blk-integrity.h | 17 +++++++++ include/linux/fs.h | 1 + include/uapi/linux/io_uring.h | 32 ++++++++++++++++ io_uring/io_uring.c | 6 +++ io_uring/rw.c | 70 ++++++++++++++++++++++++++++++++-- io_uring/rw.h | 10 ++++- 14 files changed, 302 insertions(+), 31 deletions(-)