Re: [PATCH v3 08/10] block: add support to pass user meta buffer

[Christoph Hellwig <hch@lst.de>]

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.