Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 25 May 2022 13:20:22 -0400
From:   Kent Overstreet <kent.overstreet@gmail.com>
To:     Theodore Ts'o <tytso@mit.edu>
Cc:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-block@vger.kernel.org, netdev@vger.kernel.org,
        mcgrof@kernel.org
Subject: Re: RFC: Ioctl v2
Message-ID: <20220525172022.ml7lhby2igxtlm7a@moria.home.lan>
References: <20220520161652.rmhqlvwvfrvskg4w@moria.home.lan>
 <YogoI6Augr1V6AHn@mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YogoI6Augr1V6AHn@mit.edu>
Precedence: bulk

On Fri, May 20, 2022 at 07:45:39PM -0400, Theodore Ts'o wrote:
> On Fri, May 20, 2022 at 12:16:52PM -0400, Kent Overstreet wrote:
> > 
> > Where the lack of real namespacing bites us more is when ioctls get promoted
> > from filesystem or driver on up. Ted had a good example of an ext2 ioctl getting
> > promoted to the VFS when it really shouldn't have, because it was exposing ext2
> > specific data structures.
> > 
> > But because this is as simple as changing a #define EXT2_IOC to #define FS_IOC,
> > it's really easy to do without adequate review - you don't have to change the
> > ioctl number and break userspace, so why would you?
> > 
> > Introducing real namespacing would mean that promoting an ioctl to the VFS level
> > would really have to be a new ioctl, and it'll get people to think more about
> > what the new ioctl would be.
> 
> It's not clear that making harder not to break userspace is a
> *feature*.  If existing programs are using a particular ioctl
> namespace, being able to have other file systems adopt it has
> historically been considered a *feature* not a *bug*.
> 
> At the time, we had working utilities, chattr and lsattr, which were
> deployed on all Linux distributions, and newer file systems, such as
> xfs, reiserfs, btrfs, etc., decided they wanted to piggy-back on those
> existing utilities.  Forcing folks to deploy new utilities just
> because it's the best way to force "adequate review" might be swinging
> the pendulum too far in the straight-jacket direction.

But back in those days, users updating those core utilities was a much bigger
hassle. That's changing, we don't really have users compiling from source
anymore, and we're slowly but steadily moving towards the rolling releases
world: slackware -> debian -> nixos.

And on the developer side of things, historically developers have worked on a
few packages where they were comfortable with the process, and packages tended
more towards giant monorepos, but the younger generation is more used to working
across multiple packages/repositories as necessary (this is where github has
been emphatically a good thing, despite being proprietary; it's standardized a
lot of the friction-y "how do I submit to this repo" stuff).

So I understand where you're coming from but I think this is worth rethinking.

Additionally, bikeshedding gets really painful when people are trying to plan
for all eventualities and think too far ahead. Having multiple stages of review
is _helpful_ with this. If an ioctl is filesystem-private, it's perfectly fine
for it embed filesystem specific data structures - if we can ensure that that
won't get lifted to the VFS layer without anyone noticing!

> ...
>
> In the case of extended attributes, we had perfectly working userspace
> tools that would have ***broken*** if we adhered to a doctrinaire,
> when you promote an interface, we break the userspace interface Just
> Because it's the Good Computer Science Thing To Do.

Not broken, though - it just would've needed updating to support additional
filesystems, and when the ioctls don't need changing the patches would be
trivial:

ret = ioctl_xfs_goingdown(..);
becomes
ret = ioctl_fs_goingdown(...) ?:
      ioctl_xfs_goingdown(...);

(I'm the only one I know who does that chaining syntax in C, but I like it :)

> So this approach requires that someone has to actually implement the
> wrapper library.  Who will that be?  The answer could be, "let libc do
> it", but then we need to worry about all the C libraries out there
> actually adopting the new ioctl, which takes a long time, and
> historically, some C library maintainers have had.... opinionated
> points of view about the sort of "value add that should be done at the
> C Library level".

Not libc, and we definitely don't want to have to update that library for every
new ioctl - I'm imagining that library just being responsible for the "query
kernel for ioctl numbers" part, the ioctl definitions themselves will still come
from kernel headers.

> For example, I have an ext2fs library function
> ext2fs_get_device_size2(), which wraps not only the BLKGETSIZE and
> BLKGETSIZE64 ioctls, but also the equivalents for Apple Darwin
> (DKIOCGETBLOCKCOUNT), FreeBSD/NetBSD (DIOCGDINFO and later
> DIOCGMEDIASIZE), and the Window's DeviceIoControl()'s
> IOCTL_DISK_GET_DRIVE_GEOMETRY call.  The point is that wrapper
> functions are very much orthogonal to the ioctl interface; we're all
> going to have wrapper functions, and we'll create them where they are
> needed.

This seems unrelated to the ioctl v2 discussion, but - it would be _great_ if we
could get that in a separate repository where others could make use of it :)