Re: [PATCH v3 14/15] nvme: Support atomic writes

[John Garry]

On 13/02/2024 06:42, Christoph Hellwig wrote:
> On Wed, Jan 24, 2024 at 11:38:40AM +0000, John Garry wrote:
> > From: Alan Adamson <alan.adamson@xxxxxxxxxx>
> >
> > Support reading atomic write registers to fill in request_queue
> > properties.
> >
> > Use following method to calculate limits:
> > atomic_write_max_bytes = flp2(NAWUPF ?: AWUPF)
> > atomic_write_unit_min = logical_block_size
> > atomic_write_unit_max = flp2(NAWUPF ?: AWUPF)
> > atomic_write_boundary = NABSPF
>
> Can you expand this to actually be a real commit log with full
> sentences, expanding the NVME field name acronyms and reference
> the relevant Sections and Figures in a specific version of the
> NVMe specification?

ok

> Also some implementation comments:
>
> NVMe has a particularly nasty NABO field in Identify Namespace, which
> offsets the boundary. We probably need to reject atomic writes or
> severly limit them if this field is set.

ok, and initially we'll just not support atomic writes for NABO != 0

> Please also read through TP4098(a) and look at the MAM field.

It's not public, AFAIK.

And I don't think a feature which allows us to straddle boundaries is 
too interesting really.

>  As far
> as I can tell the patch as-is assumes it always is set to 1.
>
> > +static void nvme_update_atomic_write_disk_info(struct gendisk *disk,
> > +		struct nvme_ctrl *ctrl, struct nvme_id_ns *id, u32 bs, u32 atomic_bs)
>
> Please avoid the overly long line here.
>
> > +		nvme_update_atomic_write_disk_info(disk, ctrl, id, bs, atomic_bs);
>
> .. and here.

ok, but I think that this will get shorter anyway.

>

Thanks,
John