Dear Zhang Yi On Thu, Feb 26, 2026 at 3:09 AM Zhang Yi <[email protected]> wrote: > > 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.
Thank you for your confirmation. I will follow up and submit the patch to other maintainers for their review. Best regards, Robert > 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 >
