Re: per BDI dirty limit (was Re: -mm merge plans for 2.6.24)

[Kay Sievers <kay.sievers@vrfy.org>]

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