2013-03-01 17:15:13

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH 0/9] Add blockconsole version 1.1 (try 2)

At Thu, 28 Feb 2013 16:39:53 -0500,
Joern Engel wrote:
>
> Blockconsole is a console driver very roughly similar to netconsole.
> Instead of sending messages out via UDP, they are written to a block
> device. Typically a USB stick is chosen, although in principle any
> block device will do.
>
> In most cases blockconsole is useful where netconsole is not, i.e.
> single machines without network access or without an accessable
> netconsole capture server. When using both blockconsole and
> netconsole, I have found netconsole to sometimes create a mess under
> high message load (sysrq-t, etc.) while blockconsole does not.
>
> Most importantly, a number of bugs were identified and fixed that
> would have been unexplained machine reboots without blockconsole.
>
> More highlights:
> * reasonably small and self-contained code,
> * some 100+ machine years of runtime,
> * nice tutorial with a 30-sec guide for the impatient.
>
> Special thanks to Borislav Petkov for many improvements and kicking my
> behind to provide a proper git tree and resend patches.
>
> A number of cleanup patches could be folded into the main patch, but I
> decided not to mess with git history and leave any further mistakes
> for the world to laugh at:
> git://git.kernel.org/pub/scm/linux/kernel/git/joern/bcon2.git

I got a build error after pulling onto the latest Linus tree. Also I
have another couple of trivial fixes. The patches will follow.


thanks,

Takashi


2013-03-01 17:17:52

by Takashi Iwai

[permalink] [raw]
Subject: [PATCH 3/3] blockconsole: Mark a local work struct static

Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/block/blockconsole.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/blockconsole.c b/drivers/block/blockconsole.c
index 147b8af..86744cc 100644
--- a/drivers/block/blockconsole.c
+++ b/drivers/block/blockconsole.c
@@ -564,7 +564,7 @@ static void bcon_do_add(struct work_struct *work)
}
}

-DECLARE_WORK(bcon_add_work, bcon_do_add);
+static DECLARE_WORK(bcon_add_work, bcon_do_add);

void bcon_add(const char *name)
{
--
1.8.1.4

2013-03-01 17:17:51

by Takashi Iwai

[permalink] [raw]
Subject: [PATCH 2/3] blockconsole: Rename device_lock with bc_device_lock

Avoid the name conflict with device_lock() defined in linux/device.h.

Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/block/blockconsole.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/block/blockconsole.c b/drivers/block/blockconsole.c
index c22272f..147b8af 100644
--- a/drivers/block/blockconsole.c
+++ b/drivers/block/blockconsole.c
@@ -546,17 +546,17 @@ static void bcon_create_fuzzy(const char *name)
}
}

-static DEFINE_SPINLOCK(device_lock);
+static DEFINE_SPINLOCK(bc_device_lock);
static char scanned_devices[80];

static void bcon_do_add(struct work_struct *work)
{
char local_devices[80], *name, *remainder = local_devices;

- spin_lock(&device_lock);
+ spin_lock(&bc_device_lock);
memcpy(local_devices, scanned_devices, sizeof(local_devices));
memset(scanned_devices, 0, sizeof(scanned_devices));
- spin_unlock(&device_lock);
+ spin_unlock(&bc_device_lock);

while (remainder && remainder[0]) {
name = strsep(&remainder, ",");
@@ -573,11 +573,11 @@ void bcon_add(const char *name)
* to go pick it up asap. Once it is picked up, the buffer is empty
* again, so hopefully it will suffice for all sane users.
*/
- spin_lock(&device_lock);
+ spin_lock(&bc_device_lock);
if (scanned_devices[0])
strncat(scanned_devices, ",", sizeof(scanned_devices));
strncat(scanned_devices, name, sizeof(scanned_devices));
- spin_unlock(&device_lock);
+ spin_unlock(&bc_device_lock);
schedule_work(&bcon_add_work);
}

--
1.8.1.4

2013-03-01 17:17:50

by Takashi Iwai

[permalink] [raw]
Subject: [PATCH 1/3] blockconsole: Fix undefined MAX_RT_PRIO

Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/block/blockconsole.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/block/blockconsole.c b/drivers/block/blockconsole.c
index e88b8ee..c22272f 100644
--- a/drivers/block/blockconsole.c
+++ b/drivers/block/blockconsole.c
@@ -16,6 +16,7 @@
#include <linux/string.h>
#include <linux/workqueue.h>
#include <linux/sched.h>
+#include <linux/sched/rt.h>
#include <linux/ctype.h>

#define BLOCKCONSOLE_MAGIC "\nLinux blockconsole version 1.1\n"
--
1.8.1.4

2013-03-01 17:46:26

by Jörn Engel

[permalink] [raw]
Subject: Re: [PATCH 0/9] Add blockconsole version 1.1 (try 2)

On Fri, 1 March 2013 18:15:10 +0100, Takashi Iwai wrote:
>
> I got a build error after pulling onto the latest Linus tree. Also I
> have another couple of trivial fixes. The patches will follow.

Awesome, thanks! Please give me some time for caffeine and sunshine
to take effect before merging. I want to s/bc_/bcon_/g one of the
patches, otherwise they look fine.

Jörn

--
Computer system analysis is like child-rearing; you can do grievous damage,
but you cannot ensure success."
-- Tom DeMarco

2013-03-05 17:36:45

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH 0/9] Add blockconsole version 1.1 (try 2)

At Fri, 1 Mar 2013 11:22:39 -0500,
Jörn Engel wrote:
>
> On Fri, 1 March 2013 18:15:10 +0100, Takashi Iwai wrote:
> >
> > I got a build error after pulling onto the latest Linus tree. Also I
> > have another couple of trivial fixes. The patches will follow.
>
> Awesome, thanks! Please give me some time for caffeine and sunshine
> to take effect before merging. I want to s/bc_/bcon_/g one of the
> patches, otherwise they look fine.

Thanks!

Another thing I noticed is that the panic handler calls bcon_write(),
and it doesn't write to the device by itself. Is it really supposed
to work?

Meanwhile I hacked the code to allow it being built as a module.
If anyone is interested, I'll send a patch series. It's pretty
hackish, so likely need more brush up, though.


Takashi

2013-03-06 21:13:56

by Jörn Engel

[permalink] [raw]
Subject: Re: [PATCH 0/9] Add blockconsole version 1.1 (try 2)

On Tue, 5 March 2013 18:36:41 +0100, Takashi Iwai wrote:
>
> Another thing I noticed is that the panic handler calls bcon_write(),
> and it doesn't write to the device by itself. Is it really supposed
> to work?

Clearly it is supposed to work. In reality it sometimes does and
sometimes does not. Patches to improve the success rate are welcome.

> Meanwhile I hacked the code to allow it being built as a module.
> If anyone is interested, I'll send a patch series. It's pretty
> hackish, so likely need more brush up, though.

I am interested.

Jörn

--
Ninety percent of everything is crap.
-- Sturgeon's Law

2013-03-07 18:46:53

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH 0/9] Add blockconsole version 1.1 (try 2)

At Wed, 6 Mar 2013 14:49:50 -0500,
Jörn Engel wrote:
>
> On Tue, 5 March 2013 18:36:41 +0100, Takashi Iwai wrote:
> >
> > Another thing I noticed is that the panic handler calls bcon_write(),
> > and it doesn't write to the device by itself. Is it really supposed
> > to work?
>
> Clearly it is supposed to work. In reality it sometimes does and
> sometimes does not. Patches to improve the success rate are welcome.

What I thought is to driver submit_bio in the panic handler itself
so that no scheduling is involved, like the patch below. But I'm not
sure whether it would actually work...

>
> > Meanwhile I hacked the code to allow it being built as a module.
> > If anyone is interested, I'll send a patch series. It's pretty
> > hackish, so likely need more brush up, though.
>
> I am interested.

OK, I'll send a patch series. It'll be built on top of my previous
patches. And I wrote it after merging to 3.9-rc1, so might be
slightly fuzzy to your tree.


thanks,

Takashi

---
diff --git a/drivers/block/blockconsole.c b/drivers/block/blockconsole.c
index 5fc192a..5042a5a 100644
--- a/drivers/block/blockconsole.c
+++ b/drivers/block/blockconsole.c
@@ -361,6 +361,16 @@ static void bcon_writesector(struct blockconsole *bc, int index)
submit_bio(WRITE, bio);
}

+static void bcon_do_writeback(struct blockconsole *bc)
+{
+ while (bcon_write_sector(bc) != bcon_console_sector(bc)) {
+ bcon_writesector(bc, bcon_write_sector(bc));
+ bcon_advance_write_bytes(bc, SECTOR_SIZE);
+ if (bcon_write_sector(bc) == 0)
+ bcon_erase_segment(bc);
+ }
+}
+
static int bcon_writeback(void *_bc)
{
struct blockconsole *bc = _bc;
@@ -373,12 +383,7 @@ static int bcon_writeback(void *_bc)
schedule();
if (kthread_should_stop())
break;
- while (bcon_write_sector(bc) != bcon_console_sector(bc)) {
- bcon_writesector(bc, bcon_write_sector(bc));
- bcon_advance_write_bytes(bc, SECTOR_SIZE);
- if (bcon_write_sector(bc) == 0)
- bcon_erase_segment(bc);
- }
+ bcon_do_writeback(bc);
}
return 0;
}
@@ -476,7 +481,7 @@ static int blockconsole_panic(struct notifier_block *this, unsigned long event,
n = SECTOR_SIZE - bcon_console_ofs(bc);
if (n != SECTOR_SIZE)
bcon_advance_console_bytes(bc, n);
- bcon_writeback(bc);
+ bcon_do_writeback(bc);
return NOTIFY_DONE;
}

2013-03-07 18:51:21

by Takashi Iwai

[permalink] [raw]
Subject: [PATCH 2/3] block/partitions: Support dynamic addition of partition checkers

Add helper functions to add / remove the partition checker function
dynamically. This also allows to move the partition checker code into
the driver itself, e.g. the blockconsole partition checker can be in a
part of blockconsole driver (even as a module).

Signed-off-by: Takashi Iwai <[email protected]>
---
block/partitions/check.c | 35 +++++++++++++++++++++++++++++++++++
block/partitions/check.h | 8 ++++++++
2 files changed, 43 insertions(+)

diff --git a/block/partitions/check.c b/block/partitions/check.c
index 21786ed..d71cb02 100644
--- a/block/partitions/check.c
+++ b/block/partitions/check.c
@@ -17,6 +17,7 @@
#include <linux/vmalloc.h>
#include <linux/ctype.h>
#include <linux/genhd.h>
+#include <linux/export.h>

#include "check.h"

@@ -110,6 +111,9 @@ static int (*check_part[])(struct parsed_partitions *) = {
NULL
};

+static DEFINE_MUTEX(check_part_mutex);
+static LIST_HEAD(check_part_list);
+
static struct parsed_partitions *allocate_partitions(struct gendisk *hd)
{
struct parsed_partitions *state;
@@ -172,6 +176,21 @@ check_partition(struct gendisk *hd, struct block_device *bdev)
}

}
+
+ if (res <= 0) {
+ struct partition_checker *c;
+ mutex_lock(&check_part_mutex);
+ list_for_each_entry(c, &check_part_list, list) {
+ memset(state->parts, 0, state->limit * sizeof(state->parts[0]));
+ res = c->check(state);
+ if (res < 0) {
+ err = res;
+ res = 0;
+ }
+ }
+ mutex_unlock(&check_part_mutex);
+ }
+
if (res > 0) {
printk(KERN_INFO "%s", state->pp_buf);

@@ -194,3 +213,19 @@ check_partition(struct gendisk *hd, struct block_device *bdev)
free_partitions(state);
return ERR_PTR(res);
}
+
+void append_partition_checker(struct partition_checker *c)
+{
+ mutex_lock(&check_part_mutex);
+ list_add_tail(&c->list, &check_part_list);
+ mutex_unlock(&check_part_mutex);
+}
+EXPORT_SYMBOL_GPL(append_partition_checker);
+
+void remove_partition_checker(struct partition_checker *c)
+{
+ mutex_lock(&check_part_mutex);
+ list_del(&c->list);
+ mutex_unlock(&check_part_mutex);
+}
+EXPORT_SYMBOL_GPL(remove_partition_checker);
diff --git a/block/partitions/check.h b/block/partitions/check.h
index bca9624..53c8508 100644
--- a/block/partitions/check.h
+++ b/block/partitions/check.h
@@ -55,3 +55,11 @@ extern int warn_no_part;
#ifdef CONFIG_BLOCKCONSOLE
int blockconsole_partition(struct parsed_partitions *state);
#endif
+
+struct partition_checker {
+ int (*check)(struct parsed_partitions *);
+ struct list_head list;
+};
+
+void append_partition_checker(struct partition_checker *c);
+void remove_partition_checker(struct partition_checker *c);
--
1.8.1.4

2013-03-07 18:51:20

by Takashi Iwai

[permalink] [raw]
Subject: [PATCH 1/3] Export sched_setscheduler_nocheck()

... to be usable by modules.

Signed-off-by: Takashi Iwai <[email protected]>
---
kernel/sched/core.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 7f12624..370d97d 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3993,6 +3993,7 @@ int sched_setscheduler_nocheck(struct task_struct *p, int policy,
{
return __sched_setscheduler(p, policy, param, false);
}
+EXPORT_SYMBOL_GPL(sched_setscheduler_nocheck);

static int
do_sched_setscheduler(pid_t pid, int policy, struct sched_param __user *param)
--
1.8.1.4

2013-03-07 18:51:18

by Takashi Iwai

[permalink] [raw]
Subject: [PATCH 0/3] blockconsole: make it a module

Hi,

this is a patch series to allow to build the blockconsole as a module.
In a short test, it seems working somehow. The module removal code is
a bit scary and there is much room for improvements / simplification.


thanks,

Takashi

2013-03-07 18:52:00

by Takashi Iwai

[permalink] [raw]
Subject: [PATCH 3/3] blockconsole: Allow to be a module

Move the partition checker code into blockconsole driver so that it
can be built as a module, too. linux/blockconsole.h is removed since
all stuff is found locally in drivers/block/blockconsole.c. Also some
functions are marked as static for the same reason.

A new linked list is added to blockconsole instance for managing the
proper unregistration at unloading the module on the fly.

Signed-off-by: Takashi Iwai <[email protected]>
---
block/partitions/Makefile | 1 -
block/partitions/blockconsole.c | 22 -----------
block/partitions/check.c | 3 --
block/partitions/check.h | 4 --
drivers/block/Kconfig | 2 +-
drivers/block/blockconsole.c | 84 +++++++++++++++++++++++++++++++++++++++--
include/linux/blockconsole.h | 7 ----
7 files changed, 81 insertions(+), 42 deletions(-)
delete mode 100644 block/partitions/blockconsole.c
delete mode 100644 include/linux/blockconsole.h

diff --git a/block/partitions/Makefile b/block/partitions/Makefile
index bf26d4a..03af8ea 100644
--- a/block/partitions/Makefile
+++ b/block/partitions/Makefile
@@ -18,4 +18,3 @@ obj-$(CONFIG_IBM_PARTITION) += ibm.o
obj-$(CONFIG_EFI_PARTITION) += efi.o
obj-$(CONFIG_KARMA_PARTITION) += karma.o
obj-$(CONFIG_SYSV68_PARTITION) += sysv68.o
-obj-$(CONFIG_BLOCKCONSOLE) += blockconsole.o
diff --git a/block/partitions/blockconsole.c b/block/partitions/blockconsole.c
deleted file mode 100644
index 79796a8..0000000
--- a/block/partitions/blockconsole.c
+++ /dev/null
@@ -1,22 +0,0 @@
-#include <linux/blockconsole.h>
-
-#include "check.h"
-
-int blockconsole_partition(struct parsed_partitions *state)
-{
- Sector sect;
- void *data;
- int err = 0;
-
- data = read_part_sector(state, 0, &sect);
- if (!data)
- return -EIO;
- if (!bcon_magic_present(data))
- goto out;
-
- bcon_add(state->name);
- err = 1;
-out:
- put_dev_sector(sect);
- return err;
-}
diff --git a/block/partitions/check.c b/block/partitions/check.c
index d71cb02..d9e9324 100644
--- a/block/partitions/check.c
+++ b/block/partitions/check.c
@@ -43,9 +43,6 @@ static int (*check_part[])(struct parsed_partitions *) = {
* Probe partition formats with tables at disk address 0
* that also have an ADFS boot block at 0xdc0.
*/
-#ifdef CONFIG_BLOCKCONSOLE
- blockconsole_partition,
-#endif
#ifdef CONFIG_ACORN_PARTITION_ICS
adfspart_check_ICS,
#endif
diff --git a/block/partitions/check.h b/block/partitions/check.h
index 53c8508..886ed32 100644
--- a/block/partitions/check.h
+++ b/block/partitions/check.h
@@ -52,10 +52,6 @@ put_partition(struct parsed_partitions *p, int n, sector_t from, sector_t size)

extern int warn_no_part;

-#ifdef CONFIG_BLOCKCONSOLE
-int blockconsole_partition(struct parsed_partitions *state);
-#endif
-
struct partition_checker {
int (*check)(struct parsed_partitions *);
struct list_head list;
diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 7da1360..2049ad4 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -542,7 +542,7 @@ config BLK_DEV_RSXX
module will be called rsxx.

config BLOCKCONSOLE
- bool "Block device console logging support"
+ tristate "Block device console logging support"
help
This enables logging to block devices.
See <file:Documentation/block/blockconsole.txt> for details.
diff --git a/drivers/block/blockconsole.c b/drivers/block/blockconsole.c
index 86744cc..5fc192a 100644
--- a/drivers/block/blockconsole.c
+++ b/drivers/block/blockconsole.c
@@ -4,13 +4,13 @@
* Copyright (C) 2012 Joern Engel <[email protected]>
*/
#include <linux/bio.h>
-#include <linux/blockconsole.h>
#include <linux/console.h>
#include <linux/fs.h>
#include <linux/kref.h>
#include <linux/kthread.h>
#include <linux/mm.h>
#include <linux/mount.h>
+#include <linux/module.h>
#include <linux/random.h>
#include <linux/slab.h>
#include <linux/string.h>
@@ -18,6 +18,7 @@
#include <linux/sched.h>
#include <linux/sched/rt.h>
#include <linux/ctype.h>
+#include "../../block/partitions/check.h"

#define BLOCKCONSOLE_MAGIC "\nLinux blockconsole version 1.1\n"
#define BCON_UUID_OFS (32)
@@ -68,8 +69,15 @@ struct blockconsole {
struct work_struct release_work;
struct task_struct *writeback_thread;
struct notifier_block panic_block;
+ struct list_head list;
};

+static DEFINE_SPINLOCK(bc_device_lock);
+static LIST_HEAD(bc_list_head);
+static atomic_t bc_list_count = ATOMIC_INIT(0);
+
+static int bcon_magic_present(const void *data);
+
static void bcon_get(struct blockconsole *bc)
{
kref_get(&bc->kref);
@@ -85,6 +93,7 @@ static void __bcon_release(struct work_struct *work)
invalidate_mapping_pages(bc->bdev->bd_inode->i_mapping, 0, -1);
blkdev_put(bc->bdev, FMODE_READ|FMODE_WRITE);
kfree(bc);
+ atomic_dec(&bc_list_count);
}

static void bcon_release(struct kref *kref)
@@ -285,6 +294,11 @@ static void bcon_unregister(struct work_struct *work)
del_timer_sync(&bc->pad_timer);
kthread_stop(bc->writeback_thread);
/* No new io will be scheduled anymore now */
+
+ spin_lock(&bc_device_lock);
+ list_del_init(&bc->list);
+ spin_unlock(&bc_device_lock);
+
bcon_put(bc);
}

@@ -515,7 +529,13 @@ static int bcon_create(const char *devname)
bc->panic_block.notifier_call = blockconsole_panic;
bc->panic_block.priority = INT_MAX;
atomic_notifier_chain_register(&panic_notifier_list, &bc->panic_block);
+
+ spin_lock(&bc_device_lock);
+ list_add(&bc->list, &bc_list_head);
+ spin_unlock(&bc_device_lock);
+
pr_info("now logging to %s at %llx\n", devname, bc->console_bytes >> 20);
+ atomic_inc(&bc_list_count);

return 0;

@@ -546,13 +566,13 @@ static void bcon_create_fuzzy(const char *name)
}
}

-static DEFINE_SPINLOCK(bc_device_lock);
static char scanned_devices[80];

static void bcon_do_add(struct work_struct *work)
{
char local_devices[80], *name, *remainder = local_devices;

+ atomic_inc(&bc_list_count);
spin_lock(&bc_device_lock);
memcpy(local_devices, scanned_devices, sizeof(local_devices));
memset(scanned_devices, 0, sizeof(scanned_devices));
@@ -562,11 +582,12 @@ static void bcon_do_add(struct work_struct *work)
name = strsep(&remainder, ",");
bcon_create_fuzzy(name);
}
+ atomic_dec(&bc_list_count);
}

static DECLARE_WORK(bcon_add_work, bcon_do_add);

-void bcon_add(const char *name)
+static void bcon_add(const char *name)
{
/*
* We add each name to a small static buffer and ask for a workqueue
@@ -602,7 +623,7 @@ static bool is_four_byte_hex(const void *data)
return true;
}

-int bcon_magic_present(const void *data)
+static int bcon_magic_present(const void *data)
{
size_t len = strlen(BLOCKCONSOLE_MAGIC);

@@ -616,3 +637,58 @@ int bcon_magic_present(const void *data)
return 0;
return 11;
}
+
+static int blockconsole_partition(struct parsed_partitions *state)
+{
+ Sector sect;
+ void *data;
+ int err = 0;
+
+ data = read_part_sector(state, 0, &sect);
+ if (!data)
+ return -EIO;
+ if (!bcon_magic_present(data))
+ goto out;
+
+ bcon_add(state->name);
+ err = 1;
+out:
+ put_dev_sector(sect);
+ return err;
+}
+
+static struct partition_checker bc_part_checker = {
+ .check = blockconsole_partition
+};
+
+static int __init bcon_module_init(void)
+{
+ append_partition_checker(&bc_part_checker);
+ return 0;
+}
+
+static void __exit bcon_module_exit(void)
+{
+ remove_partition_checker(&bc_part_checker);
+ cancel_work_sync(&bcon_add_work);
+
+ /* unregister all pending consoles */
+ spin_lock(&bc_device_lock);
+ while (!list_empty(&bc_list_head)) {
+ struct blockconsole *bc;
+ bc = list_first_entry(&bc_list_head, struct blockconsole, list);
+ schedule_work(&bc->unregister_work);
+ list_del_init(&bc->list);
+ }
+ spin_unlock(&bc_device_lock);
+
+ while (atomic_read(&bc_list_count)) {
+ flush_scheduled_work();
+ schedule_timeout(1);
+ }
+}
+
+MODULE_LICENSE("GPL");
+
+module_init(bcon_module_init);
+module_exit(bcon_module_exit);
diff --git a/include/linux/blockconsole.h b/include/linux/blockconsole.h
deleted file mode 100644
index 114f7c5..0000000
--- a/include/linux/blockconsole.h
+++ /dev/null
@@ -1,7 +0,0 @@
-#ifndef LINUX_BLOCKCONSOLE_H
-#define LINUX_BLOCKCONSOLE_H
-
-int bcon_magic_present(const void *data);
-void bcon_add(const char *name);
-
-#endif
--
1.8.1.4