All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Keith Busch <kbusch@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>, Keith Busch <kbusch@meta.com>,
	linux-block@vger.kernel.org, axboe@kernel.dk
Subject: Re: [PATCHv2] block: enable passthrough command statistics
Date: Mon, 7 Oct 2024 07:56:56 +0200	[thread overview]
Message-ID: <20241007055656.GA510@lst.de> (raw)
In-Reply-To: <ZwAD7RZjqpzQl43s@kbusch-mbp>

On Fri, Oct 04, 2024 at 09:04:13AM -0600, Keith Busch wrote:
> Even Jens was a little surprised to find nvme passthrough sets the bio
> bi_bdev. I didn't think it was unusual, but sounds like we are doing
> something special here.

IIRC it was added to support metadata passthrough, but I'd have to do
a little research to find the details.

> > > +	/*
> > > +	 * Ensuring the size is aligned to the block size prevents observing an
> > > +	 * invalid sectors stat.
> > > +	 */
> > > +	if (blk_rq_bytes(req) & (bdev_logical_block_size(bio->bi_bdev) - 1))
> > > +		return false;
> > 
> > Now this probably won't trigger anyway for the usual workload (although
> > it might for odd NVMe command sets like KV and the SLM), but I'd expect the
> > size to be rounded (probably up?) and not entirely dropped.
> 
> This prevents commands with payload sizes that are not representative of
> sector access. Examples from NVMe include Copy, Dataset Management, and
> all the Reservation commands. The transfer size of those commands are
> unlikely to be a block aligned, so it's a simple way to filter them out.
> Rounding the payload size up will produce misleading stats, so I think
> it's better if they don't get to use the feature.

True.  Please put this into the comments!

> 
> > > +	ret = queue_var_store(&ios, page, count);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	if (ios)
> > > +		blk_queue_flag_set(QUEUE_FLAG_IOSTATS_PASSTHROUGH,
> > > +				   disk->queue);
> > > +	else
> > > +		blk_queue_flag_clear(QUEUE_FLAG_IOSTATS_PASSTHROUGH,
> > > +				     disk->queue);
> > 
> > Why is this using queue flags now?  This isn't really blk-mq internal,
> > so it should be using queue_limits->flag as pointed out last round.
> 
> So many flags... The atomic limit update seemed overkill for just this
> flag, but okay.

I've been slowly working on making q->flags entirely limited to
blk-mq internal state.  We're not quite there yet, but I'd like to
keep up the direction rather than having to fix it up later.

      reply	other threads:[~2024-10-07  5:56 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-03 15:30 [PATCHv2] block: enable passthrough command statistics Keith Busch
2024-10-03 15:40 ` Jens Axboe
2024-10-03 18:09 ` Jens Axboe
2024-10-04  5:38 ` Christoph Hellwig
2024-10-04 15:04   ` Keith Busch
2024-10-07  5:56     ` Christoph Hellwig [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20241007055656.GA510@lst.de \
    --to=hch@lst.de \
    --cc=axboe@kernel.dk \
    --cc=kbusch@kernel.org \
    --cc=kbusch@meta.com \
    --cc=linux-block@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.