Re: [PATCH v2 2/9] nvme: set max_hw_wzeroes_unmap_sectors if device supports DEAC bit

[Zhang Yi <yi.zhang@huaweicloud.com>]

On 2/26/2026 5:43 AM, Robert Pang wrote:
> Dear Zhang Yi
> 
> Thank you for your quick response. Please see my comments below:
> 
> On Tue, Feb 24, 2026 at 6:32 PM Zhang Yi <yi.zhang@huaweicloud.com> wrote:
>>
>> Hi Robert!
>>
>> On 2/25/2026 8:05 AM, Robert Pang wrote:
>>> Dear Zhang Yi,
>>>
>>> In reviewing your patch series implementing support for the
>>> FALLOC_FL_WRITE_ZEROES flag, I noted the logic propagating
>>> max_write_zeroes_sectors to max_hw_wzeroes_unmap_sectors in commit 545fb46e5bc6
>>> "nvme: set max_hw_wzeroes_unmap_sectors if device supports DEAC bit" [1]. This
>>> appears to be intended for devices that support the Write Zeroes command
>>> alongside the DEAC bit to indicate unmap capability.
>>>
>>> Furthermore, within core.c, the NVME_QUIRK_DEALLOCATE_ZEROES quirk already
>>> identifies devices that deterministically return zeroes after a deallocate
>>> command [2]. This quirk currently enables Write Zeroes support via discard in
>>> existing implementations [3, 4].
>>>
>>> Given this, would it be appropriate to respect NVME_QUIRK_DEALLOCATE_ZEROES also
>>> to enable unmap Write Zeroes for these devices, following the prior commit
>>> 6e02318eaea5 "nvme: add support for the Write Zeroes command" [5]? I have
>>> included a proposed change to nvme_update_ns_info_block() below for your
>>> consideration.
>>>
>>
>> Thank you for your point. Overall, this makes sense to me, but I have one
>> question below.
>>
>>> Best regards
>>> Robert Pang
>>>
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index f5ebcaa2f859..9c7e2cabfab3 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -2422,7 +2422,9 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>>>          * require that, it must be a no-op if reads from deallocated data
>>>          * do not return zeroes.
>>>          */
>>> -       if ((id->dlfeat & 0x7) == 0x1 && (id->dlfeat & (1 << 3))) {
>>> +       if ((id->dlfeat & 0x7) == 0x1 && (id->dlfeat & (1 << 3)) ||
>>> +           (ns->ctrl->quirks & NVME_QUIRK_DEALLOCATE_ZEROES) &&
>>> +           (ns->ctrl->oncs & NVME_CTRL_ONCS_DSM)) {
>>                                 ^^^^^^^^^^^^^^^^^^
>> Why do you want to add a check for NVME_CTRL_ONCS_DSM? In nvme_config_discard(),
>> it appears that we prioritize ctrl->dmrsl, allowing discard to still be
>> supported even on some non-standard devices where NVME_CTRL_ONCS_DSM is not set.
>> In nvme_update_disk_info(), if the device only has NVME_QUIRK_DEALLOCATE_ZEROES,
>> we still populate lim->max_write_zeroes_sectors (which might be non-zero on
>> devices that support NVME_CTRL_ONCS_WRITE_ZEROES). Right? So I'm not sure if we
>> only need to check for NVME_QUIRK_DEALLOCATE_ZEROES here.
>>
> The check for NVME_CTRL_ONCS_DSM is to follow the same check in [3]. There, the
> check was added by 58a0c875ce02 "nvme: don't apply NVME_QUIRK_DEALLOCATE_ZEROES
> when DSM is not supported" [6]. The idea is to limit
> NVME_QUIRK_DEALLOCATE_ZEROES
> to those devices that support DSM.
> 

OK.

>>>                 ns->head->features |= NVME_NS_DEAC;
>>
>> I think we should not set NVME_NS_DEAC for the quirks case.
>>
> Make sense. In that case, will it be more appropriate to set
> max_hw_wzeroes_unmap_sectors in nvme_update_disk_info() where
> NVME_QUIRK_DEALLOCATE_ZEROES is checked? I.e.
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f5ebcaa2f859..3f5dd3f867e9 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2120,9 +2120,10 @@ static bool nvme_update_disk_info(struct
> nvme_ns *ns, struct nvme_id_ns *id,
>         lim->io_min = phys_bs;
>         lim->io_opt = io_opt;
>         if ((ns->ctrl->quirks & NVME_QUIRK_DEALLOCATE_ZEROES) &&
> -           (ns->ctrl->oncs & NVME_CTRL_ONCS_DSM))
> +           (ns->ctrl->oncs & NVME_CTRL_ONCS_DSM)) {
>                 lim->max_write_zeroes_sectors = UINT_MAX;
> -       else
> +               lim->max_hw_wzeroes_unmap_sectors = UINT_MAX;
> +       } else
>                 lim->max_write_zeroes_sectors = ns->ctrl->max_zeroes_sectors;
>         return valid;
>  }
> 

Yeah, it looks good to me.

Best regards,
Yi.

> Best regards
> Robert
> 
>> Cheers,
>> Yi.
>>
>>>                 lim.max_hw_wzeroes_unmap_sectors = lim.max_write_zeroes_sectors;
>>>         }
>>>
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=545fb46e5bc6
>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvme/host/nvme.h#n72
>>> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvme/host/core.c#n938
>>> [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvme/host/core.c#n2122
>>> [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6e02318eaea5
> [6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=58a0c875ce02