Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

On 2/10/21 5:20 AM, Christoph Hellwig wrote:

MDTS vs Write Zeroes is a common misconception.  We had that discussion
in the NVMe working group, and while the text should pretty clear that
MDTS only applies to data transfers many implementators did not understand
that, which is why we added the clarification you quoted.

Also the next NVMe spec will allow devices to advertise an explicit
Write Zeroes limit, take a look at TP4040 from the "NVM Express 1.4 Ratified
TPs" at https://nvmexpress.org/developers/nvme-specification/.

So I think limiting to MDTS unless the new WZSL is set might be a fail
safe option, even if it is more pessimistic than what the spec says.

I'd also love to retest most Write Zeroes quirks with that in place.

Do you prefer some variant of the following patch (totally untested)? OR
something else ?

Also, I think we should gradually roll out the MDTS replacement and
still keep the
write-zeroes quirk unless you are okay to just replace at one go.

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d77f3f26d8d3..d5ca27608881 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c

@@ -1167,7 +1167,8 @@ static int nvme_submit_user_cmd(struct

request_queue *q,
                if (ret)
                        goto out;
                bio = req->bio;
-               bio_set_dev(bio, bdev);
+               if(bdev)
+                       bio_set_dev(bio, bdev);
                if (bdev && meta_buffer && meta_len) {
                        meta = nvme_add_user_metadata(bio, meta_buffer,
meta_len,
                                        meta_seed, write);

@@ -1967,6 +1968,7 @@ static void nvme_config_write_zeroes(struct

gendisk *disk, struct nvme_ns *ns)
        u64 max_blocks;
 
        if (!(ns->ctrl->oncs & NVME_CTRL_ONCS_WRITE_ZEROES) ||
+           !(ns->ctrl->quirks & NVME_QUIRK_USE_MDTS_WRITE_ZEROES) ||
            (ns->ctrl->quirks & NVME_QUIRK_DISABLE_WRITE_ZEROES))
                return;
        /*

@@ -1979,10 +1981,15 @@ static void nvme_config_write_zeroes(struct

gendisk *disk, struct nvme_ns *ns)
         * configured based on the controller's MDTS field in the
         * nvme_init_identify() if available.
         */
-       if (ns->ctrl->max_hw_sectors == UINT_MAX)
-               max_blocks = (u64)USHRT_MAX + 1;
-       else
-               max_blocks = ns->ctrl->max_hw_sectors + 1;
+
+       if (!(ns->ctrl->quirks & NVME_QUIRK_USE_MDTS_WRITE_ZEROES)) {
+               if (ns->ctrl->max_hw_sectors == UINT_MAX)
+                       max_blocks = (u64)USHRT_MAX + 1;
+               else
+                       max_blocks = ns->ctrl->max_hw_sectors + 1;
+       } else {
+               max_blocks = ns->ctrl->max_mdts_hw_sectors;
+       }
 
        blk_queue_max_write_zeroes_sectors(disk->queue,
                                           nvme_lba_to_sect(ns,
max_blocks));

@@ -3141,6 +3148,11 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
        ctrl->max_hw_sectors =
                min_not_zero(ctrl->max_hw_sectors, max_hw_sectors);
 
+       /* explicitely set the mdts value to 0 */
+       ctrl->max_mdts_hw_sectors = 0;
+       if (id->mdts)
+               ctrl->max_mdts_hw_sectors =  1 << (id->mdts + page_shift
- 9);
+
        nvme_set_queue_limits(ctrl, ctrl->admin_q);
        ctrl->sgls = le32_to_cpu(id->sgls);
        ctrl->kas = le16_to_cpu(id->kas);

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 0a73e78f09c8..7b60b2196d23 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h

@@ -144,6 +144,11 @@ enum nvme_quirks {
         * NVMe 1.3 compliance.
         */
        NVME_QUIRK_NO_NS_DESC_LIST              = (1 << 15),
+       /*
+        * The controller doesn't allow the write zeores command of size
+        * more than reported by the MDTS
+        */
+       NVME_QUIRK_USE_MDTS_WRITE_ZEROES        = (1 << 16)
 };
 
 /*

@@ -268,6 +273,7 @@ struct nvme_ctrl {
 
        u64 cap;
        u32 max_hw_sectors;
+       u32 max_mdts_hw_sectors;
        u32 max_segments;
        u32 max_integrity_segments;
 #ifdef CONFIG_BLK_DEV_ZONED

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme