On 10/2/24 3:07 PM, Keith Busch wrote:
> From: Keith Busch <kbusch@xxxxxxxxxx>
>
> 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
