[RFC] next cycle fun: ->release() API change

[RFC] next cycle fun: ->release() API change

[Al Viro]

	This is going to be a major PITA, but surprisingly it seems to be
more or less feasible.  Background: ->release() method in file_operations
has wrong prototype.
	0.96a: void (*release)(struct inode *, struct file *) introduced
as abstraction for minix_release (same type).  Called by sys_close().
	0.97.1: call site moved to close_fp() (modern filp_close())
	2.0.14: we'd got void {__fput,fput}(struct inode *, struct file *)
as wrappers for ->release(), still called by close_fp()
	2.1.31: mistake - ->release() started to return int, along with
fput()/__fput().  Practically all instances always return 0.  The main
reason seems to have been NFS - it didn't get ->release() until the next
version, but that happened very soon.
	2.1.45pre4: fput() and __fput() are switched to taking just the
struct file * (happened while introducing dcache).
	2.1.118: ->flush() introduced; close_fp() calls it before
fput() and uses its return value.  fput() and __fput() are returning
void now; ->release() is _still_ returning int.  NFS has promptly added
->flush() and in 2.2.early dropped ->release().

	From that point on the return value of ->release() had been completely
lost.  Most of the instances are still returning 0 (some - via several layers
of methods in subsystems/subsubsystems/drivers/etc.).  Exceptions tend to
point to confusion in the best case, outright broken drivers in the worst.
And people _do_ get confused by that thing.  A couple of months ago one
such case happened, getting two replies: mine along the lines of "->release()
type is a mistake, too painful to fix by now" and Linus' "we should just make
->release return void", both with similar explanations of the reasons
why void would be the right return type.

	OK, so I decided to check just how painful would it be to fix that.
Diff had grown well past anything even remotely reasonable for a single
commit (443 files changed, 814 insertions(+), 1843 deletions(-) for the
last snapshot I've taken, just before going to LSFS; about one third of
megabyte worth of diff, roughly a quarter of drivers/* remaining to be
converted).  And, naturally enough, a bunch of bugfixes kept calving off
from it.  That was a flagday approach - ->release() switched to
void (*release)(struct file *) (struct inode * is both redundant and
unused by the majority of instances; ones that do use it can simply use
file_inode(file)).  Originally I planned to ask Linus to merge that monster
as an after-rc1 special, but it was obviously far too large for that.

	The next plan was to make the thing splittable.  I.e. start with
adding replacement method (I ended up calling it ->close(), suggestions for
better names are welcome), teach the (very few) call sites of ->release()
to try that if available, then convert the users of widely used instances
to new method (seq_release and friends, mostly), then go driver by driver
converting them.  The current variant (still growing - most of the drivers/
and arch/ is yet to be converted) is in vfs.git#close and at the moment it's
at 45 commits, with 618 files changed, 1428 insertions(+), 1872 deletions(-),
Amount of churn is greater that way, of course ;-/ The final commit once it's
done will be removal of ->release().

	As far as I can see, it's survivable, it kills a lot of boilerplate
code and result makes more sense.  So it might make sense to do that
conversion.  Sure, it hurts binary modules.  So what?

	_If_ we go for that, the question is how to do the merge.  Even split
into smaller chunks (and even if I get it finished by Monday), it obviously
will not be in shape for just-after-rc1 one-time merge.  AFAICS, there are
two variants:
	1) beginning of that series gets pushed into mainline - new method
introduction + seq_file-related tree-wide stuff.  The rest sits in a branch
in vfs.git.  Maintainers of other trees are welcome to cherry-pick the stuff
relevant to their trees whenever they wish and tell me to drop the duplicate
from that queue.  Or do the conversion themselves in whatever way they prefer,
and tell me to drop my variant.  Or just tell me which commits they might
want, so that I would put them into a never-rebased branch for them to pull,
etc.
	2) beginning of that series is left in vfs.git in never-rebased
branch, with folks being welcome to merge it into their trees.  The rest
as in (1).

	I thought of doing it the way I'd done kernel_thread transition, but
I don't think I can pull that off - there are far more affected trees than
twenty-odd involved there, and it had been hard enough even back then ;-/
Hell knows...

	Any comments and suggestions are welcome.

Re: [RFC] next cycle fun: ->release() API change

[Linus Torvalds]

On Wed, May 8, 2013 at 10:03 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
>         OK, so I decided to check just how painful would it be to fix that.
> Diff had grown well past anything even remotely reasonable for a single
> commit (443 files changed, 814 insertions(+), 1843 deletions(-) for the
> last snapshot I've taken, just before going to LSFS; about one third of
> megabyte worth of diff, roughly a quarter of drivers/* remaining to be
> converted).  And, naturally enough, a bunch of bugfixes kept calving off
> from it.  That was a flagday approach - ->release() switched to
> void (*release)(struct file *) (struct inode * is both redundant and
> unused by the majority of instances; ones that do use it can simply use
> file_inode(file)).  Originally I planned to ask Linus to merge that monster
> as an after-rc1 special, but it was obviously far too large for that.
>
>         The next plan was to make the thing splittable.  I.e. start with
> adding replacement method (I ended up calling it ->close(), suggestions for
> better names are welcome), teach the (very few) call sites of ->release()
> to try that if available, then convert the users of widely used instances
> to new method (seq_release and friends, mostly), then go driver by driver
> converting them.

Ugh. You know what? I'd almost prefer to just do it as a single big
commit, *without* the extra churn of then also renaming things.
Because the renaming will just be painful and result in *more*
problems, and quite frankly, this particular ABI change is "benign" in
the sense that unconverted drivers will just cause warnings - the code
will still compile (module -Werror, of course, which some
architectures use) and still work perfectly fine (modulo crazy C paper
standard issues that have nothing to do with actual reality).

In fact, the "it still works fine, just complains" makes it perfecly
reasonable to even split it up into multiple independent commits. So
rather than do the stupid renaming, I'd be perfectly happy with one
commit that just changes "int ("release)(..)" to "void (*release)(..)"
and then a boatload of "remove return value from release in
drivers/block/*" kind of commits that do the conversion.

Because renaming really doesn't buy us anything but pain.

                Linus

Re: [RFC] next cycle fun: ->release() API change

[Al Viro]

On Sat, May 11, 2013 at 10:05:05AM -0700, Linus Torvalds wrote:

> Ugh. You know what? I'd almost prefer to just do it as a single big
> commit, *without* the extra churn of then also renaming things.
> Because the renaming will just be painful and result in *more*
> problems, and quite frankly, this particular ABI change is "benign" in
> the sense that unconverted drivers will just cause warnings - the code
> will still compile (module -Werror, of course, which some
> architectures use) and still work perfectly fine (modulo crazy C paper
> standard issues that have nothing to do with actual reality).
> 
> In fact, the "it still works fine, just complains" makes it perfecly
> reasonable to even split it up into multiple independent commits. So
> rather than do the stupid renaming, I'd be perfectly happy with one
> commit that just changes "int ("release)(..)" to "void (*release)(..)"
> and then a boatload of "remove return value from release in
> drivers/block/*" kind of commits that do the conversion.
> 
> Because renaming really doesn't buy us anything but pain.

Umm...  I'd rather go the whole way and get rid of inode argument as well,
while we are at it.  It's completely redundant and it's unused in very large
majority of the instances.

Re: [RFC] next cycle fun: ->release() API change

[Linus Torvalds]

On Sat, May 11, 2013 at 10:22 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>
>> Because renaming really doesn't buy us anything but pain.
>
> Umm...  I'd rather go the whole way and get rid of inode argument as well,
> while we are at it.  It's completely redundant and it's unused in very large
> majority of the instances.

So? What's the advantage of removing it?

Also, "->close()" would be *exactly* the wrong name to call this,
since it would be absolutely and utterly misleading. "->release()" is
_not_ about close, and in fact the whole return code is partially due
to people thinking it is. It's "->flush()" that gets called at close
time.

               Linus

Re: [RFC] next cycle fun: ->release() API change

[Al Viro]

On Sat, May 11, 2013 at 12:16:22PM -0700, Linus Torvalds wrote:

> >> Because renaming really doesn't buy us anything but pain.
> >
> > Umm...  I'd rather go the whole way and get rid of inode argument as well,
> > while we are at it.  It's completely redundant and it's unused in very large
> > majority of the instances.
> 
> So? What's the advantage of removing it?

Less boilerplate?  We used to pass inode to fput() as well, but
switched to passing file alone...  Put it that way - if you were adding
such method today, would you bother with passing an argument that is
unused by most of the instances and can be trivially obtained from the
argument we have to pass?  Sure, back then you had the opposite
situation - all non-empty instances were using inode (and none of them
even looked at file), but with the mix we have these days...
Hell knows; I agree that if we really try to preserve the interface
compatibility here, just switching the return type wins, but since
the conversion for every particular driver is trivial anyway...

> Also, "->close()" would be *exactly* the wrong name to call this,
> since it would be absolutely and utterly misleading. "->release()" is
> _not_ about close, and in fact the whole return code is partially due
> to people thinking it is. It's "->flush()" that gets called at close
> time.

Agreed - I'd welcome any alternative suggestions, and yes, I understand your
point re "just call it 'release'".

Re: [RFC] next cycle fun: ->release() API change

[Linus Torvalds]

On Sat, May 11, 2013 at 2:06 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Less boilerplate?  We used to pass inode to fput() as well, but
> switched to passing file alone...

.. and that was painful.

The advantage has to be balanced against the pain it causes. I'm not
seeing the advantage here as being worth it. If this kind of thing not
only causes way more churn, _and_ it causes us to pick a new (worse)
name just because it also forces a non-compatible ABI, I'm really
doubtful.

I mean, if we had *other* reasons for the churn, and the name needed
to change anyway, then maybe, but..

             Linus

Re: [RFC] next cycle fun: ->release() API change

[Al Viro]

On Sat, May 11, 2013 at 02:12:25PM -0700, Linus Torvalds wrote:
> On Sat, May 11, 2013 at 2:06 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > Less boilerplate?  We used to pass inode to fput() as well, but
> > switched to passing file alone...
> 
> .. and that was painful.
> 
> The advantage has to be balanced against the pain it causes. I'm not
> seeing the advantage here as being worth it. If this kind of thing not
> only causes way more churn, _and_ it causes us to pick a new (worse)
> name just because it also forces a non-compatible ABI, I'm really
> doubtful.
> 
> I mean, if we had *other* reasons for the churn, and the name needed
> to change anyway, then maybe, but..

Hell knows...  FWIW, most of the work in that series is reviewing the
instances for locking and leaks - IOW, the pieces that are dropping off
into separate patches anyway.  With all of them done it'll probably
boil down to something less painful.

BTW, I'm not all that fond of 'close', for the reasons you've mentioned.
OTOH, 'release' has another problem - it's probably the most common method
name, making it very hard to grep for.  We have 60-odd structures with method
called release, many of those with widespread instances.  In include/linux
alone we have
af_alg_type
drm_global_reference
block_device_operations
cdrop_device_ops
bsg_class_device
cftype
configfs_item_operations
device_type
device
dma_buf_ops
file_operations
hsi_port
irq_affinity_notify
ipack_device
iscsi_boot_kobj
kobj_type
loop_func_table	(what the devil is _that_ doing in include/linux?)
mmu_notifier_ops
mtd_blktrans_ops
proto_ops
nfs_gpio_header
hotplug_slot
pipe_buf_operations
posix_clock_operations
posix_clock
rtc_class_ops
auth_ops
uio_info
usb_serial_driver
vfio_device_ops
vfio_iommu_driver_ops
media_file_operations
media_devnode
saa7146_use_ops
v4l2_file_operations
video_device
v4l2_device
cfsrvl	(jst wht th fck hd th thr f tht sht bn smkng?)
tcp_congestion_ops
snd_emux_operators
snd_hwdep_ops
sound_info_entry_ops
and while some of those don't have a lot of instances, some do.  I'm not
saying that going back to original K&R "struct members form a single
namespace and must be unique across different structures" would be a good
idea, but this one is inconveniently common - makes finding the instances
and call sites a very unpleasant work ;-/

BTW, a lot of those guys are returning void, but there are some that return
int and I think we ought to review those as well.  And that's probably
worth doing *before* we start merging file_operations ->release() change,
whether it's just int->void variant or anything more ambitious.

I don't know; let's hold that off for now and see how much calves off
that patchset.  If we go for your 'switch the return type of ->release()
and then deal with the instances', we can do that in 3.11-rc1 as well as
in 3.10-rc1...

Re: [RFC] next cycle fun: ->release() API change

[Al Viro]

On Sun, May 12, 2013 at 01:06:16AM +0100, Al Viro wrote:
> media_file_operations
> v4l2_file_operations
> snd_hwdep_ops
> sound_info_entry_ops
> proto_ops
> auth_ops

> BTW, a lot of those guys are returning void, but there are some that return
> int and I think we ought to review those as well.  And that's probably
> worth doing *before* we start merging file_operations ->release() change,
> whether it's just int->void variant or anything more ambitious.

Having looked through some of those:
	* cftype->release() is non-NULL only in two instances *and* the
only caller of that method is never called for those instances.  WTF does
it exist, in the first place?  NB: the set of cftype methods is unspeakably
ugly; check it and puke.
	* hsi_port->release(): AFAICS, return value is ignored by the sole
caller.  No non-trivial instances in the tree (again, AFAICS) - the only one
is "do nothing and return 0".
	* loop_func_table->release(): return value is ignored by most of that
callers; the only in-tree instance returns non-zero only if it sees an obvious
result of memory corruption.
	* posix_clock_operations->release(): NULL in the only in-tree instance
of struct posix_clock_operations.  The only caller is posix_clock_release()
and return value is passed to its caller, which drops it on the floor.
	* uio_info->release(): AFAICS, there are only 3 instances in the
tree.  All are always returning 0; incidentally, none of them ever looks
at the second argument of that method (it's int (*release)(struct uio_info *,
struct inode *)).  The only caller is uio_release(), which passes the
return value to its caller, which drops it on the floor.