Re: [PATCH v2] block: change rq_integrity_vec to respect the iterator

[Jens Axboe]

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.


-- 
Jens Axboe