| Message ID | 20241016112912.63542-5-anuj20.g@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | Read/Write with meta/integrity | expand |
On 16/10/24 01:35PM, Keith Busch wrote: >On Wed, Oct 16, 2024 at 04:59:05PM +0530, Anuj Gupta wrote: >> +struct uio_meta { >> + meta_flags_t flags; >> + u16 app_tag; >> + u32 seed; >> + struct iov_iter iter; >> +}; > >Is the seed used for anything other than the kernel's t10 generation and >verification? It looks like that's all it is for today, and that part is >skipped for userspace metadata, so I'm not sure we need it. > >I know it's been used for passthrough commands since nvme started >supporitng it, but I don't see why the driver ever bothered. I think it >wasn't necessary and we've been carrying it forward ever since. Not for generation/verfication, but seed is used to remap the ref tag when submitting metadata on a partition (see blk_integrity_prepare/complete). For cases like partitioning, MD/DM cloning, where virtual start sector is different from physical sector remapping is required. It is skipped for passthrough, but we require it for this series where I/O can be done on partition too. Christoph [1], Martin [2] also expressed the need for it in the previous version. [1] https://lore.kernel.org/linux-block/20240824084430.GG8805@lst.de/ [2] https://lore.kernel.org/linux-block/yq17cc0c9p5.fsf@ca-mkp.ca.oracle.com/
On Wed, Oct 16, 2024 at 04:59:05PM +0530, Anuj Gupta wrote: > +/* flags for integrity meta */ > +typedef __u16 __bitwise meta_flags_t; > + > +struct uio_meta { > + meta_flags_t flags; Please either add the actual flags here, or if there is a good reason to do that later add the meta_flags_t type and the member when adding it. Also maybe the type name wants a prefix, maybe uio? Also from looking at the reset of the series uio_meta is in no way block specific and referenced from io_uring. So this probably should go into uio.h?
Anuj, > +struct uio_meta { > + meta_flags_t flags; > + u16 app_tag; > + u32 seed; > + struct iov_iter iter; > +}; Glad to see the seed added. In NVMe it can be a 64-bit value so we should bump the size of that field in the user API. Not sure what to do about the storage tag. For Linux that would probably be owned by the filesystem (as opposed to the application). But I guess one could envision a userland application acting as a storage target and in that case the tag would need to be passed to the kernel.
On Mon, Oct 21, 2024 at 10:11:35PM -0400, Martin K. Petersen wrote: > Glad to see the seed added. In NVMe it can be a 64-bit value so we > should bump the size of that field in the user API. > > Not sure what to do about the storage tag. For Linux that would probably > be owned by the filesystem (as opposed to the application). But I guess > one could envision a userland application acting as a storage target and > in that case the tag would need to be passed to the kernel. Right now the series only supports PI on the block device. In that case the userspace application can clearly make use of it. If we want to support PI on file systems (which I'd really like to do for XFS) both ownership models might make sense depending on the file system, although I think just giving it to the application is going to be the only thing we'll see for a while. Maybe we need a way to query if the application can use the app tag?
Christoph, >> Not sure what to do about the storage tag. For Linux that would >> probably be owned by the filesystem (as opposed to the application). >> But I guess one could envision a userland application acting as a >> storage target and in that case the tag would need to be passed to >> the kernel. > > Right now the series only supports PI on the block device. In that > case the userspace application can clearly make use of it. If we > want to support PI on file systems (which I'd really like to do for > XFS) both ownership models might make sense depending on the file > system, although I think just giving it to the application is going > to be the only thing we'll see for a while. Maybe we need a way > to query if the application can use the app tag? tag_size currently indicates the size of the app tag available to the application. I.e. if ATO=1, tag_size is 2. And if ATO=0, tag_size is 0. To properly support the NVMe extensions I guess we'll have to have fields indicating the actual size of the reference and storage tags.
> Not sure what to do about the storage tag. For Linux that would probably > be owned by the filesystem (as opposed to the application). But I guess > one could envision a userland application acting as a storage target and > in that case the tag would need to be passed to the kernel. I will reserve space for storage tag in the user interface for now. That way, we can introduce and use it later when it is actually used. > > -- > Martin K. Petersen Oracle Linux Engineering >
diff --git a/include/linux/bio-integrity.h b/include/linux/bio-integrity.h index 90aab50a3e14..529ec7a8df20 100644 --- a/include/linux/bio-integrity.h +++ b/include/linux/bio-integrity.h @@ -30,6 +30,16 @@ struct bio_integrity_payload { struct bio_vec bip_inline_vecs[];/* embedded bvec array */ }; +/* flags for integrity meta */ +typedef __u16 __bitwise meta_flags_t; + +struct uio_meta { + meta_flags_t flags; + u16 app_tag; + u32 seed; + struct iov_iter iter; +}; + #define BIP_CLONE_FLAGS (BIP_MAPPED_INTEGRITY | BIP_CTRL_NOCHECK | \ BIP_DISK_NOCHECK | BIP_IP_CHECKSUM)