Re: [PATCHv3 07/11] nvmet: implement endurance groups

[Christoph Hellwig <hch@lst.de>]

On Tue, Nov 05, 2024 at 09:49:00AM -0800, Keith Busch wrote:
> +static void nvmet_execute_get_log_page_endgrp(struct nvmet_req *req)
> +{
> +	u64 host_reads, host_writes, data_units_read, data_units_written;
> +	struct nvme_endurance_group_log *log;
> +	u16 status;
> +
> +	req->cmd->common.nsid = cpu_to_le32((u32)
> +				le16_to_cpu(req->cmd->get_log_page.lsi));
> +	status = nvmet_req_find_ns(req);

I think this would be a good place to point out that we "emulate" one
endurance group per namespace, as the code only make sense with that
in mind.

As far as I can tell cpu_to_* does all the normal implicit integer
conversions, so the case here and the ones further down should not be
needed.

> +	host_reads = part_stat_read(req->ns->bdev, ios[READ]);
> +	data_units_read =
> +		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[READ]), 1000);
> +	host_writes = part_stat_read(req->ns->bdev, ios[WRITE]);
> +	data_units_written =
> +		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[WRITE]), 1000);

Overly long lines, but IMHO this would read nicer anyway if the
round is done later, e.g.:

	sectors_written = part_stat_read(req->ns->bdev, sectors[WRITE]);

	...

	put_unaligned_le64(DIV_ROUND_UP(sectors_written, 1000), &log->dur[0]);


> +	xa_for_each(&ctrl->subsys->namespaces, idx, ns) {
> +		if (ns->nsid <= min_endgid)
> +			continue;
> +
> +		list[i++] = cpu_to_le16((u16)ns->nsid);
> +		if (i == buf_size / sizeof(__le16))
> +			break;
> +	}

No really important, but this could use xas_for_each for avoid
doing a full lookup on every iteration.