Re: [PATCH 0/23] File descriptor hot-unplug support v2

[ebiederm@xmission.com (Eric W. Biederman)]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Mon, Jun 01, 2009 at 02:45:17PM -0700, Eric W. Biederman wrote:
>> 
>> I found myself looking at the uio, seeing that it does not support pci
>> hot-unplug, and thinking "Great yet another implementation of
>> hotunplug logic that needs to be added".
>> 
>> I decided to see what it would take to add a generic implementation of
>> the code we have for supporting hot unplugging devices in sysfs, proc,
>> sysctl, tty_io, and now almost in the tun driver.
>> 
>> Not long after I touched the tun driver and made it safe to delete the
>> network device while still holding it's file descriptor open I someone
>> else touch the code adding a different feature and my careful work
>> went up in flames.  Which brought home another point at the best of it
>> this is ultimately complex tricky code that subsystems should not need
>> to worry about.
>> 
>> What makes this even more interesting is that in the presence of pci
>> hot-unplug it looks like most subsystems and most devices will have to
>> deal with the issue one way or another.
>> 
>> This infrastructure could also be used to implement both force
>> unmounts and sys_revoke.  When I could not think of a better name for
>> I have drawn on that and used revoke.
>
> To be honest, the longer I'm looking at it, the less I like the approach...
> It really looks as if we'd be much better off with functionality sitting
> in a set of library helpers to be used by instances that need this stuff.
> Do we really want it for generic case?

I think so.  I do know I have seen enough weird cases actually being
used and not being done correctly we want a clean pattern for handling
the general case that works and is complete.

The problem seems to break up into several pieces.
- unmap support.
- Getting a list of the files that are open for an inode.
- Waking up interruptible sleepers.
- A test to see if we are executing any of the functions in 
  the file_operations structure. (needed before we can free state)
- Calling frelease and generally releasing of the state held by the
  file.

It might be possible to solve the entire problem outside of the vfs

> Note that "we might someday implement real force-umount" doesn't count;
> the same kind of arguments had been given nine years ago in case of AIO
> ("oh, sure, we'll eventually cover foo_get_block() too - it will all be
> a state machine, fully asynchronous; whaddya mean 'it's not feasible'?").
> Of course, it was _not_ feasible and had never been implemented.

> Frankly, I very much suspect that force-umount is another case like that;
> we'll need a *lot* of interesting cooperation from fs for that to work and
> to be useful.  I'd be delighted to be proven incorrect on that one, so
> if you have anything serious in that direction, please share the details.

So far nothing but thought experiments, but you have a good point at
least a proof of concept should be done of the various pieces.  To flush
out some niggling little detail that messes up the design.

So I hereby sign up for writing a sys_revoke patch, a forced umount patch
and a writing a patch to ext2 to support it.  Supporting proc and
sysfs while easy is not really the common case of an nfs exportable block
filesystem so it is not complete.

> As for the patchset in the current form...  Could you explain what's to prevent
> POSIX locks and dnotify entries from outliving a struct file you'd revoked,
> seeing that filp_close() will skip killing them in that case.

Good catch that looks like a big fat bug to me.  It seems I overlooked
the fact that we actually free things in filp_close.

Given that posix_remove_file calls vfs_lock_file which calls
file->f_op->lock it looks like something really needs to be done here.

dnotify_flush doesn't look to hard to spin a special case for revoke.

I am going to have to spend I while longer studying the rest of the
code in filp_close.  I hope I don't need to figure out the various
fl_owner_t values to safely revoke a file, but it looks like I might.

Eric