On Sat, 2007-10-27 at 09:02 -0700, Greg KH wrote:
> On Sat, Oct 27, 2007 at 10:39:59AM +0200, Peter Zijlstra wrote:
> > On Fri, 2007-10-26 at 19:40 -0700, Greg KH wrote:
> > > On Sat, Oct 27, 2007 at 03:18:08AM +0200, Peter Zijlstra wrote:
> > > > On Fri, 2007-10-26 at 22:04 +0200, Peter Zijlstra wrote:
> > > > > This crashes and burns on bootup, but I'm too tired to figure out
> > > > > what I
> > > > > did wrong... will give it another try tomorrow..
> > > >
> > > > Ok, can't sleep.. took a look. I have several problems here.
> > > >
> > > > The thing that makes it go *boom* is the __ATTR_NULL. Removing that
> > > > makes it boot. Albeit it then warns me of multiple duplicate sysfs
> > > > objects, all named "bdi".
> > > I'll look at this and see what I can come up with. Would you just like
> > > a whole new patch, or one against this one?
> >
> > Sorry for the grumpy note, I get that way at 3.30 am. Maybe I ought not
> > have mailed :-/
> >
> > This is the code I had at that time.
>
> Ah, I see a few problems. Here, try this version instead. It's
> compile-tested only, and should be a lot simpler.
>
> Note, we still are not setting the parent to the new bdi structure
> properly, so the devices will show up in /sys/devices/virtual/ instead
> of in their proper location. To do this, we need the parent of the
> device, which I'm not so sure what it should be (block device? block
> device controller?)
Assigning a parent device will only work with the upcoming conversion of
the raw kobjects in the block subsystem to "struct device".
A few comments to the patch:
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -8,6 +8,7 @@
> #include <linux/compiler.h> /* for inline */
> #include <linux/types.h> /* for size_t */
> #include <linux/stddef.h> /* for NULL */
> +#include <stdarg.h>
>
> #ifdef __cplusplus
> extern "C" {
> @@ -111,6 +112,9 @@ extern void *kmemdup(const void *src, si
> extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
> extern void argv_free(char **argv);
>
> +char *kvprintf(const char *fmt, va_list args);
> +char *kprintf(const char *fmt, ...);
Why is that here? I don't think we need this when we use the existing:
kvasprintf(GFP_KERNEL, fmt, args)
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> +
> +static struct device_attribute bdi_dev_attrs[] = {
> + __ATTR(readahead, 0644, readahead_show, readahead_store),
> + __ATTR_RO(reclaimable),
> + __ATTR_RO(writeback),
> + __ATTR_RO(dirty),
> + __ATTR_RO(bdi_dirty),
> +};
Default attributes will need the NULL termination back (see below).
> +static __init int bdi_class_init(void)
> +{
> + bdi_class = class_create(THIS_MODULE, "bdi");
> + return 0;
> +}
> +
> +__initcall(bdi_class_init);
> +
> +int bdi_register(struct backing_dev_info *bdi, const char *fmt, ...)
This function should accept a: "struct device *parent" and all callers
just pass NULL until the block layer conversion gets merged.
> +{
> + char *name;
> + va_list args;
> + int ret = -ENOMEM;
> + int i;
> +
> + va_start(args, fmt);
> + name = kvprintf(fmt, args);
kvasprintf(GFP_KERNEL, fmt, args);
> + va_end(args);
> +
> + if (!name)
> + return -ENOMEM;
> +
> + bdi->dev = device_create(bdi_class, NULL, MKDEV(0,0), name);
The parent should be passed here.
> + for (i = 0; i < ARRAY_SIZE(bdi_dev_attrs); i++) {
> + ret = device_create_file(bdi->dev, &bdi_dev_attrs[i]);
> + if (ret)
> + break;
> + }
> + if (ret) {
> + while (--i >= 0)
> + device_remove_file(bdi->dev, &bdi_dev_attrs[i]);
> + device_unregister(bdi->dev);
> + bdi->dev = NULL;
> + }
All this open-coded attribute stuff should go away and be replaced by:
bdi_class->dev_attrs = bdi_dev_attrs;
Otherwise at event time the attributes are not created and stuff hooking
into the events will not be able to set values. Also, the core will do
proper add/remove and error handling then.
Thanks,
Kay
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
