On Thu, 23 May 2024, Jens Axboe wrote:
> On 5/23/24 9:11 AM, Mikulas Patocka wrote:
> >>> @@ -853,16 +855,20 @@ static blk_status_t nvme_prep_rq(struct
> >>> goto out_free_cmd;
> >>> }
> >>>
> >>> +#ifdef CONFIG_BLK_DEV_INTEGRITY
> >>> if (blk_integrity_rq(req)) {
> >>> ret = nvme_map_metadata(dev, req, &iod->cmd);
> >>> if (ret)
> >>> goto out_unmap_data;
> >>> }
> >>> +#endif
> >>
> >> if (IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) && blk_integrity_rq(req)) {
> >>
> >> ?
> >
> > That wouldn't work, because the calls to rq_integrity_vec need to be
> > eliminated by the preprocessor.
>
> Why not just do this incremental? Cleans up the ifdef mess too, leaving
> only the one actually using rq_integrity_vec in place.
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 5f857cbc95c8..bd56416a7fa8 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -821,10 +821,10 @@ static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,
> return ret;
> }
>
> -#ifdef CONFIG_BLK_DEV_INTEGRITY
> static blk_status_t nvme_map_metadata(struct nvme_dev *dev, struct request *req,
> struct nvme_command *cmnd)
> {
> +#ifdef CONFIG_BLK_DEV_INTEGRITY
> struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> struct bio_vec bv = rq_integrity_vec(req);
>
> @@ -832,9 +832,9 @@ static blk_status_t nvme_map_metadata(struct nvme_dev *dev, struct request *req,
> if (dma_mapping_error(dev->dev, iod->meta_dma))
> return BLK_STS_IOERR;
> cmnd->rw.metadata = cpu_to_le64(iod->meta_dma);
> +#endif
> return BLK_STS_OK;
> }
> -#endif
>
> static blk_status_t nvme_prep_rq(struct nvme_dev *dev, struct request *req)
> {
> @@ -855,20 +855,16 @@ static blk_status_t nvme_prep_rq(struct nvme_dev *dev, struct request *req)
> goto out_free_cmd;
> }
>
> -#ifdef CONFIG_BLK_DEV_INTEGRITY
> - if (blk_integrity_rq(req)) {
> + if (IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) && blk_integrity_rq(req)) {
> ret = nvme_map_metadata(dev, req, &iod->cmd);
> if (ret)
> goto out_unmap_data;
> }
> -#endif
>
> nvme_start_request(req);
> return BLK_STS_OK;
> -#ifdef CONFIG_BLK_DEV_INTEGRITY
> out_unmap_data:
> nvme_unmap_data(dev, req);
> -#endif
> out_free_cmd:
> nvme_cleanup_cmd(req);
> return ret;
>
> > Should I change rq_integrity_vec to this? Then, we could get rid of the
> > ifdefs and let the optimizer remove all calls to rq_integrity_vec.
> > static inline struct bio_vec rq_integrity_vec(struct request *rq)
> > {
> > struct bio_vec bv = { };
> > return bv;
> > }
>
> Only if that eliminates runtime checking for !INTEGRITY, which I don't
> thin it will.
It will eliminate the ifdefs. If we are compiling without
CONFIG_BLK_DEV_INTEGRITY, blk_integrity_rq(req) is inline and it always
returns 0. So the optimizer will remove anything guarded with "if
(blk_integrity_rq(req))" - including the calls to nvme_map_metadata and
rq_integrity_vec.
But we need to provide dummy rq_integrity_vec for the compiler front-end.
The front-end doesn't know that blk_integrity_rq always returns zero.
So, the patch will be smaller if we get rid of the ifdefs and provide a
dummy rq_integrity_vec.
Mikulas
>
>
> --
> Jens Axboe
>
