| Message ID | 20220317112653.1019490-2-yukuai3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | optimizations for io accounting | expand |
On 3/17/22 04:26, Yu Kuai wrote: > If io accounting is disabled, there is no point to handle such device > in diskstats_show(), and it can be confused for users because all fields > in iostat are zero while the disk is handling io. > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > block/genhd.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/block/genhd.c b/block/genhd.c > index c3b32c665aec..e5307f512185 100644 > --- a/block/genhd.c > +++ b/block/genhd.c > @@ -937,6 +937,9 @@ ssize_t part_stat_show(struct device *dev, > struct disk_stats stat; > unsigned int inflight; > > + if (!blk_queue_io_stat(q)) > + return sprintf(buf, "io accounting is disabled\n"); > + Hmm ... the above looks sub-optimal to me. Has it been considered to return an error code instead or even better to hide the stat attribute if I/O accounting is disabled? The latter can be achieved by modifying disk_visible(). Thanks, Bart.
在 2022/03/17 22:06, Bart Van Assche 写道: > On 3/17/22 04:26, Yu Kuai wrote: >> If io accounting is disabled, there is no point to handle such device >> in diskstats_show(), and it can be confused for users because all fields >> in iostat are zero while the disk is handling io. >> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> --- >> block/genhd.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/block/genhd.c b/block/genhd.c >> index c3b32c665aec..e5307f512185 100644 >> --- a/block/genhd.c >> +++ b/block/genhd.c >> @@ -937,6 +937,9 @@ ssize_t part_stat_show(struct device *dev, >> struct disk_stats stat; >> unsigned int inflight; >> + if (!blk_queue_io_stat(q)) >> + return sprintf(buf, "io accounting is disabled\n"); >> + > > Hmm ... the above looks sub-optimal to me. Has it been considered to > return an error code instead or even better to hide the stat attribute > if I/O accounting is disabled? The latter can be achieved by modifying > disk_visible(). Hi, It's right this way is much better, i'll hide the 'stat' in next iteration. BTW, do you have any suggestion about patch 3? Thanks, Kuai
On 3/17/22 18:36, yukuai (C) wrote: > 在 2022/03/17 22:06, Bart Van Assche 写道: >> On 3/17/22 04:26, Yu Kuai wrote: >>> If io accounting is disabled, there is no point to handle such device >>> in diskstats_show(), and it can be confused for users because all fields >>> in iostat are zero while the disk is handling io. >>> >>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>> --- >>> block/genhd.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/block/genhd.c b/block/genhd.c >>> index c3b32c665aec..e5307f512185 100644 >>> --- a/block/genhd.c >>> +++ b/block/genhd.c >>> @@ -937,6 +937,9 @@ ssize_t part_stat_show(struct device *dev, >>> struct disk_stats stat; >>> unsigned int inflight; >>> + if (!blk_queue_io_stat(q)) >>> + return sprintf(buf, "io accounting is disabled\n"); >>> + >> >> Hmm ... the above looks sub-optimal to me. Has it been considered to >> return an error code instead or even better to hide the stat attribute >> if I/O accounting is disabled? The latter can be achieved by modifying >> disk_visible(). > > It's right this way is much better, i'll hide the 'stat' in next > iteration. Please note that modifying disk_visible() only is not sufficient. sysfs_update_group() needs to be called to trigger a call to disk_visible() if a variable has changed that affects the return value of disk_visible(). > BTW, do you have any suggestion about patch 3? I need more time to analyze that patch. Thanks, Bart.
在 2022/03/18 11:10, Bart Van Assche 写道: > On 3/17/22 18:36, yukuai (C) wrote: >> 在 2022/03/17 22:06, Bart Van Assche 写道: >>> On 3/17/22 04:26, Yu Kuai wrote: >>>> If io accounting is disabled, there is no point to handle such device >>>> in diskstats_show(), and it can be confused for users because all >>>> fields >>>> in iostat are zero while the disk is handling io. >>>> >>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>>> --- >>>> block/genhd.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/block/genhd.c b/block/genhd.c >>>> index c3b32c665aec..e5307f512185 100644 >>>> --- a/block/genhd.c >>>> +++ b/block/genhd.c >>>> @@ -937,6 +937,9 @@ ssize_t part_stat_show(struct device *dev, >>>> struct disk_stats stat; >>>> unsigned int inflight; >>>> + if (!blk_queue_io_stat(q)) >>>> + return sprintf(buf, "io accounting is disabled\n"); >>>> + >>> >>> Hmm ... the above looks sub-optimal to me. Has it been considered to >>> return an error code instead or even better to hide the stat >>> attribute if I/O accounting is disabled? The latter can be achieved >>> by modifying disk_visible(). > > >> It's right this way is much better, i'll hide the 'stat' in next >> iteration. > > Please note that modifying disk_visible() only is not sufficient. > sysfs_update_group() needs to be called to trigger a call to > disk_visible() if a variable has changed that affects the return value > of disk_visible(). Hi, Thanks for your advice, I wonder can the queue flag 'QUEUE_FLAG_IO_STAT' change after the disk is created? I don't see it can change anywhere. In this case, perhaps we don't need to consider calling sysfs_update_group(). > >> BTW, do you have any suggestion about patch 3? > > I need more time to analyze that patch. Thanks for sending time on the patch. Kuai
On Thu, Mar 17, 2022 at 07:26:51PM +0800, Yu Kuai wrote: > If io accounting is disabled, there is no point to handle such device > in diskstats_show(), and it can be confused for users because all fields > in iostat are zero while the disk is handling io. But changing the output will break existing users looking at it.
diff --git a/block/genhd.c b/block/genhd.c index c3b32c665aec..e5307f512185 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -937,6 +937,9 @@ ssize_t part_stat_show(struct device *dev, struct disk_stats stat; unsigned int inflight; + if (!blk_queue_io_stat(q)) + return sprintf(buf, "io accounting is disabled\n"); + if (queue_is_mq(q)) inflight = blk_mq_in_flight(q, bdev); else @@ -1207,6 +1210,8 @@ static int diskstats_show(struct seq_file *seqf, void *v) xa_for_each(&gp->part_tbl, idx, hd) { if (bdev_is_partition(hd) && !bdev_nr_sectors(hd)) continue; + if (!blk_queue_io_stat(gp->queue)) + continue; if (queue_is_mq(gp->queue)) inflight = blk_mq_in_flight(gp->queue, hd); else
If io accounting is disabled, there is no point to handle such device in diskstats_show(), and it can be confused for users because all fields in iostat are zero while the disk is handling io. Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- block/genhd.c | 5 +++++ 1 file changed, 5 insertions(+)