breaking leases on metadata changes

Eventually we should probably do the same thing to the file operations as well.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/locks.c |   98 ++++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 62 insertions(+), 36 deletions(-)

fs/locks.c +55 -36

index 9b8408e..b342902 100644
@@ -1352,18 +1352,7 @@ int fcntl_getlease(struct file *filp)
 	return type;
 }
 
-/**
- *	generic_setlease	-	sets a lease on an open file
- *	@filp: file pointer
- *	@arg: type of lease to obtain
- *	@flp: input - file_lock to use, output - file_lock inserted
- *
- *	The (input) flp->fl_lmops->lm_break function is required
- *	by break_lease().
- *
- *	Called with file_lock_lock held.
- */
-int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
+int generic_add_lease(struct file *filp, long arg, struct file_lock **flp)
 {
 	struct file_lock *fl, **before, **my_before = NULL, *lease;
 	struct dentry *dentry = filp->f_path.dentry;

@@ -1372,30 +1361,14 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 
 	lease = *flp;
 
-	error = -EACCES;
-	if ((current_fsuid() != inode->i_uid) && !capable(CAP_LEASE))
-		goto out;
-	error = -EINVAL;
-	if (!S_ISREG(inode->i_mode))
+	error = -EAGAIN;
+	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
 		goto out;
-	error = security_file_lock(filp, arg);
-	if (error)
+	if ((arg == F_WRLCK)
+	    && ((dentry->d_count > 1)
+		|| (atomic_read(&inode->i_count) > 1)))
 		goto out;
 
-	time_out_leases(inode);
-
-	BUG_ON(!(*flp)->fl_lmops->lm_break);
-
-	if (arg != F_UNLCK) {
-		error = -EAGAIN;
-		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
-			goto out;
-		if ((arg == F_WRLCK)
-		    && ((dentry->d_count > 1)
-			|| (atomic_read(&inode->i_count) > 1)))
-			goto out;
-	}
-
 	/*
 	 * At this point, we know that if there is an exclusive
 	 * lease on this file, then we hold it on this filp

@@ -1433,9 +1406,6 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 		goto out;
 	}
 
-	if (arg == F_UNLCK)
-		goto out;
-
 	error = -EINVAL;
 	if (!leases_enable)
 		goto out;

@@ -1446,6 +1416,62 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 out:
 	return error;
 }
+
+int generic_delete_lease(struct file *filp, struct file_lock **flp)
+{
+	struct file_lock *fl, **before;
+	struct dentry *dentry = filp->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+
+	for (before = &inode->i_flock;
+			((fl = *before) != NULL) && IS_LEASE(fl);
+			before = &fl->fl_next) {
+		if (fl->fl_file != filp)
+			continue;
+		return (*flp)->fl_lmops->lm_change(before, F_UNLCK);
+	}
+	return -EAGAIN;
+}
+
+/**
+ *	generic_setlease	-	sets a lease on an open file
+ *	@filp: file pointer
+ *	@arg: type of lease to obtain
+ *	@flp: input - file_lock to use, output - file_lock inserted
+ *
+ *	The (input) flp->fl_lmops->lm_break function is required
+ *	by break_lease().
+ *
+ *	Called with file_lock_lock held.
+ */
+int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+	int error;
+
+	if ((current_fsuid() != inode->i_uid) && !capable(CAP_LEASE))
+		return -EACCES;
+	if (!S_ISREG(inode->i_mode))
+		return -EINVAL;
+	error = security_file_lock(filp, arg);
+	if (error)
+		return error;
+
+	time_out_leases(inode);
+
+	BUG_ON(!(*flp)->fl_lmops->lm_break);
+
+	switch (arg) {
+	case F_UNLCK:
+		return generic_delete_lease(filp, flp);

Quoted 2 lines · J. Bruce Fields

Eventually we should probably do the same thing to the file operations as well.

Nice, the code refactoring makes it a lot more readable.

NFSv4 uses leases to guarantee that clients can cash metadata as well as

data.

(I suspect the same is true for Samba.)

This covers chmod, chown, etc.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/attr.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

fs/attr.c +3 -0

index 538e279..4ce31cb 100644
@@ -231,6 +231,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
 	error = security_inode_setattr(dentry, attr);
 	if (error)
 		return error;
+	error = break_lease(inode, O_WRONLY);
+	if (error)
+		return error;
 
 	if (inode->i_op->setattr)
 		error = inode->i_op->setattr(dentry, attr);

Signature · 1 line

1.7.4.1

In setlease, we use i_writecount to decide whether we can give out a read lease.

In open, we break leases before incrementing i_writecount.

There is therefore a window between the break lease and the i_writecount increment when setlease could add a new read lease.

This would leave us with a simultaneous write open and read lease, which shouldn't happen.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/namei.c |    5 +----
 fs/open.c  |    4 ++++
 2 files changed, 5 insertions(+), 4 deletions(-)

fs/namei.c +1 -4

index 2826db3..6ff59e5 100644
@@ -2044,10 +2044,7 @@ static int may_open(struct path *path, int acc_mode, int flag)
 	if (flag & O_NOATIME && !inode_owner_or_capable(inode))
 		return -EPERM;
 
-	/*
-	 * Ensure there are no outstanding leases on the file.
-	 */
-	return break_lease(inode, flag);
+	return 0;
 }
 
 static int handle_truncate(struct file *filp)

fs/open.c +4 -0

index f711921..22c41b5 100644
@@ -685,6 +685,10 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
 	if (error)
 		goto cleanup_all;
 
+	error = break_lease(inode, f->f_flags);
+	if (error)
+		goto cleanup_all;
+
 	if (!open && f->f_op)
 		open = f->f_op->open;
 	if (open) {

Signature · 1 line

1.7.4.1

Quoted 8 lines · J. Bruce Fields

In setlease, we use i_writecount to decide whether we can give out a read lease.

In open, we break leases before incrementing i_writecount.

There is therefore a window between the break lease and the i_writecount increment when setlease could add a new read lease.

Quoted 2 lines

This would leave us with a simultaneous write open and read lease, which shouldn't happen.

And maybe someone that knows the open code better than me could confirm whether it's reasonable to move the break_lease() call to __dentry_open like this....

--b.

Quoted 41 lines

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/namei.c |    5 +----
 fs/open.c  |    4 ++++
 2 files changed, 5 insertions(+), 4 deletions(-)

fs/namei.c +1 -4

index 2826db3..6ff59e5 100644
@@ -2044,10 +2044,7 @@ static int may_open(struct path *path, int acc_mode, int flag)
 	if (flag & O_NOATIME && !inode_owner_or_capable(inode))
 		return -EPERM;
 
-	/*
-	 * Ensure there are no outstanding leases on the file.
-	 */
-	return break_lease(inode, flag);
+	return 0;
 }
 
 static int handle_truncate(struct file *filp)

fs/open.c +4 -0

index f711921..22c41b5 100644
@@ -685,6 +685,10 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
 	if (error)
 		goto cleanup_all;
 
+	error = break_lease(inode, f->f_flags);
+	if (error)
+		goto cleanup_all;
+
 	if (!open && f->f_op)
 		open = f->f_op->open;
 	if (open) {

Signature · 1 line

1.7.4.1

In setlease, we use i_writecount to decide whether we can give out a read lease.

In open, we break leases before incrementing i_writecount.

There is therefore a window between the break lease and the i_writecount increment when setlease could add a new read lease.

This would leave us with a simultaneous write open and read lease, which shouldn't happen.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/namei.c |    5 +----
 fs/open.c  |    4 ++++
 2 files changed, 5 insertions(+), 4 deletions(-)

fs/namei.c +1 -4

index 2826db3..6ff59e5 100644
@@ -2044,10 +2044,7 @@ static int may_open(struct path *path, int acc_mode, int flag)
 	if (flag & O_NOATIME && !inode_owner_or_capable(inode))
 		return -EPERM;
 
-	/*
-	 * Ensure there are no outstanding leases on the file.
-	 */
-	return break_lease(inode, flag);
+	return 0;
 }
 
 static int handle_truncate(struct file *filp)

fs/open.c +4 -0

index f711921..22c41b5 100644
@@ -685,6 +685,10 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
 	if (error)
 		goto cleanup_all;
 
+	error = break_lease(inode, f->f_flags);
+	if (error)
+		goto cleanup_all;
+
 	if (!open && f->f_op)
 		open = f->f_op->open;
 	if (open) {

Signature · 1 line

1.7.4.1

Quoted 5 lines · J. Bruce Fields

In setlease, we use i_writecount to decide whether we can give out a read lease.

In open, we break leases before incrementing i_writecount.

Quoted 5 lines

There is therefore a window between the break lease and the i_writecount increment when setlease could add a new read lease.

This would leave us with a simultaneous write open and read lease, which shouldn't happen.

Al, could you apply this for 3.2, if you don't see any problem?

(Patch 1 of this series only touches locks.c, and I'm queueing up such patches through the nfsd tree. Patches 3-6 I intend to rewrite, probably not in time for 3.2 unless I'm very lucky.)

--b.

Quoted 31 lines

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/namei.c |    5 +----
 fs/open.c  |    4 ++++
 2 files changed, 5 insertions(+), 4 deletions(-)

fs/namei.c +1 -4

index 2826db3..6ff59e5 100644
@@ -2044,10 +2044,7 @@ static int may_open(struct path *path, int acc_mode, int flag)
 	if (flag & O_NOATIME && !inode_owner_or_capable(inode))
 		return -EPERM;
 
-	/*
-	 * Ensure there are no outstanding leases on the file.
-	 */
-	return break_lease(inode, flag);
+	return 0;
 }
 
 static int handle_truncate(struct file *filp)

fs/open.c +0 -0

index f711921..22c41b5 100644
@@ -685,6 +685,10 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
 	if (error)
 		goto cleanup_all;
 

Quoted 10 lines

fs/open.c +4 -0

+	error = break_lease(inode, f->f_flags);
+	if (error)
+		goto cleanup_all;
+
 	if (!open && f->f_op)
 		open = f->f_op->open;
 	if (open) {

Signature · 1 line

1.7.4.1

A read lease is used by NFSv4 as a guarantee that a client can perform local read opens without informing the server.

The open operation takes the last component of the pathname as an argument, thus is also a lookup operation, and giving the client the above guarantee means informing the client before we allow anything that would change the set of names pointing to the inode.

Quoted 7 lines · J. Bruce Fields

A read lease is used by NFSv4 as a guarantee that a client can perform local read opens without informing the server.

The open operation takes the last component of the pathname as an argument, thus is also a lookup operation, and giving the client the above guarantee means informing the client before we allow anything that would change the set of names pointing to the inode.

That seems like totally strange semantics. A useful defintion of a lase operation would mean it guarantees I/O can happen, and wouldn't care about metadata operation. That's the whole point in adding a stateful open to nfs4, isn't it?

To rely on the i_mutex for exclusion between setlease and rename, we need rename to take the i_mutex on the source as well as on any possible target.

I suspect this is deadlock-free, but I need to think this proof through again. And I'm not sure what to do about lockdep.

Signed-off-by J. Bruce Fields <[email protected]>

---
 Documentation/filesystems/directory-locking |   11 ++++++-----
 fs/namei.c                                  |   17 +++++++++++++++--
 2 files changed, 21 insertions(+), 7 deletions(-)

Documentation/filesystems/directory-locking +6 -5

index ff7b611..c51cbed 100644
@@ -12,8 +12,8 @@ kinds of locks - per-inode (->i_mutex) and per-filesystem
 locks victim and calls the method.
 
 4) rename() that is _not_ cross-directory.  Locking rules: caller locks
-the parent, finds source and target, if target already exists - locks it
-and then calls the method.
+the parent, finds source and target, locks source, also locks target if
+it already exists, and then calls the method.
 
 5) link creation.  Locking rules:
 	* lock parent

@@ -30,6 +30,7 @@ rules:
 		fail with -ENOTEMPTY
 	* if new parent is equal to or is a descendent of source
 		fail with -ELOOP
+	* lock source if it is not a directory.
 	* if target exists - lock it.
 	* call the method.
 

@@ -56,9 +57,9 @@ objects - A < B iff A is an ancestor of B.
     renames will be blocked on filesystem lock and we don't start changing
     the order until we had acquired all locks).
 
-(3) any operation holds at most one lock on non-directory object and
-    that lock is acquired after all other locks.  (Proof: see descriptions
-    of operations).
+(3) locks on non-directory objects are acquired only after taking locks
+    on their parents (which remain their parents by (1) and (2)).
+    (Proof: see descriptions of operations).
 
 	Now consider the minimal deadlock.  Each process is blocked on
 attempt to acquire some lock and already holds at least one lock.  Let's

fs/namei.c +13 -2

index 5c78f72..c0220f7 100644
@@ -3058,6 +3058,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 			    struct inode *new_dir, struct dentry *new_dentry)
 {
 	struct inode *target = new_dentry->d_inode;
+	struct inode *source = old_dentry->d_inode;
 	int error;
 
 	error = security_inode_rename(old_dir, old_dentry, new_dir, new_dentry);

@@ -3065,13 +3066,23 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 		return error;
 
 	dget(new_dentry);
-	if (target)
+	mutex_lock(&source->i_mutex);
+	error = break_lease(source, O_WRONLY);
+	if (error)
+		goto out_unlock_source;
+	if (target) {
 		mutex_lock(&target->i_mutex);
-
+		error = break_lease(target, O_WRONLY);
+		if (error)
+			goto out;
+	}
 	error = -EBUSY;
 	if (d_mountpoint(old_dentry)||d_mountpoint(new_dentry))
 		goto out;
 
+	error = break_lease(old_dentry->d_inode, O_WRONLY);
+	if (error)
+		goto out;
 	error = old_dir->i_op->rename(old_dir, old_dentry, new_dir, new_dentry);
 	if (error)
 		goto out;

Quoted 6 lines · J. Bruce Fields

To rely on the i_mutex for exclusion between setlease and rename, we need rename to take the i_mutex on the source as well as on any possible target.

I suspect this is deadlock-free, but I need to think this proof through again. And I'm not sure what to do about lockdep.

Not sure that I will be of any help, but how about posting the lockdep messages?

Quoted 10 lines · Mimi Zohar

Quoted 6 lines · J. Bruce Fields

To rely on the i_mutex for exclusion between setlease and rename, we need rename to take the i_mutex on the source as well as on any possible target.

I suspect this is deadlock-free, but I need to think this proof through again. And I'm not sure what to do about lockdep.

Not sure that I will be of any help, but how about posting the lockdep messages?

Sure, appended below, but it's not particularly surprising--we're taking i_mutex's on four different objects (both parents, source, and target if any) where before there were three.

I suppose the solution is another i_mutex lock class, used only on the lock of the source inode?

Quoted 10 lines · Mimi Zohar

Quoted 6 lines · J. Bruce Fields

To rely on the i_mutex for exclusion between setlease and rename, we need rename to take the i_mutex on the source as well as on any possible target.

I suspect this is deadlock-free, but I need to think this proof through again. And I'm not sure what to do about lockdep.

Not sure that I will be of any help, but how about posting the lockdep messages?

Sure, appended below, but it's not particularly surprising--we're taking i_mutex's on four different objects (both parents, source, and target if any) where before there were three.

I suppose the solution is another i_mutex lock class, used only on the lock of the source inode?

--b.

=============================================
[ INFO: possible recursive locking detected ]
3.1.0-rc1-00076-g0e7e722 #599
---------------------------------------------
mount/333 is trying to acquire lock:
(&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff811126d8>] vfs_rename+0x278/0x450

Sep 20 17:05:54 pip1 kernel:
but task is already holding lock:
(&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff81112b03>] sys_renameat+0x253/0x2d0
Sep 20 17:05:54 pip1 kernel:
other info that might help us debug this:
Possible unsafe locking scenario:
Sep 20 17:05:54 pip1 kernel:
      CPU0
      ----
 lock(&sb->s_type->i_mutex_key);
 lock(&sb->s_type->i_mutex_key);
Sep 20 17:05:54 pip1 kernel:
*** DEADLOCK ***
Sep 20 17:05:54 pip1 kernel:
May be due to missing lock nesting notation
Sep 20 17:05:54 pip1 kernel:
2 locks held by mount/333:
#0: (&sb->s_type->i_mutex_key#2/1){+.+.+.}, at: [<ffffffff8110efc8>] lock_rename+0xe8/0xf0
#1:  (&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff81112b03>] sys_renameat+0x253/0x2d0
Sep 20 17:05:54 pip1 kernel:
stack backtrace:
Pid: 333, comm: mount Not tainted 3.1.0-rc1-00076-g0e7e722 #599
Call Trace:
[<ffffffff8107c9df>] __lock_acquire+0x15bf/0x1d80
[<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
[<ffffffff8107d794>] lock_acquire+0x94/0x140
[<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
[<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
[<ffffffff81979d7f>] mutex_lock_nested+0x4f/0x360
[<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
[<ffffffff8103b2b1>] ?  get_parent_ip+0x11/0x50
[<ffffffff8197ed9d>] ?  sub_preempt_count+0x9d/0xd0
[<ffffffff811126d8>] vfs_rename+0x278/0x450
[<ffffffff8103b2b1>] ?  get_parent_ip+0x11/0x50
[<ffffffff81112b5d>] sys_renameat+0x2ad/0x2d0
[<ffffffff810edab3>] ? remove_vma+0x53/0x70
[<ffffffff81079b0d>] ?  trace_hardirqs_on_caller+0xfd/0x190
[<ffffffff81079bad>] ?  trace_hardirqs_on+0xd/0x10
[<ffffffff810edab3>] ? remove_vma+0x53/0x70
[<ffffffff81982998>] ?  sysret_check+0x26/0x60
[<ffffffff81079b0d>] ?  trace_hardirqs_on_caller+0xfd/0x190
[<ffffffff81112b9b>] sys_rename+0x1b/0x20
[<ffffffff81982968>] system_call_fastpath+0x16/0x1b

Quoted 16 lines · J. Bruce Fields

Quoted 10 lines · Mimi Zohar

Quoted 6 lines · J. Bruce Fields

To rely on the i_mutex for exclusion between setlease and rename, we need rename to take the i_mutex on the source as well as on any possible target.

I suspect this is deadlock-free, but I need to think this proof through again. And I'm not sure what to do about lockdep.

Not sure that I will be of any help, but how about posting the lockdep messages?

Sure, appended below, but it's not particularly surprising--we're taking i_mutex's on four different objects (both parents, source, and target if any) where before there were three.

Quoted 4 lines

I suppose the solution is another i_mutex lock class, used only on the lock of the source inode?

--b.

I must not be missing something, but it seems taking the i_mutex here is unnecessary. fs/namei.c:lock_rename() already does all the locking for you.

Mimi

Quoted 50 lines

=============================================
[ INFO: possible recursive locking detected ]
3.1.0-rc1-00076-g0e7e722 #599
---------------------------------------------
mount/333 is trying to acquire lock:
(&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff811126d8>] vfs_rename+0x278/0x450

Sep 20 17:05:54 pip1 kernel:
but task is already holding lock:
(&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff81112b03>] sys_renameat+0x253/0x2d0
Sep 20 17:05:54 pip1 kernel:
other info that might help us debug this:
Possible unsafe locking scenario:
Sep 20 17:05:54 pip1 kernel:
      CPU0
      ----
 lock(&sb->s_type->i_mutex_key);
 lock(&sb->s_type->i_mutex_key);
Sep 20 17:05:54 pip1 kernel:
*** DEADLOCK ***
Sep 20 17:05:54 pip1 kernel:
May be due to missing lock nesting notation
Sep 20 17:05:54 pip1 kernel:
2 locks held by mount/333:
#0: (&sb->s_type->i_mutex_key#2/1){+.+.+.}, at: [<ffffffff8110efc8>] lock_rename+0xe8/0xf0
#1:  (&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff81112b03>] sys_renameat+0x253/0x2d0
Sep 20 17:05:54 pip1 kernel:
stack backtrace:
Pid: 333, comm: mount Not tainted 3.1.0-rc1-00076-g0e7e722 #599
Call Trace:
[<ffffffff8107c9df>] __lock_acquire+0x15bf/0x1d80
[<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
[<ffffffff8107d794>] lock_acquire+0x94/0x140
[<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
[<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
[<ffffffff81979d7f>] mutex_lock_nested+0x4f/0x360
[<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
[<ffffffff8103b2b1>] ?  get_parent_ip+0x11/0x50
[<ffffffff8197ed9d>] ?  sub_preempt_count+0x9d/0xd0
[<ffffffff811126d8>] vfs_rename+0x278/0x450
[<ffffffff8103b2b1>] ?  get_parent_ip+0x11/0x50
[<ffffffff81112b5d>] sys_renameat+0x2ad/0x2d0
[<ffffffff810edab3>] ? remove_vma+0x53/0x70
[<ffffffff81079b0d>] ?  trace_hardirqs_on_caller+0xfd/0x190
[<ffffffff81079bad>] ?  trace_hardirqs_on+0xd/0x10
[<ffffffff810edab3>] ? remove_vma+0x53/0x70
[<ffffffff81982998>] ?  sysret_check+0x26/0x60
[<ffffffff81079b0d>] ?  trace_hardirqs_on_caller+0xfd/0x190
[<ffffffff81112b9b>] sys_rename+0x1b/0x20
[<ffffffff81982968>] system_call_fastpath+0x16/0x1b

So there's this longstanding problem that read leases should really be broken on anything that changes an inode's metadata or any change to the set of links pointing to the inode.

(Cc'ing samba-technical for confirmation of that statement from Samba's point of view....)

So we need mutual exclusion between read leases and link, unlink, rename, etc.

Hi Bruce,

Quoted 7 lines

So there's this longstanding problem that read leases should really be broken on anything that changes an inode's metadata or any change to the set of links pointing to the inode.

(Cc'ing samba-technical for confirmation of that statement from Samba's point of view....)

Quoted 2 lines

So we need mutual exclusion between read leases and link, unlink, rename, etc.

As far as I know oplocks in SMB only care about content and attribute only opens (without the READ/WRITE access bits) doesn't break oplocks.

So changing the modification time of a file should not break the oplock.

I think link and rename doesn't break oplocks, while unlink would.

But Samba requirements may change for SMB 2.2 support with directory leases. We're currently exploring what the details are.

Anyway, if this is all correct then it sounds like v4 read delegations and level 2 oplocks may be more different than I'd thought.