On 06/19/2017 12:35 AM, Christoph Hellwig wrote:

Can you add linux-nvme for the next repost?

As said before I think we should rely on implicit streams allocation,
as that will make the whole patch a lot simpler, and it solves the issue
that your current patch will take away your 4 streams from the general
pool on every controller that supports streams, which isn't optimal.

The only thing it'll change in the patch is the removal of
nvme_streams_allocate(), which is 20 lines of code. So I don't
think it'll simplify things a lot. The patch is already very simple.
But if we don't need to allocate the streams, then of course it should
just go.

quoted

+	streamid = (req->cmd_flags & REQ_WRITE_LIFE_MASK)
+			>> __REQ_WRITE_HINT_SHIFT;

Can we add a helper to convert the REQ_* flags back to the hint next
to where we have the helper to do the reverse one?

OK, will od.

quoted

+	if (streamid == WRITE_LIFE_NONE)
+		return;
+
+	/* for now just round-robin, do something more clever later */
+	if (streamid > ctrl->nr_streams)
+		streamid = (streamid % ctrl->nr_streams) + 1;

I thought you were going to fix that to do more intelligent collapsing?

Sure, if you want me to do that now, I can. I propose:

- With 4 streams, direct mapping.
- With 3 streams, collapse two longest life times.
- With 2 streams, collapse short+medium, and long+extreme.
- With 1 stream, don't use streams.

We could potentially still use streams with just 1 stream, since we
have the default of no stream as well. But I don't think it makes
sense at that point.

quoted

-	ns->queue->limits.discard_granularity = logical_block_size;
+	if (ctrl->nr_streams && ns->sws && ns->sgs) {
+		unsigned int sz = logical_block_size * ns->sws * ns->sgs;
+
+		ns->queue->limits.discard_alignment = sz;

I don't think this is correct:

"Data that is aligned to and in multiples of the Stream Write Size (SWS)
 provides optimal performance of the write commands to the controller.
 The Stream Granularity Size indicates the size of the media that is prepared
 as a unit for future allocation for write commands and is a multiple of the
 Stream Write Size. The controller may allocate and group together a stream
 in Stream Granularity Size (SGS) units. Refer to Figure 293."

So I think the ns->sws should go away.

The SGS value is in units of SWS, however.

quoted

+		ns->queue->limits.discard_granularity = sz;
+	} else {
+		u32 logical_block_size = queue_logical_block_size(ns->queue);

I think we already have a logical_block_size with the same value in
scope here.

Oops yes.

quoted

+	ns->sws = le32_to_cpu(s.sws);
+	ns->sgs = le16_to_cpu(s.sgs);
+
+	if (ns->sws) {
+		unsigned int bs = 1 << ns->lba_shift;
+
+		blk_queue_io_min(ns->queue, bs * ns->sws);
+		if (ns->sgs)
+			blk_queue_io_opt(ns->queue, bs * ns->sws * ns->sgs);

drop the multiplication with ns->sws here as well.

See above.

-- 
Jens Axboe