Re: [RFC] change of ->bd_op->open() semantics

Re: [RFC] change of ->bd_op->open() semantics

[Andries.Brouwer]

Al writes:

    4) moving the call of partition-reading code into do_open() (for cases when
    ->bd_contains is non-NULL), killing the "set block_device fields by hand"
    mess in check_partitions()

Hmm. I probably misunderstand some things.

There must be a data structure with the information that belongs
to a disk, opened or not. The size, the sectorsize, the partitions, ...
The structure that I made kdev_t point at.

What are you working towards? There is a struct block_device.
But it is not permanent. It is created when the device is opened
and disappears when it is closed.
So it doesnt help in replacing kdev_t - the permanent data must still
be somewhere. There is a struct gendisk. That is better.
It is not difficult to make sure that all devices that have array data
also get a struct gendisk.
Then there is struct request_queue. I see that it got hardsect_size,
but it seems a less suitable place for general disk info.

[So, let me repeat: Question: where do you plan to put permanent
disk data?]

Now about this partition-reading code. It requires a partitiontable type,
just like mounting requires a filesystem type. Now mount() has a
parameter that specifies the filesystem type. But open() does not
have a parameter that specifies a partitiontable type.
It seems to me that doing partitiontable reading in do_open()
is a really bad idea.

Maybe you have a "do it only once" kludge in mind?
To get something ugly, equivalent to the present situation?

Andries

Re: [RFC] change of ->bd_op->open() semantics

[Alexander Viro]

On Fri, 24 May 2002, Linus Torvalds wrote:

> 
> 
> On Fri, 24 May 2002, Alexander Viro wrote:
> >
> > 	It has an additional benefit of killing the array of default
> > queues on the same pass - a thing we will need to do sooner or later
> > anyway.
> 
> I'd like to see this, because we want to make the "find the right queue" a
> much more expensive operation (no longer some fairly simple mapping from
> major number - a more dynamic and general "register this queue for minors
> xxxx-yyyy of major zzz").
> 
> Doing it just once at open() time allows for that to happen without any
> performance downside.

OK.  Plan of company:

1) killing the last caller of get_hardsect_size() (switching it to
bdev_hardsect_size()).

2) killing blk_get_queue() and switching to bdev_get_queue()

3) moving the contents of bdev_get_queue() into do_open() and check_partitions()
and caching result in new field of struct block_device; cleaning it at the
same places that reset ->bd_op, etc.; making bdev_get_queue() return cached
pointer.

4) moving the call of partition-reading code into do_open() (for cases when
->bd_contains is non-NULL), killing the "set block_device fields by hand"
mess in check_partitions()

5) moving the code that sets ->bd_queue into instances of ->open() (leaving
the code in do_open() in place for a while - it's OK since it would be
conditional by ->bd_queue == NULL).

6) after the series of per-driver patches (see 5) is over, kill that code
in do_open() and kill (now unused) array.

7) deal with devfs-related fallout from (4).

(4) is needed since we really can't afford any IO-before-open() anymore and
we need lazy partition-reading to deal with that.  OTOH, we can very well
do (1)--(3) and stay at that - result will still have the array, etc., but at
least we won't have recalculate the damn thing for each request.

I would prefer to go all way and move all code that chooses queue into
the ->open(), but it might make sense to split it into several series.
Your call...

Now, I've already sent a patch that does (1) and (2) (last one sent on
Wednesday), but it doesn't show up on bkbits.  If there were any problems
with it - please, tell.  If not - I can
	* resend it, following it by (3)
	* send (3) alone
	* wait for 2.5.18, rediff and send (1)--(3).
Take your pick...

I'd definitely like to have a testing point between (3) and (4)--(7), so
unless you are going to release 2.5.18 right now I'd prefer to get (1)--(3)
into it...

Re: [RFC] change of ->bd_op->open() semantics

[Martin Dalecki]

Uz.ytkownik Alexander Viro napisa?:
> 
> On Fri, 24 May 2002, Linus Torvalds wrote:

> Now, I've already sent a patch that does (1) and (2) (last one sent on
> Wednesday), but it doesn't show up on bkbits.  If there were any problems
> with it - please, tell.  If not - I can
> 	* resend it, following it by (3)
> 	* send (3) alone
> 	* wait for 2.5.18, rediff and send (1)--(3).
> Take your pick...
> 
> I'd definitely like to have a testing point between (3) and (4)--(7), so
> unless you are going to release 2.5.18 right now I'd prefer to get (1)--(3)
> into it...

I would love to be warned about it, since the ATA code is
very well tricky in the way in esp. partition scanning goes on.
Plese not as well the the recently added attach() method
for ATA/ATAPI device type drivers may very well come
in between as well. (patch nr. 70.)

Re: [RFC] change of ->bd_op->open() semantics

[Martin Dalecki]

Uz.ytkownik Linus Torvalds napisa?:
> 
> On Fri, 24 May 2002, Alexander Viro wrote:
> 
>>	It has an additional benefit of killing the array of default
>>queues on the same pass - a thing we will need to do sooner or later
>>anyway.
> 
> 
> I'd like to see this, because we want to make the "find the right queue" a
> much more expensive operation (no longer some fairly simple mapping from
> major number - a more dynamic and general "register this queue for minors
> xxxx-yyyy of major zzz").
> 
> Doing it just once at open() time allows for that to happen without any
> performance downside.
> 
> 		Linus

Current ATA code says about this:
/*
  * Returns the queue which corresponds to a given device.
  *
  * FIXME: this should take struct block_device * as argument in future.
  */
static request_queue_t *ata_get_queue(kdev_t dev)
{
	struct ata_channel *ch = (struct ata_channel *)blk_dev[major(dev)].data;

	/* FIXME: ALLERT: This discriminates between master and slave! */
	return &ch->drives[DEVICE_NR(dev) & 1].queue;
}

Guess who did spill the FIXME: entiers there?
So of course plese plese go ahead as soon as possible!.
I would love to don't have to look at the above any longer.

Re: [RFC] change of ->bd_op->open() semantics

[Linus Torvalds]

On Fri, 24 May 2002, Alexander Viro wrote:
>
> 	It has an additional benefit of killing the array of default
> queues on the same pass - a thing we will need to do sooner or later
> anyway.

I'd like to see this, because we want to make the "find the right queue" a
much more expensive operation (no longer some fairly simple mapping from
major number - a more dynamic and general "register this queue for minors
xxxx-yyyy of major zzz").

Doing it just once at open() time allows for that to happen without any
performance downside.

		Linus

[RFC] change of ->bd_op->open() semantics

[Alexander Viro]

	Folks, I propose to cache the result of blk_get_queue() in
struct block_device upon ->open().

	We already have struct block_device * whenever we are looking for
a queue.  So the only real problem arises if value of blk_get_queue()
changes while device is opened.  AFAICS nothing in the kernel pulls
such tricks (and to support them we would need to take care of a lot
of things, so changing the cached value wouldn't be a problem anyway).

	Yes, it means updating every instance of ->open() for block
devices.  All 30-odd of them.  I can do that in a single pass - changes
are trivial.

	It has an additional benefit of killing the array of default
queues on the same pass - a thing we will need to do sooner or later
anyway.

	Does anybody see a problem with that?