bdi: register sysfs bdi device only once per queue

bdi: register sysfs bdi device only once per queue

[Kay Sievers]

Andrew, can you pick this up this please?

Thanks,
Kay


From: Kay Sievers <kay.sievers@vrfy.org>
Subject: bdi: register sysfs bdi device only once per queue

Devices which share the same queue, like floppies and mtd devices,
get registered multiple times in the bdi interface, but bdi accounts
only the last registered device of the devices sharing one queue.

On remove, all earlier registered devices leak, stay around in
sysfs, and cause "duplicate filename" errors if the devices are
re-created.

This prevents the creation of multiple bdi interfaces per queue,
and the bdi device will carry the dev_t name of the block device
which is the first one registered, of the pool of devices using
the same queue.

Tested-By: Peter Korsgaard <jacmet@sunsite.dk>
Acked-By: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-Off-By: Kay Sievers <kay.sievers@vrfy.org>
---

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index f2e574d..e6676e5 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -176,6 +176,9 @@ int bdi_register(struct backing_dev_info *bdi, struct device *parent,
 	int ret = 0;
 	struct device *dev;
 
+	if (bdi->dev)
+		goto exit;
+
 	va_start(args, fmt);
 	dev = device_create_vargs(bdi_class, parent, MKDEV(0, 0), bdi, fmt, args);
 	va_end(args);

Re: bdi: register sysfs bdi device only once per queue

[Peter Zijlstra]

On Mon, 2008-11-03 at 16:53 +0100, Kay Sievers wrote:
> Andrew, can you pick this up this please?
> 
> Thanks,
> Kay
> 
> 
> From: Kay Sievers <kay.sievers@vrfy.org>
> Subject: bdi: register sysfs bdi device only once per queue
> 
> Devices which share the same queue, like floppies and mtd devices,
> get registered multiple times in the bdi interface, but bdi accounts
> only the last registered device of the devices sharing one queue.

David, is there any reason the mtd devices do this?

> On remove, all earlier registered devices leak, stay around in
> sysfs, and cause "duplicate filename" errors if the devices are
> re-created.
> 
> This prevents the creation of multiple bdi interfaces per queue,
> and the bdi device will carry the dev_t name of the block device
> which is the first one registered, of the pool of devices using
> the same queue.
> 
> Tested-By: Peter Korsgaard <jacmet@sunsite.dk>
> Acked-By: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-Off-By: Kay Sievers <kay.sievers@vrfy.org>
> ---
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index f2e574d..e6676e5 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -176,6 +176,9 @@ int bdi_register(struct backing_dev_info *bdi, struct device *parent,
>  	int ret = 0;
>  	struct device *dev;
>  
> +	if (bdi->dev)
> +		goto exit;
> +
>  	va_start(args, fmt);
>  	dev = device_create_vargs(bdi_class, parent, MKDEV(0, 0), bdi, fmt, args);
>  	va_end(args);

Would it make sense to make it print something like, please fix me?

Re: bdi: register sysfs bdi device only once per queue

[Andrew Morton]

On Mon, 03 Nov 2008 17:08:50 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, 2008-11-03 at 16:53 +0100, Kay Sievers wrote:
> > Andrew, can you pick this up this please?
> > 
> > Thanks,
> > Kay
> > 
> > 
> > From: Kay Sievers <kay.sievers@vrfy.org>
> > Subject: bdi: register sysfs bdi device only once per queue
> > 
> > Devices which share the same queue, like floppies and mtd devices,
> > get registered multiple times in the bdi interface, but bdi accounts
> > only the last registered device of the devices sharing one queue.
> 
> David, is there any reason the mtd devices do this?

[tap tap - is this thing turned on?]

> > On remove, all earlier registered devices leak, stay around in
> > sysfs, and cause "duplicate filename" errors if the devices are
> > re-created.
> > 
> > This prevents the creation of multiple bdi interfaces per queue,
> > and the bdi device will carry the dev_t name of the block device
> > which is the first one registered, of the pool of devices using
> > the same queue.
> > 
> > Tested-By: Peter Korsgaard <jacmet@sunsite.dk>
> > Acked-By: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Signed-Off-By: Kay Sievers <kay.sievers@vrfy.org>
> > ---
> > 
> > diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> > index f2e574d..e6676e5 100644
> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
> > @@ -176,6 +176,9 @@ int bdi_register(struct backing_dev_info *bdi, struct device *parent,
> >  	int ret = 0;
> >  	struct device *dev;
> >  
> > +	if (bdi->dev)
> > +		goto exit;
> > +
> >  	va_start(args, fmt);
> >  	dev = device_create_vargs(bdi_class, parent, MKDEV(0, 0), bdi, fmt, args);
> >  	va_end(args);
> 
> Would it make sense to make it print something like, please fix me?

Yes, I think it would.

--- a/mm/backing-dev.c~bdi-register-sysfs-bdi-device-only-once-per-queue-fix
+++ a/mm/backing-dev.c
@@ -176,7 +176,7 @@ int bdi_register(struct backing_dev_info
 	int ret = 0;
 	struct device *dev;
 
-	if (bdi->dev)
+	if (WARN_ON(bdi->dev))
 		goto exit;
 
 	va_start(args, fmt);
_


It's a bit cheeky to add a known-to-trigger WARN_ON into -rc4 but if we
don't do this then the issue will just get swept under the carpet.

Re: bdi: register sysfs bdi device only once per queue

[David Woodhouse]

On Wed, 2008-11-05 at 12:10 -0800, Andrew Morton wrote:
> > > Devices which share the same queue, like floppies and mtd devices,
> > > get registered multiple times in the bdi interface, but bdi accounts
> > > only the last registered device of the devices sharing one queue.
> > 
> > David, is there any reason the mtd devices do this?
> 
> [tap tap - is this thing turned on?]

No particular reason. It's been done that way since the dawn of time,
when it was perfectly normal. Nobody's yet bothered to change that, when
changing the rules.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: bdi: register sysfs bdi device only once per queue

[Peter Zijlstra]

On Wed, 2008-11-05 at 23:10 +0100, David Woodhouse wrote:
> On Wed, 2008-11-05 at 12:10 -0800, Andrew Morton wrote:
> > > > Devices which share the same queue, like floppies and mtd devices,
> > > > get registered multiple times in the bdi interface, but bdi accounts
> > > > only the last registered device of the devices sharing one queue.
> > > 
> > > David, is there any reason the mtd devices do this?
> > 
> > [tap tap - is this thing turned on?]
> 
> No particular reason. It's been done that way since the dawn of time,
> when it was perfectly normal. Nobody's yet bothered to change that, when
> changing the rules.

I don't think it ever made sense, or was intended that way, but yeah it
didn't really matter in any substantial way.

I can remember touching some mtd stuff back when I did the
per-bdi-writeback stuff, but I can't seem to recall all the details.

Might be time to stare at the mtd code a bit...

Re: bdi: register sysfs bdi device only once per queue

[Peter Zijlstra]

On Wed, 2008-11-05 at 23:10 +0100, David Woodhouse wrote:
> On Wed, 2008-11-05 at 12:10 -0800, Andrew Morton wrote:
> > > > Devices which share the same queue, like floppies and mtd devices,
> > > > get registered multiple times in the bdi interface, but bdi accounts
> > > > only the last registered device of the devices sharing one queue.
> > > 
> > > David, is there any reason the mtd devices do this?
> > 
> > [tap tap - is this thing turned on?]
> 
> No particular reason. It's been done that way since the dawn of time,
> when it was perfectly normal. Nobody's yet bothered to change that, when
> changing the rules.

It looks to be the consequence of using one queue for a bunch of devices
(which seems odd), and its all tied into this translation layer which
starts a thread per queue.

Do you want me to rip this up and make it do one queue/thread per
device? - seems invasive...