RE: [PATCH] 2.5.43 cciss rescan disk ioctl

From: Cameron, Steve (Steve.Cameron@hp.com)
Date: Fri Oct 18 2002 - 11:43:25 EST


Al Viro wrote:

> On Fri, 18 Oct 2002, Stephen Cameron wrote:
>
[...]
>
> > + if (minor(inode->i_rdev) != 0) {
> > + /* if not node 0 make sure it is a
> partition = 0 */
> > + if (minor(inode->i_rdev) & 0x0f)
> > + return -ENXIO;
>
> That's bogus. We call ->open() only for entire disk.

Ok.

>
> > + kdev = mk_kdev(MAJOR_NR + ctlr, disk->first_minor);
> > + bdev = bdget(kdev_t_to_nr(kdev));
> > + rescan_partitions(disk, bdev);
>
> ... and that is (a) obvious leak and

Ok. I thought it might be, but I wasn't sure.
(I was able to imagine a way it might work
and not be a leak, as bdget appears to search through a list and only
alloc if it doesn't find it already there.) But I don't have
anything like a complete understanding of bdget(), to state the
obvious.

< (b) broken unless disk is already open - it will try to do IO on bdev.

Hmm. It worked when I tried it (apart from the leak).
Maybe the disk was already open. The situation was that the
disk is there, (will respond to inquiry) but is reserved by
another host so read capacity will fail. Maybe I was just
(un)lucky. Anyway, I think you're saying there are situations
that I didn't try that don't work. Ok, forget this patch.

> More interesting issue, though, is whether we need to bother with
> complications coming from that interface anymore. Notice that
> comment re "too many device nodes" doesn't hold these days - that
> sort of stuff looks like a candidate for "write command into node
> on driverfs" - especially when we are talking about configuring
> the device, shutting disks down/starting them up, etc. Linus, Pat?
> Unless I'm mistaken that's precisely the sort of work that is
> supposed to live in driverfs...
>

I haven't looked into driverfs too much yet. (too much time
spent keeping up with the latest redhat/suse kernel, etc.)

Is there anything written up about it to help out driver
writers? I'm not much looking forward to telling our ACU
and CIM guys they need to change all their apps to use
driverfs interfaces. Also, I'm hoping driverfs is better
than /proc, in the sense that /proc tends to be read-only,
or write-only, unlike ioctl() where you can put in a complex
input and get back a corresponding output. With the likes of
/proc, you can write something in there, but how do you get
back a corresponding output? Read it? Some other thread
can race you for it? (And if you're talking about shell
scripts using it, then processes could race.)

An ioctl-ish interface is superior for some applications.
(not that the one that this patch
lamely attempts to implement is necessarily such an application,
but the array config utility definitely is.) Hopefully
driverfs provides that kind of interface.

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



This archive was generated by hypermail 2b29 : Wed Oct 23 2002 - 22:00:41 EST