[PATCH] nvme: don't apply NVME_QUIRK_DEALLOCATE_ZEROES when DSM is not supported

[PATCH] nvme: don't apply NVME_QUIRK_DEALLOCATE_ZEROES when DSM is not supported

[Christoph Hellwig]

Commit 63dfa1004322 ("nvme: move NVME_QUIRK_DEALLOCATE_ZEROES out of
nvme_config_discard") started applying the NVME_QUIRK_DEALLOCATE_ZEROES
quirk even then the Dataset Management is not supported.  It turns out
that there versions of these old Intel SSDs that have DSM support
disabled in the firmware, which will now lead to errors everytime
a Write Zeroes command is issued.  Fix this by checking for DSM support
before applying the quirk.

Reported-by: Saeed Mirzamohammadi <saeed.mirzamohammadi@oracle.com>
Fixes: 63dfa1004322 ("nvme: move NVME_QUIRK_DEALLOCATE_ZEROES out of nvme_config_discard")
Tested-by: Saeed Mirzamohammadi <saeed.mirzamohammadi@oracle.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index bfd71511c85f..5e5c9e15ad0c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2043,7 +2043,8 @@ static bool nvme_update_disk_info(struct nvme_ns *ns, struct nvme_id_ns *id,
 	lim->physical_block_size = min(phys_bs, atomic_bs);
 	lim->io_min = phys_bs;
 	lim->io_opt = io_opt;
-	if (ns->ctrl->quirks & NVME_QUIRK_DEALLOCATE_ZEROES)
+	if ((ns->ctrl->quirks & NVME_QUIRK_DEALLOCATE_ZEROES) &&
+	    (ns->ctrl->oncs & NVME_CTRL_ONCS_DSM))
 		lim->max_write_zeroes_sectors = UINT_MAX;
 	else
 		lim->max_write_zeroes_sectors = ns->ctrl->max_zeroes_sectors;
-- 
2.45.2

Re: [PATCH] nvme: don't apply NVME_QUIRK_DEALLOCATE_ZEROES when DSM is not supported

[Chaitanya Kulkarni]

On 11/26/24 22:42, Christoph Hellwig wrote:
> Commit 63dfa1004322 ("nvme: move NVME_QUIRK_DEALLOCATE_ZEROES out of
> nvme_config_discard") started applying the NVME_QUIRK_DEALLOCATE_ZEROES
> quirk even then the Dataset Management is not supported.  It turns out
> that there versions of these old Intel SSDs that have DSM support
> disabled in the firmware, which will now lead to errors everytime
> a Write Zeroes command is issued.  Fix this by checking for DSM support
> before applying the quirk.
>
> Reported-by: Saeed Mirzamohammadi<saeed.mirzamohammadi@oracle.com>
> Fixes: 63dfa1004322 ("nvme: move NVME_QUIRK_DEALLOCATE_ZEROES out of nvme_config_discard")
> Tested-by: Saeed Mirzamohammadi<saeed.mirzamohammadi@oracle.com>
> Signed-off-by: Christoph Hellwig<hch@lst.de>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH] nvme: don't apply NVME_QUIRK_DEALLOCATE_ZEROES when DSM is not supported

[Nitesh Shetty]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

On 27/11/24 07:42AM, Christoph Hellwig wrote:
>Commit 63dfa1004322 ("nvme: move NVME_QUIRK_DEALLOCATE_ZEROES out of
>nvme_config_discard") started applying the NVME_QUIRK_DEALLOCATE_ZEROES
>quirk even then the Dataset Management is not supported.  It turns out
>that there versions of these old Intel SSDs that have DSM support
>disabled in the firmware, which will now lead to errors everytime
>a Write Zeroes command is issued.  Fix this by checking for DSM support
>before applying the quirk.
>
>Reported-by: Saeed Mirzamohammadi <saeed.mirzamohammadi@oracle.com>
>Fixes: 63dfa1004322 ("nvme: move NVME_QUIRK_DEALLOCATE_ZEROES out of nvme_config_discard")
>Tested-by: Saeed Mirzamohammadi <saeed.mirzamohammadi@oracle.com>
>Signed-off-by: Christoph Hellwig <hch@lst.de>
>---

Reviewed-by: Nitesh Shetty <nj.shetty@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] nvme: don't apply NVME_QUIRK_DEALLOCATE_ZEROES when DSM is not supported

[Keith Busch]

On Wed, Nov 27, 2024 at 07:42:18AM +0100, Christoph Hellwig wrote:
> Commit 63dfa1004322 ("nvme: move NVME_QUIRK_DEALLOCATE_ZEROES out of
> nvme_config_discard") started applying the NVME_QUIRK_DEALLOCATE_ZEROES
> quirk even then the Dataset Management is not supported.  It turns out
> that there versions of these old Intel SSDs that have DSM support
> disabled in the firmware, which will now lead to errors everytime
> a Write Zeroes command is issued.  Fix this by checking for DSM support
> before applying the quirk.

I still think this is wrong, and we should just remove the quirk for
this device. With the exception to this firmware version, this device
generally supports both discards and write zeroes. The only reason it
added this quirk was because the quirk used to mean something completely
different (specifically, it would set the "discard_zeroes_data"
attribute that's no longer used). It didn't mean to prefer discards over
write zeroes, but that's what it means now, and that's not what this
drive wants.

Re: [PATCH] nvme: don't apply NVME_QUIRK_DEALLOCATE_ZEROES when DSM is not supported

[Christoph Hellwig]

On Wed, Nov 27, 2024 at 08:45:04AM -0700, Keith Busch wrote:
> I still think this is wrong, and we should just remove the quirk for
> this device. With the exception to this firmware version, this device
> generally supports both discards and write zeroes. The only reason it
> added this quirk was because the quirk used to mean something completely
> different (specifically, it would set the "discard_zeroes_data"
> attribute that's no longer used). It didn't mean to prefer discards over
> write zeroes, but that's what it means now, and that's not what this
> drive wants.

I'll have to trust you on this device, and certainly won't object
removing this weirdo quick.  But as long as we don't remove the
quirk entirely we'll also need this fix.  So I guess we should go
for both?

Re: [PATCH] nvme: don't apply NVME_QUIRK_DEALLOCATE_ZEROES when DSM is not supported

[Keith Busch]

On Wed, Nov 27, 2024 at 04:48:12PM +0100, Christoph Hellwig wrote:
> On Wed, Nov 27, 2024 at 08:45:04AM -0700, Keith Busch wrote:
> > I still think this is wrong, and we should just remove the quirk for
> > this device. With the exception to this firmware version, this device
> > generally supports both discards and write zeroes. The only reason it
> > added this quirk was because the quirk used to mean something completely
> > different (specifically, it would set the "discard_zeroes_data"
> > attribute that's no longer used). It didn't mean to prefer discards over
> > write zeroes, but that's what it means now, and that's not what this
> > drive wants.
> 
> I'll have to trust you on this device, and certainly won't object
> removing this weirdo quick.  But as long as we don't remove the
> quirk entirely we'll also need this fix.  So I guess we should go
> for both?

Sure, that sounds good to me.

Re: [PATCH] nvme: don't apply NVME_QUIRK_DEALLOCATE_ZEROES when DSM is not supported

[Keith Busch]

On Wed, Nov 27, 2024 at 07:42:18AM +0100, Christoph Hellwig wrote:
> Commit 63dfa1004322 ("nvme: move NVME_QUIRK_DEALLOCATE_ZEROES out of
> nvme_config_discard") started applying the NVME_QUIRK_DEALLOCATE_ZEROES
> quirk even then the Dataset Management is not supported.  It turns out
> that there versions of these old Intel SSDs that have DSM support
> disabled in the firmware, which will now lead to errors everytime
> a Write Zeroes command is issued.  Fix this by checking for DSM support
> before applying the quirk.
> 
> Reported-by: Saeed Mirzamohammadi <saeed.mirzamohammadi@oracle.com>
> Fixes: 63dfa1004322 ("nvme: move NVME_QUIRK_DEALLOCATE_ZEROES out of nvme_config_discard")
> Tested-by: Saeed Mirzamohammadi <saeed.mirzamohammadi@oracle.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Thanks, applied to nvme-6.13.

Re: [PATCH] nvme: don't apply NVME_QUIRK_DEALLOCATE_ZEROES when DSM is not supported

[Saeed Mirzamohammadi]

Thanks! Can this be applied to 6.11+ stable releases as well since the broken commit is in 6.9.y?

Saeed

> On Dec 2, 2024, at 9:54 AM, Keith Busch <kbusch@kernel.org> wrote:
> 
> On Wed, Nov 27, 2024 at 07:42:18AM +0100, Christoph Hellwig wrote:
>> Commit 63dfa1004322 ("nvme: move NVME_QUIRK_DEALLOCATE_ZEROES out of
>> nvme_config_discard") started applying the NVME_QUIRK_DEALLOCATE_ZEROES
>> quirk even then the Dataset Management is not supported.  It turns out
>> that there versions of these old Intel SSDs that have DSM support
>> disabled in the firmware, which will now lead to errors everytime
>> a Write Zeroes command is issued.  Fix this by checking for DSM support
>> before applying the quirk.
>> 
>> Reported-by: Saeed Mirzamohammadi <saeed.mirzamohammadi@oracle.com>
>> Fixes: 63dfa1004322 ("nvme: move NVME_QUIRK_DEALLOCATE_ZEROES out of nvme_config_discard")
>> Tested-by: Saeed Mirzamohammadi <saeed.mirzamohammadi@oracle.com>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Thanks, applied to nvme-6.13.

Re: [PATCH] nvme: don't apply NVME_QUIRK_DEALLOCATE_ZEROES when DSM is not supported

[Keith Busch]

On Mon, Dec 02, 2024 at 06:03:02PM +0000, Saeed Mirzamohammadi wrote:
> Thanks! Can this be applied to 6.11+ stable releases as well since the broken commit is in 6.9.y?

I don't think it can go to 6.9.y because it is not an LTS and that
branch appears to have already ended stable support.

Since this patch has the "Fixes" tag though, the stable auto patch
selection should grab it for the appropriate stable branches that are
still supported, so branches like 6.11.y and 6.6.y should get it. I'll
keep an eye out for it after the next pull request just to make sure.