Re: [PATCH 1/2] locks: introduce i_blockleases to close lease races

On Sun, 2011-06-12 at 15:12 -0400, J. Bruce Fields wrote: 

On Sun, Jun 12, 2011 at 03:10:04PM -0400, Mimi Zohar wrote:

quoted

On Sun, 2011-06-12 at 00:08 -0400, J. Bruce Fields wrote:

quoted

On Fri, Jun 10, 2011 at 05:34:46PM -0400, J. Bruce Fields wrote:

quoted

On Fri, Jun 10, 2011 at 04:24:00PM -0400, Mimi Zohar wrote:

quoted

On Thu, 2011-06-09 at 20:10 -0400, J. Bruce Fields wrote:

quoted

From: J. Bruce Fields <redacted>

Since break_lease is called before i_writecount is incremented, there's
a window between the two where a setlease call would have no way to know
that an open is about to happen.

So unless the break_lease() call is moved from may_open() to after 
nameidata_to_filp(), I don't see any other options.

Actually, offhand I can't see why that wouldn't be OK.

Though I think we still end up needing something like i_blockleases to
handle unlink, link, rename, chown, and chmod.

Well, I guess there's a bizarre alternative that wouldn't require a new
inode field:

In lieu of adding a new inode field, another possible option, a bit
kludgy, would be extending i_flock with an additional fl_flag
FL_BLOCKLEASE.

#define IS_BLOCKLEASE(fl)    (fl->fl_flags & FL_BLOCKLEASE)

Alas, that would mean adding and removing one of these file locks around
every single link, unlink, rename,....

--b.

You're adding a call to break_lease() for each of them.  Currently
__break_lease() is only called if a lease exists. Assuming there aren't
any existing leases, couldn't break_lease() call something like
block_lease()?  The free would be after the link, unlink, ...,
completed/failed.

(You wouldn't actually need to alloc/free the 'struct file_lock' each
time, just set the pointer and reset to NULL.)

Mimi

quoted

Mimi

quoted

What we care about is conflicts between read leases and operations that
modify the metadata of the inode or the set of names pointing to it.

As far as I can tell those operations all take the i_mutex either on the
inode itself or on the parents of one of its aliases.

So, you could prevent break_lease/setlease races by calling setlease
under *all* of those i_mutexes:

	- take i_mutex on the inode
	- take i_lock to prevent the set of aliases from changing
	- take i_mutex for parent of each alias
	- set the lease
	- drop the parent i_mutexes, etc.

where the i_mutexes would all be taken with mutex_trylock, and we'd just
fail the whole setlease if any of them failed.

???

--b.