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.
> ns->head->features |= NVME_NS_DEAC;
I think we should not set NVME_NS_DEAC for the quirks case.
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
