Re: [PATCH 0/5] new dasd ioctl patchkit

Re: [PATCH 0/5] new dasd ioctl patchkit

[Horst Hummel]

> Here's a new patchkit to fix the dasd ioctl mess, against
> 2.6.16-rc2-mm1. I've built an s390 crosscompiler to compile-test them
and
> I've booted the resulting kernel with a debian image in hercules (not
that
> this excercises the ioctl path a whole lot, but I didn't find tools
that
> actually used any of these ioctls).
>
Thanks for doing some test this time. 
If I got that right, there is no big difference related to your last
proposal.

Unfortunately neither me nor Stefan W. got an answer on the question
about being more precise on 'ioctl mess' or any other statements
like 'adds more junk to already crappy code'.

We can't see - and you did not specify - reasons why the current
approach does not work. Therefore I don't see any urgency to change
that NOW instead of discuss the design change (consulting Martin - 
he will be back next week) and do the ioctl change when we have an 
agreed solution including ALL components. 

I agree that you proposal is straight forward and looks more pretty,
but I don't like the approach to just delete code that doesn't fit
your ideas.

As already mentioned we are currently working on a solution to move
the whole cmb-code out of the DASD device driver. 
In addintion I talked to Stefan W. and he is doing some evalutation
about possible solution for the eer_module on how adapt that module
to your proposal while keeping the functionality. 

As I already said, I would like to wait for final solution and 
don't apply the patches NOW.

regards
Horst Hummel

Re: [PATCH 0/5] new dasd ioctl patchkit

[Christoph Hellwig]

On Tue, Feb 14, 2006 at 05:53:08PM +0100, Horst Hummel wrote:
> Unfortunately neither me nor Stefan W. got an answer on the question
> about being more precise on 'ioctl mess' or any other statements
> like 'adds more junk to already crappy code'.
> 
> We can't see - and you did not specify - reasons why the current
> approach does not work. Therefore I don't see any urgency to change
> that NOW instead of discuss the design change (consulting Martin - 
> he will be back next week) and do the ioctl change when we have an 
> agreed solution including ALL components. 

 (1) devices have pretty clear ownership.  Implementing ioctls in
     different modules means we get a very undefined API.  Given that
     ioctls are generally deprecated adding extensions to that is
     a bad idea.
 (2) cleaning up that mess reduced code size a lot
 (3) the cmb module had more glue code than actual ioctl implementation
     which speaks words at length.
 (4) adding new ioctls is not okay in general, you should be using
     better APIs.
 (5) you're not just adding new ioctls but also add them using the
     dynamic registration facility that I NACKed before and remove in
     earlier sent patches, but you copletly ignored all objections
     and just resent the patches anyway
 (6) in addition to the dynamic ioctls it's also adding a misc device,
     leading to a totally incoherent interface.

> I agree that you proposal is straight forward and looks more pretty,
> but I don't like the approach to just delete code that doesn't fit
> your ideas.

It's not code that doesn't fit mu ideas but code that was rushed in
despite my veto for good reasons.

> As I already said, I would like to wait for final solution and 
> don't apply the patches NOW.

that's totally fine with me.  As long as we backout the bogus eer
patch before 2.6.16 all the cleanups and even the eckd ioctl fix
can wait.  But don't put this crappy interface into 1.6.16 and thus
SLES10 so that applications start to rely on it.

Re: [PATCH 0/5] new dasd ioctl patchkit

[Carsten Otte]

Christoph Hellwig wrote:
> As long as we backout the bogus eer
> patch before 2.6.16 all the cleanups and even the eckd ioctl fix
> can wait.  But don't put this crappy interface into 1.6.16 and thus
> SLES10 so that applications start to rely on it.
ACK: Given that a) both Horst and Christoph think the ioctl interface
needs cleanup but proposed cleanup interfers with existing
functionality (cmb), and b) later cleanup would change the
user-interface of eer, we should rush neither the ioctl change nor
eer into .16 until the maintainer is back afaics.

Carsten
-- 

Carsten Otte
IBM Linux technology center
ARCH=s390

Re: [PATCH 0/5] new dasd ioctl patchkit

[Andrew Morton]

Carsten Otte <cotte@de.ibm.com> wrote:
>
> Christoph Hellwig wrote:
> > As long as we backout the bogus eer
> > patch before 2.6.16 all the cleanups and even the eckd ioctl fix
> > can wait.  But don't put this crappy interface into 1.6.16 and thus
> > SLES10 so that applications start to rely on it.
> ACK: Given that a) both Horst and Christoph think the ioctl interface
> needs cleanup but proposed cleanup interfers with existing
> functionality (cmb), and b) later cleanup would change the
> user-interface of eer, we should rush neither the ioctl change nor
> eer into .16 until the maintainer is back afaics.
> 

I don't have a patch in hand to purely back out the eer modeule.  What I
have is

dasd-cleanup-dasd_ioctl.patch
dasd-cleanup-dasd_ioctl-fix.patch
dasd-add-per-disciple-ioctl-method.patch
dasd-merge-dasd_cmd-into-dasd_mod.patch
dasd-backout-dasd_eer-module.patch
dasd-kill-dynamic-ioctl-registration.patch

So what do we do?

Re: [PATCH 0/5] new dasd ioctl patchkit

[Heiko Carstens]

> > > As long as we backout the bogus eer
> > > patch before 2.6.16 all the cleanups and even the eckd ioctl fix
> > > can wait.  But don't put this crappy interface into 1.6.16 and thus
> > > SLES10 so that applications start to rely on it.
> > ACK: Given that a) both Horst and Christoph think the ioctl interface
> > needs cleanup but proposed cleanup interfers with existing
> > functionality (cmb), and b) later cleanup would change the
> > user-interface of eer, we should rush neither the ioctl change nor
> > eer into .16 until the maintainer is back afaics.
> > 
> 
> I don't have a patch in hand to purely back out the eer modeule.  What I
> have is
> 
> dasd-cleanup-dasd_ioctl.patch
> dasd-cleanup-dasd_ioctl-fix.patch
> dasd-add-per-disciple-ioctl-method.patch
> dasd-merge-dasd_cmd-into-dasd_mod.patch
> dasd-backout-dasd_eer-module.patch
> dasd-kill-dynamic-ioctl-registration.patch
> 
> So what do we do?

Looks like people still haven't agreed. If they _finally_ agree on something
and the result is that the eer should be backed out, I will send that patch.
Sorry... I'm just the stupid patch monkey here.

Heiko

Re: [PATCH 0/5] new dasd ioctl patchkit

[Stefan Weinhuber]

Christoph Hellwig <hch@lst.de> wrote on 14.02.2006 20:09:09:

>  (1) devices have pretty clear ownership.  Implementing ioctls in
>      different modules means we get a very undefined API.  Given that
>      ioctls are generally deprecated adding extensions to that is
>      a bad idea.

So you are saying: 'If it's a dasd function, do it in the dasd 
driver!'. Right? 
Well, I can understand that point. As the invocation of the new 
ioctls is actually changing the behavior of a dasd device, I'm
looking into moving that part from dasd_eer into the main dasd module. 
Since this will remove the dynamic ioctl registration as well, it
will also remove the conflict with your cleanup code.

>  (4) adding new ioctls is not okay in general, you should be using
>      better APIs.

Hm, you suggested sysfs in a previous mail. Once I have moved
the eerset/eerget logic into the main dasd driver, I could provide
a sysfs interface instead of the ioctls (right now that would be
difficult).

But I'd like to know why an ioctl is so bad. I know that the ioctl
interface as such can be error prone, but in our case we just 
transfer an integer back and forth. Sysfs always struck me as an
end user interface, something that a human can use with echo and 
cat instead of specialized tools. The new ioctls however should not
be used directly, but by a tool or daemon that knows what it does
and can handle the changed behavior of the dasd device. 
Until now we have always implemented functionality like that as
ioctls (e.g. quiesce/resume, reserve/release) so the new ioctls are
more consistent with the existing interface, and I'd like to keep it
that way. 

>  (5) you're not just adding new ioctls but also add them using the
>      dynamic registration facility that I NACKed before and remove in
>      earlier sent patches, but you copletly ignored all objections
>      and just resent the patches anyway

As I said, the dynamic ioctls can be removed from my code.
(I don't want to dwell on it, but your first NACK against dasd_eer
was because it added code to compat_ioctl.c. I wasn't aware of your
discussion with Martin about the dynamic ioctls.) 

>  (6) in addition to the dynamic ioctls it's also adding a misc device,
>      leading to a totally incoherent interface.

Well, the dasd ioctl will be moved so the misc device will be the
only interface implemented by dasd_eer. 

> that's totally fine with me.  As long as we backout the bogus eer
> patch before 2.6.16 all the cleanups and even the eckd ioctl fix
> can wait.  But don't put this crappy interface into 1.6.16 and thus
> SLES10 so that applications start to rely on it.

As said above, I still prefer the ioctls over a sysfs interface.
Since the other issues do not cause interface changes I would 
prefer to keep the code in. 

Best Regards /  Mit freundlichen Grüßen

Stefan Weinhuber

-------------------------------------------------------------------
IBM Deutschland Entwicklung GmbH
Linux for zSeries Development & Services

[PATCH 0/5] new dasd ioctl patchkit

[Christoph Hellwig]

Here's a new patchkit to fix the dasd ioctl mess, against
2.6.16-rc2-mm1.  I've built an s390 crosscompiler to compile-test them and
I've booted the resulting kernel with a debian image in hercules (not that
this excercises the ioctl path a whole lot, but I didn't find tools that
actually used any of these ioctls).

The patches are also split more fine-grained than last time.

Re: [PATCH 0/5] new dasd ioctl patchkit

[Heiko Carstens]

> Here's a new patchkit to fix the dasd ioctl mess, against
> 2.6.16-rc2-mm1.  I've built an s390 crosscompiler to compile-test them and
> I've booted the resulting kernel with a debian image in hercules (not that
> this excercises the ioctl path a whole lot, but I didn't find tools that
> actually used any of these ioctls).
> 
> The patches are also split more fine-grained than last time.

I leave it up to the dasd people (now on cc) to comment on your patches.

Thanks,
Heiko