When the process is blocked on mandatory lock and someone changes

the inode's permissions, so that the lock is no longer mandatory,

nobody wakes up the blocked process, but probably should.
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
diff --git a/fs/attr.c b/fs/attr.c

index ae58bd3..7a8506f 100644

--- a/fs/attr.c

+++ b/fs/attr.c

@@ -104,7 +104,7 @@ int notify_change(struct dentry * dentry

 {

 	struct inode *inode = dentry->d_inode;

 	mode_t mode;

-	int error;

+	int error, mandatory;

 	struct timespec now;

 	unsigned int ia_valid = attr->ia_valid;
@@ -151,6 +151,8 @@ int notify_change(struct dentry * dentry

 	if (ia_valid & ATTR_SIZE)

 		down_write(&dentry->d_inode->i_alloc_sem);
+	mandatory = (inode->i_flock && MANDATORY_LOCK(inode));

+

 	if (inode->i_op && inode->i_op->setattr) {

 		error = security_inode_setattr(dentry, attr);

 		if (!error)

@@ -171,8 +173,11 @@ int notify_change(struct dentry * dentry

 	if (ia_valid & ATTR_SIZE)

 		up_write(&dentry->d_inode->i_alloc_sem);
-	if (!error)

+	if (!error) {

 		fsnotify_change(dentry, ia_valid);

+		if (mandatory)

+			locks_wakeup_mandatory(inode);

+	}
 	return error;

 }

diff --git a/fs/locks.c b/fs/locks.c

index 83ba887..c0c2281 100644

--- a/fs/locks.c

+++ b/fs/locks.c

@@ -1106,7 +1106,8 @@ int locks_mandatory_area(int read_write,

 			break;

 		if (!(fl.fl_flags & FL_SLEEP))

 			break;

-		error = wait_event_interruptible(fl.fl_wait, !fl.fl_next);

+		error = wait_event_interruptible(fl.fl_wait,

+				!fl.fl_next || !__MANDATORY_LOCK(inode));

 		if (!error) {

 			/*

 			 * If we've been sleeping someone might have

@@ -1125,6 +1126,20 @@ int locks_mandatory_area(int read_write,
 EXPORT_SYMBOL(locks_mandatory_area);
+void locks_wakeup_mandatory(struct inode *inode)

+{

+	struct file_lock *fl, **before;

+

+	lock_kernel();

+	for_each_lock(inode, before) {

+		fl = *before;

+

+		if (IS_POSIX(fl))

+			locks_wake_u...
[ message continues ]

I suppose so.  Does anyone actually use mandatory locking?
Would it be worth adding a
	if (MANDATORY_LOCK(inode))

		return;
to the beginning of locks_wakeup_mandatory() to avoid walking the list

of locks in that case?  Perhaps setattr is rare enough that this just

isn't worth caring about.
but early enough that someone can still block on the lock while the file

is still marked for mandatory locking?  (And is the inode->i_flock check

there really necessary?)
Well, there are probably worse races in the mandatory locking code.

(For example, my impression is that a mandatory lock can be applied just

after the locks_mandatory_area() checks but before the io actually

completes.)
-

...there are. The inode->i_lock is protected with lock_kernel() only

and is not in sync with any other checks for inodes. This is sad :(
Thanks,

Pavel

-

OK, but why not just remove the inode->i_flock check there?  I can't see
I would also prefer a locking scheme that didn't rely on the BKL.  That
... I'm not aware of other races in the existing file-locking code.  It

sounds like you might be.  Could you give specific examples?
--b.

-

I would as well :) But I don't know the locking code good enough to

start fixing. Besides, even if I send a patch series that handles this,

I don't think that anyone will accept it, due to "this changes too much
Well, there's a long standing BUG in leases code - when we made all the

checks in inserting lease, we call the locks_alloc_lock() and may fall

asleep. Bu after the wakeup nobody re-checks for the things to change.
-

Several people have expressed interest in a locking scheme for locks.c

(and probably lockd) that doesn't depend on BKL, so I don't think it

would be ignored.  But, yes, it would have to be done very carefully;
OK.  Thanks in advance for finding any!
--b.

-

Another long-term project might be to convert the current list of locks

into a more scalable structure: something like an rbtree might be more

appropriate for really large numbers of locks.
Cheers

  Trond
-