* [PATCH 1/2] pstore: Enable compression on normal path (again)
@ 2016-05-18 12:00 Namhyung Kim
2016-05-18 12:00 ` [PATCH 2/2] pstore: Cleanup pstore_dump() Namhyung Kim
2016-05-20 23:03 ` [PATCH 1/2] pstore: Enable compression on normal path (again) Kees Cook
0 siblings, 2 replies; 4+ messages in thread
From: Namhyung Kim @ 2016-05-18 12:00 UTC (permalink / raw)
To: Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck; +Cc: LKML
The commit f0e2efcfd2717 ("pstore: do not use message compression
without lock") added a check to 'is_locked' to avoid breakage in
concurrent accesses. But it has a side-effect of disabling compression
on normal path since 'is_locked' variable is not set. As normal path
always takes the lock, it should be initialized to 1.
This also makes the unlock code a bit simpler.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
fs/pstore/platform.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 588461bb2dd4..04a0164a2066 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -284,7 +284,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
u64 id;
unsigned int part = 1;
unsigned long flags = 0;
- int is_locked = 0;
+ int is_locked = 1;
int ret;
why = get_reason_str(reason);
@@ -350,10 +350,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
total += total_len;
part++;
}
- if (pstore_cannot_block_path(reason)) {
- if (is_locked)
- spin_unlock_irqrestore(&psinfo->buf_lock, flags);
- } else
+ if (is_locked)
spin_unlock_irqrestore(&psinfo->buf_lock, flags);
}
--
2.8.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] pstore: Cleanup pstore_dump()
2016-05-18 12:00 [PATCH 1/2] pstore: Enable compression on normal path (again) Namhyung Kim
@ 2016-05-18 12:00 ` Namhyung Kim
2016-05-20 23:03 ` Kees Cook
2016-05-20 23:03 ` [PATCH 1/2] pstore: Enable compression on normal path (again) Kees Cook
1 sibling, 1 reply; 4+ messages in thread
From: Namhyung Kim @ 2016-05-18 12:00 UTC (permalink / raw)
To: Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck; +Cc: LKML
The code is duplicate between compression is enabled or not.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
fs/pstore/platform.c | 32 +++++++++++++-------------------
1 file changed, 13 insertions(+), 19 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 04a0164a2066..277730f6c462 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -304,19 +304,25 @@ static void pstore_dump(struct kmsg_dumper *dumper,
int hsize;
int zipped_len = -1;
size_t len;
- bool compressed;
+ bool compressed = false;
size_t total_len;
if (big_oops_buf && is_locked) {
dst = big_oops_buf;
- hsize = sprintf(dst, "%s#%d Part%u\n", why,
- oopscount, part);
- size = big_oops_buf_sz - hsize;
+ size = big_oops_buf_sz;
+ } else {
+ dst = psinfo->buf;
+ size = psinfo->bufsize;
+ }
- if (!kmsg_dump_get_buffer(dumper, true, dst + hsize,
- size, &len))
- break;
+ hsize = sprintf(dst, "%s#%d Part%u\n", why, oopscount, part);
+ size -= hsize;
+
+ if (!kmsg_dump_get_buffer(dumper, true, dst + hsize,
+ size, &len))
+ break;
+ if (big_oops_buf && is_locked) {
zipped_len = pstore_compress(dst, psinfo->buf,
hsize + len, psinfo->bufsize);
@@ -324,21 +330,9 @@ static void pstore_dump(struct kmsg_dumper *dumper,
compressed = true;
total_len = zipped_len;
} else {
- compressed = false;
total_len = copy_kmsg_to_buffer(hsize, len);
}
} else {
- dst = psinfo->buf;
- hsize = sprintf(dst, "%s#%d Part%u\n", why, oopscount,
- part);
- size = psinfo->bufsize - hsize;
- dst += hsize;
-
- if (!kmsg_dump_get_buffer(dumper, true, dst,
- size, &len))
- break;
-
- compressed = false;
total_len = hsize + len;
}
--
2.8.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] pstore: Enable compression on normal path (again)
2016-05-18 12:00 [PATCH 1/2] pstore: Enable compression on normal path (again) Namhyung Kim
2016-05-18 12:00 ` [PATCH 2/2] pstore: Cleanup pstore_dump() Namhyung Kim
@ 2016-05-20 23:03 ` Kees Cook
1 sibling, 0 replies; 4+ messages in thread
From: Kees Cook @ 2016-05-20 23:03 UTC (permalink / raw)
To: Namhyung Kim; +Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML
On Wed, May 18, 2016 at 5:00 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> The commit f0e2efcfd2717 ("pstore: do not use message compression
> without lock") added a check to 'is_locked' to avoid breakage in
> concurrent accesses. But it has a side-effect of disabling compression
> on normal path since 'is_locked' variable is not set. As normal path
> always takes the lock, it should be initialized to 1.
>
> This also makes the unlock code a bit simpler.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> fs/pstore/platform.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 588461bb2dd4..04a0164a2066 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -284,7 +284,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> u64 id;
> unsigned int part = 1;
> unsigned long flags = 0;
> - int is_locked = 0;
> + int is_locked = 1;
> int ret;
>
> why = get_reason_str(reason);
> @@ -350,10 +350,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> total += total_len;
> part++;
> }
> - if (pstore_cannot_block_path(reason)) {
> - if (is_locked)
> - spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> - } else
> + if (is_locked)
> spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> }
This makes sense, but I think I'd prefer that is_locked get set in the
"else", since that makes it a little easier to read, IMO. I'll adjust
it and add it to the tree.
Thanks!
-Kees
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] pstore: Cleanup pstore_dump()
2016-05-18 12:00 ` [PATCH 2/2] pstore: Cleanup pstore_dump() Namhyung Kim
@ 2016-05-20 23:03 ` Kees Cook
0 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2016-05-20 23:03 UTC (permalink / raw)
To: Namhyung Kim; +Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML
On Wed, May 18, 2016 at 5:00 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> The code is duplicate between compression is enabled or not.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> fs/pstore/platform.c | 32 +++++++++++++-------------------
> 1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 04a0164a2066..277730f6c462 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -304,19 +304,25 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> int hsize;
> int zipped_len = -1;
> size_t len;
> - bool compressed;
> + bool compressed = false;
> size_t total_len;
>
> if (big_oops_buf && is_locked) {
> dst = big_oops_buf;
> - hsize = sprintf(dst, "%s#%d Part%u\n", why,
> - oopscount, part);
> - size = big_oops_buf_sz - hsize;
> + size = big_oops_buf_sz;
> + } else {
> + dst = psinfo->buf;
> + size = psinfo->bufsize;
> + }
>
> - if (!kmsg_dump_get_buffer(dumper, true, dst + hsize,
> - size, &len))
> - break;
> + hsize = sprintf(dst, "%s#%d Part%u\n", why, oopscount, part);
> + size -= hsize;
> +
> + if (!kmsg_dump_get_buffer(dumper, true, dst + hsize,
> + size, &len))
> + break;
>
> + if (big_oops_buf && is_locked) {
> zipped_len = pstore_compress(dst, psinfo->buf,
> hsize + len, psinfo->bufsize);
>
> @@ -324,21 +330,9 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> compressed = true;
> total_len = zipped_len;
> } else {
> - compressed = false;
> total_len = copy_kmsg_to_buffer(hsize, len);
> }
> } else {
> - dst = psinfo->buf;
> - hsize = sprintf(dst, "%s#%d Part%u\n", why, oopscount,
> - part);
> - size = psinfo->bufsize - hsize;
> - dst += hsize;
> -
> - if (!kmsg_dump_get_buffer(dumper, true, dst,
> - size, &len))
> - break;
> -
> - compressed = false;
> total_len = hsize + len;
> }
>
> --
> 2.8.0
>
Nice cleanup, thanks! Applied.
-Kees
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-05-20 23:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-18 12:00 [PATCH 1/2] pstore: Enable compression on normal path (again) Namhyung Kim
2016-05-18 12:00 ` [PATCH 2/2] pstore: Cleanup pstore_dump() Namhyung Kim
2016-05-20 23:03 ` Kees Cook
2016-05-20 23:03 ` [PATCH 1/2] pstore: Enable compression on normal path (again) Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).