Message-ID: <19012.49673.454853.975682@notabene.brown>
Date:	Fri, 26 Jun 2009 22:41:45 +1000
From:	Neil Brown <neilb@...e.de>
To:	Jens Axboe <jens.axboe@...cle.com>
Cc:	"Martin K. Petersen" <martin.petersen@...cle.com>,
	Mike Snitzer <snitzer@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Alasdair G Kergon <agk@...hat.com>,
	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-raid@...r.kernel.org, linux-ide@...r.kernel.org,
	linux-fsdevel@...r.kernel.org,
	device-mapper development <dm-devel@...hat.com>
Subject: Re: [dm-devel] REQUEST for new 'topology' metrics to be moved out
	of the 'queue' sysfs directory.

On Thursday June 25, jens.axboe@...cle.com wrote:
> On Thu, Jun 25 2009, NeilBrown wrote:
> > > You seem to be hung up on the fact that you don't queue things.  I think
> > > that's beside the point.  You *do* have a request_queue thanks to
> > > calling blk_queue_make_request() in md.c.  And there is more to
> > > request_queue than the values you brought up.  Like the callback
> > > functions.  I'm not saying that all the values in request_queue apply to
> > > MD, but I really don't understand what all the fuss is about.  Other
> > > than the presence of the string "queue" in the choice of naming.
> > >
> > 
> > Well names are very important.  And as I said later we could possibly keep
> > them in 'queue' and make 'queue' a more generic directory.  I don't like
> > that but it is probably better than the current situation.
> 
> Sorry to ask the obvious question, but what would the point of all this
> pain be? The existing values can't go anywhere else, so you'd have to
> add symlinks back into queue/ anyway.

Why cannot the existing values go any where else?  I don't understand
that assertion at all.

> 
> > As you say, I do currently have a request_queue, but that is an internal
> > detail, not part of the externally visible interface, and it is something
> > that is very much in my sights as something I want to change.  I'm
> > still working out the details so I'm a fair way from a concrete proposal
> > and a long way from some code.  That change certainly doesn't have
> > to happen in any rush.  But we should get the externally visible
> > names "right" if we can.
> 
> What crack are you smoking? :-)

I have a special mix of crack that helps me see Patterns everywhere,
even in C code.  Some patterns are bright, shiny, and elegant.  Others
are muddy and confused.  struct request_queue has a distinct shadow
over it just now.

> 
> A block device must have a request_queue, that's pretty much spread
> throughout the kernel. The fact that md/dm is only using a subset of the
> functionality is not a very good reason for re-writing large parts of
> that design. We could save some space, but whether the queue is 200
> bytes or 1400 bytes doesn't really make a whole lot of real-world
> difference. It's not like we allocate/deallocate these all the time,
> they are mostly static structures.

It isn't about saving space.  It is about maintainability.  To be
maintainable, the code must be easy to understand.  For that, it must
be well structured.

Every block device has a 'gendisk' (aka generic disk).
Every block device also (currently) has a request_queue.
If I have generic data that is applicable to all disks, where should I
put it?  One would think "gendisk".
If I have data that is related to request handling (as in uses of 
'struct request'), where should that go?  in request_queue I suspect.
But there are several fields in request_queue that are not related to
requests, and are generic to all disks.

I think "generic data goes in gendisk" is something that it would be
easy for people to understand.
The current 'topology' values are intended to be generic to all disks
so they should ideally go in gendisk.  I'm not pushing for that now.
Longer term, that would be my aim, but for now I'm just focussing on
restoring the 'queue' subdirectory to it's previous non-generic state.

i.e. revert the change that made 'queue' appear for md and dm and loop
and nbd and .... which have all never needed it before.
And find somewhere else to put the topology data - probably just the
top level.
i.e. add the names as DEVICE_ATTRs in genhd.c, and write the 'show'
routine to dereference ->queue carefully, just like
disk_alignment_offset_show.
(oooo... just noticed that the 'alignment_offset' attribute can
 contain the string '-1'.... while I have been guilty of that sort of
 thing myself, I would much rather it said "misaligned".)


As for how intrusive vs beneficial it would be to move all the generic
fields out of request_queue and allow md to not have a request queue,
that will have to be a discussion for another day.  I do hope to
eventually present you with a series of patches which does just that.
My aim would be to make sure each one was clearly beneficial.  And I
do have a grand vision involving this which is more than just tidying
up some small in-elegances.  Only time will tell how it eventuates.


But for now, please please please can we revert the change which made
'queue' appear in md and dm devices, (and loop and ...) and put these
generic values somewhere ... generic?

Thanks.

NeilBrown

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists