Re: [PATCHv2] block: enable passthrough command statistics

[Keith Busch <kbusch@kernel.org>]

On Fri, Oct 04, 2024 at 07:38:28AM +0200, Christoph Hellwig wrote: > 
> Missing. At the end of the sentence.  But even then this doesn't
> explain why not accouting these requests is fine.
> 
>  - requests without a bio are all those that don't transfer data
>  - requests with a bio but not bdev are almost all passthrough requests
>    as far as I can tell, with the only exception of nvme I/O command
>    passthrough.
> 
> I.e. what we have here is a special casing for nvme I/O commands.  Maybe
> that's fine, but the comments and commit log should leave a clearly
> visible trace of that and not confuse the hell out of people trying to
> understand the logic later.

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.
 
> > +	/*
> > +	 * 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.

> > +	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.