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 <[email protected]> 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
