Re: [PATCH] block: enable passthrough command statistics

[Jens Axboe <axboe@kernel.dk>]

On 10/2/24 3:07 PM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Applications using the passthrough interfaces for advance protocol IO
> want to continue seeing the disk stats. These requests had been fenced
> off from this block layer feature. While the block layer doesn't
> necessarily know what a passthrough command does, we do know the data
> size and direction, which is enough to account for the command's stats.
> 
> Since tracking these has the potential to produce unexpected results,
> the passthrough stats are locked behind a new queue feature flag that
> needs to be enabled using the /sys/block/<dev>/queue/iostats attribute.

Looks good go me in terms of allowing passthrough stats, and adding the
0/1/2 for iostats (like we do for nomerge too, for example). Minor nit
below.

> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index e85941bec857b..99f438beb6c67 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -246,7 +246,6 @@ static ssize_t queue_##_name##_store(struct gendisk *disk,		\
>  
>  QUEUE_SYSFS_FEATURE(rotational, BLK_FEAT_ROTATIONAL)
>  QUEUE_SYSFS_FEATURE(add_random, BLK_FEAT_ADD_RANDOM)
> -QUEUE_SYSFS_FEATURE(iostats, BLK_FEAT_IO_STAT)
>  QUEUE_SYSFS_FEATURE(stable_writes, BLK_FEAT_STABLE_WRITES);
>  
>  #define QUEUE_SYSFS_FEATURE_SHOW(_name, _feature)			\
> @@ -272,6 +271,40 @@ static ssize_t queue_nr_zones_show(struct gendisk *disk, char *page)
>  	return queue_var_show(disk_nr_zones(disk), page);
>  }
>  
> +static ssize_t queue_iostats_show(struct gendisk *disk, char *page)
> +{
> +	return queue_var_show((bool)blk_queue_passthrough_stat(disk->queue) +
> +			      (bool)blk_queue_io_stat(disk->queue), page);
> +}
> +
> +static ssize_t queue_iostats_store(struct gendisk *disk, const char *page,
> +				   size_t count)
> +{
> +	struct queue_limits lim;
> +	unsigned long ios;
> +	ssize_t ret;
> +
> +	ret = queue_var_store(&ios, page, count);
> +	if (ret < 0)
> +		return ret;
> +
> +	lim = queue_limits_start_update(disk->queue);
> +	if (!ios)
> +		lim.features &= ~(BLK_FEAT_IO_STAT | BLK_FEAT_PASSTHROUGH_STAT);
> +	else if (ios == 2)
> +		lim.features |= BLK_FEAT_IO_STAT | BLK_FEAT_PASSTHROUGH_STAT;
> +	else if (ios == 1) {
> +		lim.features |= BLK_FEAT_IO_STAT;
> +		lim.features &= ~BLK_FEAT_PASSTHROUGH_STAT;
> +	}

Nit: use braces for all of them, if one requires it. And might be
cleaner to simply do:

	lim = queue_limits_start_update(disk->queue);
	lim.features &= ~(BLK_FEAT_IO_STAT | BLK_FEAT_PASSTHROUGH_STAT);
	if (ios == 2)
		lim.features |= BLK_FEAT_IO_STAT | BLK_FEAT_PASSTHROUGH_STAT;
	else if (ios == 1)
		lim.features |= BLK_FEAT_IO_STAT;

and then I guess the braces comment no longer applies.

-- 
Jens Axboe