Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf")

Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf")

[Khazhy Kumykov]

[-- 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 --]

Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf")

[Khazhy Kumykov]

[-- 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 --]

Re: Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf")

[Khazhy Kumykov]

[-- 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 --]

[RFC PATCH] blkcg: rewind seq_file if no stats

[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

Re: [RFC PATCH] blkcg: rewind seq_file if no stats

[Tejun Heo]

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

[RESEND][RFC PATCH] blkcg: rewind seq_file if no stats

[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

Re: [RESEND][RFC PATCH] blkcg: rewind seq_file if no stats

[Khazhy Kumykov]

[-- 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 --]

Re: [RESEND][RFC PATCH] blkcg: rewind seq_file if no stats

[Khazhy Kumykov]

[-- 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 --]

Re: Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf")

[Christoph Hellwig]

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.

Re: Issue with 252c651a4c85 ("blk-cgroup: stop using seq_get_buf")

[Christoph Hellwig]

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.

Re: [RESEND][RFC PATCH] blkcg: rewind seq_file if no stats

[Christoph Hellwig]

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.

Re: [RESEND][RFC PATCH] blkcg: rewind seq_file if no stats

[Khazhy Kumykov]

[-- 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 --]

Re: [RESEND][RFC PATCH] blkcg: rewind seq_file if no stats

[Khazhy Kumykov]

[-- 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 --]