* Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf")
@ 2022-05-12 22:42 ` Khazhy Kumykov
0 siblings, 0 replies; 13+ messages in thread
From: Khazhy Kumykov @ 2022-05-12 22:42 UTC (permalink / raw)
To: Tejun Heo, Jens Axboe, Christoph Hellwig; +Cc: linux-block, w.bumiller, cgroups
[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]
I can see in the latest tip, if we have devices with no statistics,
we'll print the maj:min and then nothing else, which can end up
looking weird, (e.g. like below.) I see that in older kernels, we
avoided printing the device name at all if there were no stats, and it
looks like this behavior was silently broken by 252c651a4c85
("blk-cgroup: stop using seq_get_buf"), where before we prepared the
whole line then decided at the end whether to commit it or not.
I do see a patch "blk-cgroup: always terminate io.stat lines" that
addresses this by just unconditionally printing the newline (though it
looks like that patch never landed). I'm wondering if it's worth
trying to bring back the old behavior of not printing these devices at
all if they have no statistics. (e.g. by having the "first"
stats-haver writing out the bdev name, or going back to scnprintf)
9:0 8:0 rbytes=16720896 wbytes=905216 rios=768 wios=67 dbytes=0 dios=0
1:15 1:14 1:13 1:12 1:11 1:10 1:9 1:8 1:7 1:6 1:5 1:4 1:3 1:2 1:1 1:0
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3999 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf") @ 2022-05-12 22:42 ` Khazhy Kumykov 0 siblings, 0 replies; 13+ messages in thread From: Khazhy Kumykov @ 2022-05-12 22:42 UTC (permalink / raw) To: Tejun Heo, Jens Axboe, Christoph Hellwig Cc: linux-block-u79uwXL29TY76Z2rM5mHXA, w.bumiller-YTcQvvOqK21BDgjK7y7TUQ, cgroups-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 1007 bytes --] I can see in the latest tip, if we have devices with no statistics, we'll print the maj:min and then nothing else, which can end up looking weird, (e.g. like below.) I see that in older kernels, we avoided printing the device name at all if there were no stats, and it looks like this behavior was silently broken by 252c651a4c85 ("blk-cgroup: stop using seq_get_buf"), where before we prepared the whole line then decided at the end whether to commit it or not. I do see a patch "blk-cgroup: always terminate io.stat lines" that addresses this by just unconditionally printing the newline (though it looks like that patch never landed). I'm wondering if it's worth trying to bring back the old behavior of not printing these devices at all if they have no statistics. (e.g. by having the "first" stats-haver writing out the bdev name, or going back to scnprintf) 9:0 8:0 rbytes=16720896 wbytes=905216 rios=768 wios=67 dbytes=0 dios=0 1:15 1:14 1:13 1:12 1:11 1:10 1:9 1:8 1:7 1:6 1:5 1:4 1:3 1:2 1:1 1:0 [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 3999 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf") 2022-05-12 22:42 ` Khazhy Kumykov (?) @ 2022-05-12 23:25 ` Khazhy Kumykov 2022-05-12 23:29 ` [RFC PATCH] blkcg: rewind seq_file if no stats Khazhismel Kumykov 2022-05-13 17:40 ` [RESEND][RFC " Khazhismel Kumykov -1 siblings, 2 replies; 13+ messages in thread From: Khazhy Kumykov @ 2022-05-12 23:25 UTC (permalink / raw) To: Tejun Heo, Jens Axboe, Christoph Hellwig, Al Viro Cc: linux-block, w.bumiller, cgroups [-- Attachment #1: Type: text/plain, Size: 570 bytes --] On Thu, May 12, 2022 at 3:42 PM Khazhy Kumykov <khazhy@google.com> wrote: > > all if they have no statistics. (e.g. by having the "first" > stats-haver writing out the bdev name, or going back to scnprintf) It might be cleaner (at least here where we want to conditionally write a prefix) if we were allowed to "rewind" seq_file (see following), though I'm not that familiar with seq_file to know if this is actually a good idea :) In principle, it doesn't seem too different from seq_get_buf/seq_commit, except we now have the checks in seq_printf preventing overruns [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 3999 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC PATCH] blkcg: rewind seq_file if no stats 2022-05-12 23:25 ` Khazhy Kumykov @ 2022-05-12 23:29 ` Khazhismel Kumykov 2022-05-13 16:30 ` Tejun Heo 2022-05-13 17:40 ` [RESEND][RFC " Khazhismel Kumykov 1 sibling, 1 reply; 13+ messages in thread From: Khazhismel Kumykov @ 2022-05-12 23:29 UTC (permalink / raw) To: tj, axboe, viro; +Cc: linux-block, linux-fsdevel, Khazhismel Kumykov In lieu of get_seq_buf + seq_commit, provide a way to "undo" writes if we use seq_printf Fixes: 252c651a4c85 ("blk-cgroup: stop using seq_get_buf") Signed-off-by: Khazhismel Kumykov <khazhy@google.com> --- block/blk-cgroup.c | 5 +++++ fs/seq_file.c | 14 ++++++++++++++ include/linux/seq_file.h | 2 ++ 3 files changed, 21 insertions(+) diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index 8dfe62786cd5..50043a742c48 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -909,6 +909,7 @@ static void blkcg_print_one_stat(struct blkcg_gq *blkg, struct seq_file *s) const char *dname; unsigned seq; int i; + int scookie; if (!blkg->online) return; @@ -917,6 +918,8 @@ static void blkcg_print_one_stat(struct blkcg_gq *blkg, struct seq_file *s) if (!dname) return; + scookie = seq_checkpoint(s); + seq_printf(s, "%s ", dname); do { @@ -956,6 +959,8 @@ static void blkcg_print_one_stat(struct blkcg_gq *blkg, struct seq_file *s) if (has_stats) seq_printf(s, "\n"); + else + seq_restore(s, scookie); } static int blkcg_print_stat(struct seq_file *sf, void *v) diff --git a/fs/seq_file.c b/fs/seq_file.c index 7ab8a58c29b6..c3ec6b57334e 100644 --- a/fs/seq_file.c +++ b/fs/seq_file.c @@ -408,6 +408,20 @@ void seq_printf(struct seq_file *m, const char *f, ...) } EXPORT_SYMBOL(seq_printf); +int seq_checkpoint(struct seq_file *m) +{ + return m->count; +} +EXPORT_SYMBOL(seq_checkpoint); + +void seq_restore(struct seq_file *m, int count) +{ + if (WARN_ON_ONCE(count > m->count || count > m->size)) + return; + m->count = count; +} +EXPORT_SYMBOL(seq_restore); + #ifdef CONFIG_BINARY_PRINTF void seq_bprintf(struct seq_file *m, const char *f, const u32 *binary) { diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h index 60820ab511d2..d3a05f7c2750 100644 --- a/include/linux/seq_file.h +++ b/include/linux/seq_file.h @@ -117,6 +117,8 @@ __printf(2, 0) void seq_vprintf(struct seq_file *m, const char *fmt, va_list args); __printf(2, 3) void seq_printf(struct seq_file *m, const char *fmt, ...); +int seq_checkpoint(struct seq_file *m); +void seq_restore(struct seq_file *m, int count); void seq_putc(struct seq_file *m, char c); void seq_puts(struct seq_file *m, const char *s); void seq_put_decimal_ull_width(struct seq_file *m, const char *delimiter, -- 2.36.0.550.gb090851708-goog ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] blkcg: rewind seq_file if no stats 2022-05-12 23:29 ` [RFC PATCH] blkcg: rewind seq_file if no stats Khazhismel Kumykov @ 2022-05-13 16:30 ` Tejun Heo 0 siblings, 0 replies; 13+ messages in thread From: Tejun Heo @ 2022-05-13 16:30 UTC (permalink / raw) To: Khazhismel Kumykov; +Cc: axboe, viro, linux-block, linux-fsdevel On Thu, May 12, 2022 at 04:29:07PM -0700, Khazhismel Kumykov wrote: > In lieu of get_seq_buf + seq_commit, provide a way to "undo" writes if > we use seq_printf > > Fixes: 252c651a4c85 ("blk-cgroup: stop using seq_get_buf") I don't hate it but can you please repost with Christoph Hellwig <hch@lst.de>? He might have an a lot stronger opinion on it. Thanks. tejun ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RESEND][RFC PATCH] blkcg: rewind seq_file if no stats 2022-05-12 23:25 ` Khazhy Kumykov 2022-05-12 23:29 ` [RFC PATCH] blkcg: rewind seq_file if no stats Khazhismel Kumykov @ 2022-05-13 17:40 ` Khazhismel Kumykov 2022-05-13 18:06 ` Khazhy Kumykov 2022-05-17 6:12 ` Christoph Hellwig 1 sibling, 2 replies; 13+ messages in thread From: Khazhismel Kumykov @ 2022-05-13 17:40 UTC (permalink / raw) To: Tejun Heo, Jens Axboe, Alexander Viro, Christoph Hellwig Cc: linux-block, linux-fsdevel, linux-kernel, cgroups, Khazhismel Kumykov Restores the previous behavior of only displaying devices for which we have statistics (and removes the current, broken, behavior of printing devname with no newline if no statistics) In lieu of get_seq_buf + seq_commit, provide a way to "undo" writes if we use seq_printf Fixes: 252c651a4c85 ("blk-cgroup: stop using seq_get_buf") Signed-off-by: Khazhismel Kumykov <khazhy@google.com> --- block/blk-cgroup.c | 5 +++++ fs/seq_file.c | 14 ++++++++++++++ include/linux/seq_file.h | 2 ++ 3 files changed, 21 insertions(+) diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index 8dfe62786cd5..50043a742c48 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -909,6 +909,7 @@ static void blkcg_print_one_stat(struct blkcg_gq *blkg, struct seq_file *s) const char *dname; unsigned seq; int i; + int scookie; if (!blkg->online) return; @@ -917,6 +918,8 @@ static void blkcg_print_one_stat(struct blkcg_gq *blkg, struct seq_file *s) if (!dname) return; + scookie = seq_checkpoint(s); + seq_printf(s, "%s ", dname); do { @@ -956,6 +959,8 @@ static void blkcg_print_one_stat(struct blkcg_gq *blkg, struct seq_file *s) if (has_stats) seq_printf(s, "\n"); + else + seq_restore(s, scookie); } static int blkcg_print_stat(struct seq_file *sf, void *v) diff --git a/fs/seq_file.c b/fs/seq_file.c index 7ab8a58c29b6..c3ec6b57334e 100644 --- a/fs/seq_file.c +++ b/fs/seq_file.c @@ -408,6 +408,20 @@ void seq_printf(struct seq_file *m, const char *f, ...) } EXPORT_SYMBOL(seq_printf); +int seq_checkpoint(struct seq_file *m) +{ + return m->count; +} +EXPORT_SYMBOL(seq_checkpoint); + +void seq_restore(struct seq_file *m, int count) +{ + if (WARN_ON_ONCE(count > m->count || count > m->size)) + return; + m->count = count; +} +EXPORT_SYMBOL(seq_restore); + #ifdef CONFIG_BINARY_PRINTF void seq_bprintf(struct seq_file *m, const char *f, const u32 *binary) { diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h index 60820ab511d2..d3a05f7c2750 100644 --- a/include/linux/seq_file.h +++ b/include/linux/seq_file.h @@ -117,6 +117,8 @@ __printf(2, 0) void seq_vprintf(struct seq_file *m, const char *fmt, va_list args); __printf(2, 3) void seq_printf(struct seq_file *m, const char *fmt, ...); +int seq_checkpoint(struct seq_file *m); +void seq_restore(struct seq_file *m, int count); void seq_putc(struct seq_file *m, char c); void seq_puts(struct seq_file *m, const char *s); void seq_put_decimal_ull_width(struct seq_file *m, const char *delimiter, -- 2.36.0.550.gb090851708-goog ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RESEND][RFC PATCH] blkcg: rewind seq_file if no stats @ 2022-05-13 18:06 ` Khazhy Kumykov 0 siblings, 0 replies; 13+ messages in thread From: Khazhy Kumykov @ 2022-05-13 18:06 UTC (permalink / raw) To: Tejun Heo, Jens Axboe, Alexander Viro, Christoph Hellwig Cc: linux-block, linux-fsdevel, linux-kernel, cgroups [-- Attachment #1: Type: text/plain, Size: 315 bytes --] On Fri, May 13, 2022 at 10:40 AM Khazhismel Kumykov <khazhy@google.com> wrote: > +void seq_restore(struct seq_file *m, int count) (Yes, this and elsewhere should be size_t to match m->count) > +{ > + if (WARN_ON_ONCE(count > m->count || count > m->size)) > + return; > + m->count = count; [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 3999 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RESEND][RFC PATCH] blkcg: rewind seq_file if no stats @ 2022-05-13 18:06 ` Khazhy Kumykov 0 siblings, 0 replies; 13+ messages in thread From: Khazhy Kumykov @ 2022-05-13 18:06 UTC (permalink / raw) To: Tejun Heo, Jens Axboe, Alexander Viro, Christoph Hellwig Cc: linux-block-u79uwXL29TY76Z2rM5mHXA, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 344 bytes --] On Fri, May 13, 2022 at 10:40 AM Khazhismel Kumykov <khazhy-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote: > +void seq_restore(struct seq_file *m, int count) (Yes, this and elsewhere should be size_t to match m->count) > +{ > + if (WARN_ON_ONCE(count > m->count || count > m->size)) > + return; > + m->count = count; [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 3999 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RESEND][RFC PATCH] blkcg: rewind seq_file if no stats 2022-05-13 17:40 ` [RESEND][RFC " Khazhismel Kumykov 2022-05-13 18:06 ` Khazhy Kumykov @ 2022-05-17 6:12 ` Christoph Hellwig 2022-05-17 6:27 ` Khazhy Kumykov 1 sibling, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2022-05-17 6:12 UTC (permalink / raw) To: Khazhismel Kumykov Cc: Tejun Heo, Jens Axboe, Alexander Viro, Christoph Hellwig, linux-block, linux-fsdevel, linux-kernel, cgroups On Fri, May 13, 2022 at 10:40:30AM -0700, Khazhismel Kumykov wrote: > Restores the previous behavior of only displaying devices for which we > have statistics (and removes the current, broken, behavior of printing > devname with no newline if no statistics) > > In lieu of get_seq_buf + seq_commit, provide a way to "undo" writes if > we use seq_printf I have to say I much prefer the simpler fix from Wolfgang. But if we want to go down this route it needs to split into a patch for the seq_file infrastruture and a separate one for the blk-cgroup code. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RESEND][RFC PATCH] blkcg: rewind seq_file if no stats @ 2022-05-17 6:27 ` Khazhy Kumykov 0 siblings, 0 replies; 13+ messages in thread From: Khazhy Kumykov @ 2022-05-17 6:27 UTC (permalink / raw) To: Christoph Hellwig Cc: Tejun Heo, Jens Axboe, Alexander Viro, linux-block, linux-fsdevel, linux-kernel, cgroups [-- Attachment #1: Type: text/plain, Size: 882 bytes --] On Mon, May 16, 2022 at 11:12 PM Christoph Hellwig <hch@lst.de> wrote: > > On Fri, May 13, 2022 at 10:40:30AM -0700, Khazhismel Kumykov wrote: > > Restores the previous behavior of only displaying devices for which we > > have statistics (and removes the current, broken, behavior of printing > > devname with no newline if no statistics) > > > > In lieu of get_seq_buf + seq_commit, provide a way to "undo" writes if > > we use seq_printf > > I have to say I much prefer the simpler fix from Wolfgang. But Wolfgang's fix still results in a change in behavior, and in my opinion a worse behavior - we don't need to print out the device names if there are no stats. > if we want to go down this route it needs to split into a patch > for the seq_file infrastruture and a separate one for the blk-cgroup > code. Of course - I can spin this into a series if we think it's worthwhile [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 3999 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RESEND][RFC PATCH] blkcg: rewind seq_file if no stats @ 2022-05-17 6:27 ` Khazhy Kumykov 0 siblings, 0 replies; 13+ messages in thread From: Khazhy Kumykov @ 2022-05-17 6:27 UTC (permalink / raw) To: Christoph Hellwig Cc: Tejun Heo, Jens Axboe, Alexander Viro, linux-block-u79uwXL29TY76Z2rM5mHXA, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 904 bytes --] On Mon, May 16, 2022 at 11:12 PM Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote: > > On Fri, May 13, 2022 at 10:40:30AM -0700, Khazhismel Kumykov wrote: > > Restores the previous behavior of only displaying devices for which we > > have statistics (and removes the current, broken, behavior of printing > > devname with no newline if no statistics) > > > > In lieu of get_seq_buf + seq_commit, provide a way to "undo" writes if > > we use seq_printf > > I have to say I much prefer the simpler fix from Wolfgang. But Wolfgang's fix still results in a change in behavior, and in my opinion a worse behavior - we don't need to print out the device names if there are no stats. > if we want to go down this route it needs to split into a patch > for the seq_file infrastruture and a separate one for the blk-cgroup > code. Of course - I can spin this into a series if we think it's worthwhile [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 3999 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf") @ 2022-05-17 6:11 ` Christoph Hellwig 0 siblings, 0 replies; 13+ messages in thread From: Christoph Hellwig @ 2022-05-17 6:11 UTC (permalink / raw) To: Khazhy Kumykov Cc: Tejun Heo, Jens Axboe, Christoph Hellwig, linux-block, w.bumiller, cgroups On Thu, May 12, 2022 at 03:42:54PM -0700, Khazhy Kumykov wrote: > I can see in the latest tip, if we have devices with no statistics, > we'll print the maj:min and then nothing else, which can end up > looking weird, (e.g. like below.) I see that in older kernels, we > avoided printing the device name at all if there were no stats, and it > looks like this behavior was silently broken by 252c651a4c85 > ("blk-cgroup: stop using seq_get_buf"), where before we prepared the > whole line then decided at the end whether to commit it or not. > > I do see a patch "blk-cgroup: always terminate io.stat lines" that > addresses this by just unconditionally printing the newline (though it > looks like that patch never landed). It should really go in, I just sent a ping for it. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf") @ 2022-05-17 6:11 ` Christoph Hellwig 0 siblings, 0 replies; 13+ messages in thread From: Christoph Hellwig @ 2022-05-17 6:11 UTC (permalink / raw) To: Khazhy Kumykov Cc: Tejun Heo, Jens Axboe, Christoph Hellwig, linux-block-u79uwXL29TY76Z2rM5mHXA, w.bumiller-YTcQvvOqK21BDgjK7y7TUQ, cgroups-u79uwXL29TY76Z2rM5mHXA On Thu, May 12, 2022 at 03:42:54PM -0700, Khazhy Kumykov wrote: > I can see in the latest tip, if we have devices with no statistics, > we'll print the maj:min and then nothing else, which can end up > looking weird, (e.g. like below.) I see that in older kernels, we > avoided printing the device name at all if there were no stats, and it > looks like this behavior was silently broken by 252c651a4c85 > ("blk-cgroup: stop using seq_get_buf"), where before we prepared the > whole line then decided at the end whether to commit it or not. > > I do see a patch "blk-cgroup: always terminate io.stat lines" that > addresses this by just unconditionally printing the newline (though it > looks like that patch never landed). It should really go in, I just sent a ping for it. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-05-17 6:27 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-12 22:42 Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf") Khazhy Kumykov
2022-05-12 22:42 ` Khazhy Kumykov
2022-05-12 23:25 ` Khazhy Kumykov
2022-05-12 23:29 ` [RFC PATCH] blkcg: rewind seq_file if no stats Khazhismel Kumykov
2022-05-13 16:30 ` Tejun Heo
2022-05-13 17:40 ` [RESEND][RFC " Khazhismel Kumykov
2022-05-13 18:06 ` Khazhy Kumykov
2022-05-13 18:06 ` Khazhy Kumykov
2022-05-17 6:12 ` Christoph Hellwig
2022-05-17 6:27 ` Khazhy Kumykov
2022-05-17 6:27 ` Khazhy Kumykov
2022-05-17 6:11 ` Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf") Christoph Hellwig
2022-05-17 6:11 ` Christoph Hellwig
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.