+}
+
static struct ftrace_ops pstore_ftrace_ops __read_mostly = {
.func = pstore_ftrace_call,
};
@@ -131,8 +146,16 @@ MODULE_PARM_DESC(record_ftrace,
void pstore_register_ftrace(void)
{
- if (!psinfo->write)
- return;
+ struct pstore_info_list *entry;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
+ if (!entry->psi->write) {
+ rcu_read_unlock();
+ return;
+ }
+ rcu_read_unlock();
pstore_ftrace_dir = debugfs_create_dir("pstore", NULL);
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index d0d9bfdad30c..bee71c7da995 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -285,7 +285,7 @@ static const struct super_operations pstore_ops = {
.show_options = pstore_show_options,
};
-static struct dentry *psinfo_lock_root(void)
+static struct dentry *psinfo_lock_root(struct pstore_info *psinfo)
{
struct dentry *root;
@@ -309,7 +309,7 @@ int pstore_put_backend_records(struct pstore_info *psi)
struct dentry *root;
int rc = 0;
- root = psinfo_lock_root();
+ root = psinfo_lock_root(psi);
if (!root)
return 0;
@@ -398,21 +398,22 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
* when we are re-scanning the backing store looking to add new
* error records.
*/
-void pstore_get_records(int quiet)
+void pstore_get_records(struct pstore_info *psi, int quiet)
{
struct dentry *root;
- root = psinfo_lock_root();
+ root = psinfo_lock_root(psi);
if (!root)
return;
- pstore_get_backend_records(psinfo, root, quiet);
+ pstore_get_backend_records(psi, root, quiet);
inode_unlock(d_inode(root));
}
static int pstore_fill_super(struct super_block *sb, void *data, int silent)
{
struct inode *inode;
+ struct pstore_info_list *entry;
sb->s_maxbytes = MAX_LFS_FILESIZE;
sb->s_blocksize = PAGE_SIZE;
@@ -437,7 +438,13 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
scoped_guard(mutex, &pstore_sb_lock)
pstore_sb = sb;
- pstore_get_records(0);
+ if (!psback)
+ return 0;
+
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list)
+ pstore_get_records(entry->psi, 0);
+ mutex_unlock(&psback_lock);
return 0;
}
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index 801d6c0b170c..4b1c7ba27052 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -33,10 +33,10 @@ static inline void pstore_register_pmsg(void) {}
static inline void pstore_unregister_pmsg(void) {}
#endif
-extern struct pstore_info *psinfo;
+extern struct pstore_backends *psback;
extern void pstore_set_kmsg_bytes(int);
-extern void pstore_get_records(int);
+extern void pstore_get_records(struct pstore_info *psi, int quiet);
extern void pstore_get_backend_records(struct pstore_info *psi,
struct dentry *root, int quiet);
extern int pstore_put_backend_records(struct pstore_info *psi);
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 03425928d2fb..432a41852a07 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -62,12 +62,12 @@ static void pstore_dowork(struct work_struct *);
static DECLARE_WORK(pstore_work, pstore_dowork);
/*
- * psinfo_lock protects "psinfo" during calls to
+ * psback_lock protects "psback" during calls to
* pstore_register(), pstore_unregister(), and
* the filesystem mount/unmount routines.
*/
-static DEFINE_MUTEX(psinfo_lock);
-struct pstore_info *psinfo;
+DEFINE_MUTEX(psback_lock);
+struct pstore_backends *psback;
static char *backend;
module_param(backend, charp, 0444);
@@ -104,7 +104,7 @@ static void *compress_workspace;
*/
#define DMESG_COMP_PERCENT 60
-static char *big_oops_buf;
+static char *big_oops_buf[PSTORE_BACKEND_NUM];
I think big_oops_buf should live in the pstore_info_list struct, then
no indexes are needed to be tracked.
static size_t max_compressed_size;
void pstore_set_kmsg_bytes(int bytes)
@@ -201,7 +201,7 @@ static int pstore_compress(const void *in, void *out,
return zstream.total_out;
}
-static void allocate_buf_for_compression(void)
+static void allocate_buf_for_compression(struct pstore_info *psinfo, int pos)
{
size_t compressed_size;
char *buf;
@@ -241,21 +241,21 @@ static void allocate_buf_for_compression(void)
}
/* A non-NULL big_oops_buf indicates compression is available. */
- big_oops_buf = buf;
+ big_oops_buf[pos] = buf;
e.g.: entry->big_oops_buf = buf;
max_compressed_size = compressed_size;
And should this be included as well, or are we just constantly setting
this to the same value?
pr_info("Using crash dump compression: %s\n", compress);
}
-static void free_buf_for_compression(void)
+static void free_buf_for_compression(int pos)
Instead of "pos" this would take the ps_info.
{
if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && compress_workspace) {
vfree(compress_workspace);
compress_workspace = NULL;
}
- kvfree(big_oops_buf);
- big_oops_buf = NULL;
+ kvfree(big_oops_buf[pos]);
+ big_oops_buf[pos] = NULL;
max_compressed_size = 0;
}
@@ -274,8 +274,9 @@ void pstore_record_init(struct pstore_record *record,
* callback from kmsg_dump. Save as much as we can (up to kmsg_bytes) from the
* end of the buffer.
*/
-static void pstore_dump(struct kmsg_dumper *dumper,
- enum kmsg_dump_reason reason)
+static void pstore_do_dump(struct kmsg_dumper *dumper,
+ enum kmsg_dump_reason reason,
+ struct pstore_info *psinfo, int pos)
{
struct kmsg_dump_iter iter;
unsigned long total = 0;
@@ -315,7 +316,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
record.part = part;
record.buf = psinfo->buf;
- dst = big_oops_buf ?: psinfo->buf;
+ dst = big_oops_buf[pos] ?: psinfo->buf;
dst_size = max_compressed_size ?: psinfo->bufsize;
/* Write dump header. */
@@ -328,7 +329,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
dst_size, &dump_size))
break;
- if (big_oops_buf) {
+ if (big_oops_buf[pos]) {
zipped_len = pstore_compress(dst, psinfo->buf,
header_size + dump_size,
psinfo->bufsize);
@@ -372,6 +373,19 @@ static void pstore_dump(struct kmsg_dumper *dumper,
}
}
+static void pstore_dump(struct kmsg_dumper *dumper,
+ enum kmsg_dump_reason reason)
+{
+ struct pstore_info_list *entry;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_DMESG)
+ pstore_do_dump(dumper, reason,
+ entry->psi, entry->index);
Then no need for index here.
+ rcu_read_unlock();
+}
+
static struct kmsg_dumper pstore_dumper = {
.dump = pstore_dump,
};
@@ -390,13 +404,11 @@ static void pstore_unregister_kmsg(void)
}
#ifdef CONFIG_PSTORE_CONSOLE
-static void pstore_console_write(struct console *con, const char *s, unsigned c)
+static void pstore_console_do_write(struct console *con, const char *s,
+ unsigned c, struct pstore_info *psinfo)
{
struct pstore_record record;
- if (!c)
- return;
-
pstore_record_init(&record, psinfo);
record.type = PSTORE_TYPE_CONSOLE;
@@ -405,6 +417,21 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
psinfo->write(&record);
}
+static void pstore_console_write(struct console *con, const char *s,
+ unsigned int c)
+{
+ struct pstore_info_list *entry;
+
+ if (!c)
+ return;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_CONSOLE)
+ pstore_console_do_write(con, s, c, entry->psi);
+ rcu_read_unlock();
+}
+
static struct console pstore_console = {
.write = pstore_console_write,
.index = -1,
@@ -413,7 +440,7 @@ static struct console pstore_console = {
static void pstore_register_console(void)
{
/* Show which backend is going to get console writes. */
- strscpy(pstore_console.name, psinfo->name,
+ strscpy(pstore_console.name, "pstore console",
sizeof(pstore_console.name));
A nice-to-have for this might be to include all the backend names
included, but that is probably overkill, so this is fine.
/*
* Always initialize flags here since prior unregister_console()
@@ -464,12 +491,15 @@ static int pstore_write_user_compat(struct pstore_record *record,
*/
int pstore_register(struct pstore_info *psi)
{
+ struct pstore_info_list *entry;
+ struct pstore_info_list *newpsi;
char *new_backend;
- if (backend && strcmp(backend, psi->name)) {
- pr_warn("backend '%s' already in use: ignoring '%s'\n",
- backend, psi->name);
- return -EBUSY;
+ /* backend has to be enabled for going on registering */
+ if (backend && !strstr(backend, psi->name) &&
I think I'd like a helper to do the "is this name in the list?", as that
would be more robust. e.g. if we ever had a "ram" backend and a "cram"
backend, and someone used "backend=cram", suddenly both "cram" and "ram"
match in the code above.
+ strcmp(backend, "all")) {
style preference, can you make this strcmp be:
strcmp(backend, "all") != 0
it seems redundant, but it helps readers remember that "strcmp() == 0"
is "matches".
+ pr_warn("backend '%s' not enabled\n", psi->name);
nit, I think a more clear error might be something like:
"backend '%s' ignored: not present in pstore.backend=...\n"
which gives people a little hint about what's wrong.
+ return -EINVAL;
}
/* Sanity check flags. */
@@ -486,79 +516,118 @@ int pstore_register(struct pstore_info *psi)
return -EINVAL;
}
- new_backend = kstrdup(psi->name, GFP_KERNEL);
- if (!new_backend)
- return -ENOMEM;
-
- mutex_lock(&psinfo_lock);
- if (psinfo) {
- pr_warn("backend '%s' already loaded: ignoring '%s'\n",
- psinfo->name, psi->name);
- mutex_unlock(&psinfo_lock);
- kfree(new_backend);
- return -EBUSY;
+ mutex_lock(&psback_lock);
+
+ /*
+ * If no backend specified, first come first served to
+ * maintain backward compatibility
+ */
+ if (!backend) {
+ pr_warn("no backend enabled, registering backend '%s'\n",
+ psi->name);
I'd move this to after the if below, so you can't get this message if it
were going to just immediately fail. And a nit on language. I think this
would be more clear:
"pstore.backend=... not specified; registering first available: '%s'\n"
+ new_backend = kstrdup(psi->name, GFP_KERNEL);
+ if (!new_backend) {
+ mutex_unlock(&psback_lock);
+ return -ENOMEM;
+ }
+ }
+
+ if (psback) {
+ if (psback->flag == PSTORE_LIST_FULL) {
+ pr_warn("backend registration space is used up: "
+ "ignoring '%s'\n", psi->name);
+ mutex_unlock(&psback_lock);
+ return -EBUSY;
+ }
All of this flag/index stuff can go away with big_oops_buf moved.
+ list_for_each_entry(entry, &psback->list_entry, list) {
+ if (strcmp(entry->psi->name, psi->name) == 0) {
+ pr_warn("backend '%s' already loaded\n",
+ psi->name);
"backend '%s' already loaded; not loading it again\n"
+ mutex_unlock(&psback_lock);
+ return -EPERM;
+ }
+ }
+ } else {
+ psback = kzalloc(sizeof(*psback), GFP_KERNEL);
+ INIT_LIST_HEAD(&psback->list_entry);
}
if (!psi->write_user)
psi->write_user = pstore_write_user_compat;
- psinfo = psi;
- mutex_init(&psinfo->read_mutex);
- spin_lock_init(&psinfo->buf_lock);
+ newpsi = kzalloc(sizeof(*newpsi), GFP_KERNEL);
+ newpsi->psi = psi;
+ newpsi->index = ffz(psback->flag);
+ psback->flag |= (1 << newpsi->index);
+
+ mutex_init(&psi->read_mutex);
+ spin_lock_init(&psi->buf_lock);
+
+ if (psi->flags & PSTORE_FLAGS_DMESG &&
+ !psback->front_cnt[PSTORE_TYPE_DMESG])
+ allocate_buf_for_compression(psi, newpsi->index);
Can't we just ignore the front_cnt? The only reason to have
big_oops_buf is if the backend supports PSTORE_FLAGS_DMESG, yes?
- if (psi->flags & PSTORE_FLAGS_DMESG)
- allocate_buf_for_compression();
+ pstore_get_records(psi, 0);
- pstore_get_records(0);
+ list_add_rcu(&newpsi->list, &psback->list_entry);
- if (psi->flags & PSTORE_FLAGS_DMESG) {
- pstore_dumper.max_reason = psinfo->max_reason;
+ if (psi->flags & PSTORE_FLAGS_DMESG &&
+ !psback->front_cnt[PSTORE_TYPE_DMESG]++) {
+ pstore_dumper.max_reason = psi->max_reason;
pstore_register_kmsg();
}
Hmm... does this mean we need multiple pstore_dump instances? (i.e. like
big_oops_buf) The backend configures the max_reason, so we'll need to
register multiple kmsg handlers to have different max_reasons. Right now
this just means "last registered backend wins" which would be
surprising.
Let's add a pstore_dumper instance to pstore_info_list, along with
big_oops_buf. All of these globals are really per-backend instances now.
- if (psi->flags & PSTORE_FLAGS_CONSOLE)
+ if (psi->flags & PSTORE_FLAGS_CONSOLE
+ && !psback->front_cnt[PSTORE_TYPE_CONSOLE]++)
pstore_register_console();
- if (psi->flags & PSTORE_FLAGS_FTRACE)
+ if (psi->flags & PSTORE_FLAGS_FTRACE &&
+ !psback->front_cnt[PSTORE_TYPE_FTRACE]++)
pstore_register_ftrace();
- if (psi->flags & PSTORE_FLAGS_PMSG)
+ if (psi->flags & PSTORE_FLAGS_PMSG &&
+ !psback->front_cnt[PSTORE_TYPE_PMSG]++)
pstore_register_pmsg();
/* Start watching for new records, if desired. */
pstore_timer_kick();
/*
- * Update the module parameter backend, so it is visible
+ * When module parameter backend is not specified,
+ * update the module parameter backend, so it is visible
* through /sys/module/pstore/parameters/backend
*/
- backend = new_backend;
+ if (!backend)
+ backend = new_backend;
pr_info("Registered %s as persistent store backend\n", psi->name);
- mutex_unlock(&psinfo_lock);
+ mutex_unlock(&psback_lock);
return 0;
}
EXPORT_SYMBOL_GPL(pstore_register);
void pstore_unregister(struct pstore_info *psi)
{
+ struct pstore_info_list *entry;
/* It's okay to unregister nothing. */
if (!psi)
return;
- mutex_lock(&psinfo_lock);
-
- /* Only one backend can be registered at a time. */
- if (WARN_ON(psi != psinfo)) {
- mutex_unlock(&psinfo_lock);
+ /* Can not unregister an unenabled backend*/
+ if (WARN_ON(!strstr(backend, psi->name) && strcmp(backend, "all")))
return;
- }
+
+ mutex_lock(&psback_lock);
/* Unregister all callbacks. */
- if (psi->flags & PSTORE_FLAGS_PMSG)
+ if (psi->flags & PSTORE_FLAGS_PMSG &&
+ !--psback->front_cnt[PSTORE_TYPE_PMSG])
pstore_unregister_pmsg();
- if (psi->flags & PSTORE_FLAGS_FTRACE)
+ if (psi->flags & PSTORE_FLAGS_FTRACE &&
+ !--psback->front_cnt[PSTORE_TYPE_FTRACE])
pstore_unregister_ftrace();
- if (psi->flags & PSTORE_FLAGS_CONSOLE)
+ if (psi->flags & PSTORE_FLAGS_CONSOLE &&
+ !--psback->front_cnt[PSTORE_TYPE_CONSOLE])
pstore_unregister_console();
- if (psi->flags & PSTORE_FLAGS_DMESG)
+ if (psi->flags & PSTORE_FLAGS_DMESG &&
+ !--psback->front_cnt[PSTORE_TYPE_DMESG])
pstore_unregister_kmsg();
/* Stop timer and make sure all work has finished. */
@@ -568,19 +637,30 @@ void pstore_unregister(struct pstore_info *psi)
/* Remove all backend records from filesystem tree. */
pstore_put_backend_records(psi);
- free_buf_for_compression();
+ list_for_each_entry(entry, &psback->list_entry, list) {
Since you're using list_del below, doesn't this above need to use the
_safe version of the list walker?
+ if (entry->psi == psi) {
+ list_del_rcu(&entry->list);
+ psback->flag ^= 1 << entry->index;
+ synchronize_rcu();
Do we want the synchronize here? I think it might be better to do the
list removal first, and then outside of the psback_lock we can do it and
finalize the cleanup.
+ free_buf_for_compression(entry->index);
+ kfree(entry);
+ break;
+ }
+ }
- psinfo = NULL;
- kfree(backend);
- backend = NULL;
+ if (psback->flag == PSOTRE_LIST_EMPTY) {
+ kfree(psback);
+ psback = NULL;
+ }
pr_info("Unregistered %s as persistent store backend\n", psi->name);
As this might help with debugging, can you move this pr_info to near the
top of the function (and say "Unregistering '%s' ...") so any weird
problems reported to dmesg will show up _after_ this report of intent.
:)
- mutex_unlock(&psinfo_lock);
+ mutex_unlock(&psback_lock);
}
EXPORT_SYMBOL_GPL(pstore_unregister);
static void decompress_record(struct pstore_record *record,
- struct z_stream_s *zstream)
+ struct z_stream_s *zstream,
+ struct pstore_info *psinfo)
{
int ret;
int unzipped_len;
@@ -697,7 +777,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
break;
}
- decompress_record(record, &zstream);
+ decompress_record(record, &zstream, psi);
rc = pstore_mkfile(root, record);
if (rc) {
/* pstore_mkfile() did not take record, so free it. */
@@ -729,7 +809,12 @@ void pstore_get_backend_records(struct pstore_info *psi,
static void pstore_dowork(struct work_struct *work)
{
- pstore_get_records(1);
+ struct pstore_info_list *entry;
+
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list)
+ pstore_get_records(entry->psi, 1);
+ mutex_unlock(&psback_lock);
}
static void pstore_timefunc(struct timer_list *unused)
@@ -745,11 +830,15 @@ static void pstore_timefunc(struct timer_list *unused)
static int __init pstore_init(void)
{
int ret;
+ struct pstore_info_list *entry;
ret = pstore_init_fs();
- if (ret)
- free_buf_for_compression();
-
+ if (ret) {
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list)
+ free_buf_for_compression(entry->index);
+ mutex_unlock(&psback_lock);
At init, we'll have no list, yes?
