On Fri, Aug 23, 2024 at 04:08:09PM +0530, Anuj Gupta wrote:
> From: Kanchan Joshi <joshi.k@xxxxxxxxxxx>
>
> 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.
