Re: [PATCH] Allow cluster-wide flock

Re: [PATCH] Allow cluster-wide flock

[Alan Cox]

On Llu, 2004-08-30 at 16:19, Ken Preslan wrote:
> Hi,
> 
> Below is a patch that lets a cluster filesystem (such as GFS) implement
> flock across a the cluster.  Please apply.

flock affects local node only traditionally and applications expect high
performance from it. Our documentation merely says

       flock(2) does not lock files over NFS.  Use fcntl(2) instead:
that does
       work  over  NFS,  given  a  sufficiently  recent version of Linux
and a
       server which supports locking.

I'm not sure how we should count GFS but other than noting a need to
think about it I see no problems with a cluster being "local"

[PATCH] Allow cluster-wide flock

[Ken Preslan]

Hi,

Below is a patch that lets a cluster filesystem (such as GFS) implement
flock across a the cluster.  Please apply.

Thanks!


diff -urN -p linux-2.6.9-rc1-mm1/fs/locks.c linux/fs/locks.c
--- linux-2.6.9-rc1-mm1/fs/locks.c	2004-08-27 10:54:32.939282703 -0500
+++ linux/fs/locks.c	2004-08-27 10:54:34.540910950 -0500
@@ -1318,6 +1318,33 @@ out_unlock:
 }
 
 /**
+ * flock_lock_file_wait - Apply a FLOCK-style lock to a file
+ * @filp: The file to apply the lock to
+ * @fl: The lock to be applied
+ *
+ * Add a FLOCK style lock to a file.
+ */
+int flock_lock_file_wait(struct file *filp, struct file_lock *fl)
+{
+	int error;
+	might_sleep();
+	for (;;) {
+		error = flock_lock_file(filp, fl);
+		if ((error != -EAGAIN) || !(fl->fl_flags & FL_SLEEP))
+			break;
+		error = wait_event_interruptible(fl->fl_wait, !fl->fl_next);
+		if (!error)
+			continue;
+
+		locks_delete_block(fl);
+		break;
+	}
+	return error;
+}
+
+EXPORT_SYMBOL(flock_lock_file_wait);
+
+/**
  *	sys_flock: - flock() system call.
  *	@fd: the file descriptor to lock.
  *	@cmd: the type of lock to apply.
@@ -1365,6 +1392,13 @@ asmlinkage long sys_flock(unsigned int f
 	if (error)
 		goto out_free;
 
+	if (filp->f_op && filp->f_op->lock) {
+		error = filp->f_op->lock(filp,
+					(can_sleep) ? F_SETLKW : F_SETLK,
+					lock);
+		goto out_free;
+	}
+
 	for (;;) {
 		error = flock_lock_file(filp, lock);
 		if ((error != -EAGAIN) || !can_sleep)
@@ -1732,6 +1766,12 @@ void locks_remove_flock(struct file *fil
 	if (!inode->i_flock)
 		return;
 
+	if (filp->f_op && filp->f_op->lock) {
+		struct file_lock fl = { .fl_flags = FL_FLOCK,
+					.fl_type = F_UNLCK };
+		filp->f_op->lock(filp, F_SETLKW, &fl);
+	}	
+
 	lock_kernel();
 	before = &inode->i_flock;
 
diff -urN -p linux-2.6.9-rc1-mm1/include/linux/fs.h linux/include/linux/fs.h
--- linux-2.6.9-rc1-mm1/include/linux/fs.h	2004-08-27 10:54:32.941282239 -0500
+++ linux/include/linux/fs.h	2004-08-27 10:54:34.541910717 -0500
@@ -697,6 +697,7 @@ extern int posix_lock_file_wait(struct f
 extern void posix_block_lock(struct file_lock *, struct file_lock *);
 extern void posix_unblock_lock(struct file *, struct file_lock *);
 extern int posix_locks_deadlock(struct file_lock *, struct file_lock *);
+extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl);
 extern int __break_lease(struct inode *inode, unsigned int flags);
 extern void lease_get_mtime(struct inode *, struct timespec *time);
 extern int lock_may_read(struct inode *, loff_t start, unsigned long count);

RE: [PATCH] Allow cluster-wide flock

[Trond Myklebust]

I missed Ken's original mail this morning, so apologies for breaking the
thread...

So I have 2 comments:

Firstly, it would be nice to throw out the existing wait loop in
sys_flock(), and replace it with a call to this new
flock_lock_file_wait(). I suppose that can be done in a separate cleanup
patch, though...


More importantly, though, I'm concerned that the overloading of
f_op->lock() may break behaviour on NFS and CIFS.
NFS currently has a test in nfs_lock() that causes it to return -ENOLCK
if the argument is not a posix lock. I'm planning on changing that by
implementing a proper flock(), but we need an interim solution that
doesn't change existing behaviour for people.

Any comment on the effects on CIFS, Steven?


One solution that I've already suggested to Ken & co is to use a
separate f_op->flock() call. That would be my preference, since the
filesystems have to treat flock and posix locks differently anyway.

Alternatively, we're going to have to fix up NFS at least (CIFS?) before
this patch can be applied to the mainline kernel.

Cheers,
  Trond

Re: [PATCH] Allow cluster-wide flock

[Steve French]

> I'm concerned that the overloading of
> f_op->lock() may break behaviour on NFS and CIFS.

Seems like a clear case that if their behavior is different we have to
be able to distinguish between them and there is a risk of overloading
f_op->lock().  It would not be particularly hard to carry them on the wire
(ie as network requests using a slightly modified newer SMBLockingX 
that would work to Samba).   Do you want these locks enforced [both] on the
client and server too (if there are races in the lock state when multiple
clients are created/deleting locks)?

> NFS currently has a test in nfs_lock() that causes it to return -ENOLCK
> if the argument is not a posix lock. 

I am willing to use a more relaxed semantic than that, but will
need to add an optional extension to the CIFS Protocol Unix Extensions
to distinguish the three (at least) types of locks

>
> Any comment on the effects on CIFS, Steven?

I am not sure I can answer until I get the current 2.6.9 locking
code working again and have time to prototype/experiment.
Cifs locking code regressed in two places 
(one of which I have fixed recently).  This is not the
usual impossible problem of trying to reconcile POSIX vs. 
SMB/CIFS locks well enough to pass connectathon lock tests 7 
and 10 to both Windows and Samba servers.  CIFS (the network
protocol) only defines mandatory locks (and overlapping locks are
not coalesced into one larger lock).  Since some clients such
as Windows (and OS/2, DOS etc - but not Unix & Linux 
clients) expect mandatory locks (rather than Advisory) 
there is not a perfect solution without extending the
protocol to allow the client to tell the server
that the locks are Advisory vs. Mandatory
(so after discussions with others on the Samba team this
is something that I am likely to do this year on the client
(ie optionally extend the protocol to distinguish the
two lock types), as soon as jra has time to change
the Samba server side) ... 

but in the meantime the CIFS lock code is broken. It looks like
the  global fs change to fix posix locking last week
broke cifs.  unlock (at least in connectathon 
lock subtest 7) can generate over CIFS a message:
	Attempting to free lock with active block list
probably because an additional change to unlock is necessary
in fs/cifs/file.c  I did fix one unrelated bug in the 
cifs lock code yesterday but today was trying to understand
what test 7 is attempting to do and why the block list is
not freed for cifs (but works locally).

Re: [PATCH] Allow cluster-wide flock

[Ken Preslan]

On Mon, Aug 30, 2004 at 07:05:29PM -0400, Trond Myklebust wrote:
> ...
> Firstly, it would be nice to throw out the existing wait loop in
> sys_flock(), and replace it with a call to this new
> flock_lock_file_wait(). I suppose that can be done in a separate cleanup
> patch, though...
> ... 
> One solution that I've already suggested to Ken & co is to use a
> separate f_op->flock() call. That would be my preference, since the
> filesystems have to treat flock and posix locks differently anyway.
> ...

Thanks for the suggestions.  Below is a patch that implements both of
them.


diff -urN -p linux-2.6.9-rc1-mm2/fs/locks.c linux/fs/locks.c
--- linux-2.6.9-rc1-mm2/fs/locks.c	2004-08-31 16:44:58.385205028 -0500
+++ linux/fs/locks.c	2004-08-31 16:45:03.260092514 -0500
@@ -1392,24 +1392,12 @@ asmlinkage long sys_flock(unsigned int f
 	if (error)
 		goto out_free;
 
-	if (filp->f_op && filp->f_op->lock) {
-		error = filp->f_op->lock(filp,
-					(can_sleep) ? F_SETLKW : F_SETLK,
-					lock);
-		goto out_free;
-	}
-
-	for (;;) {
-		error = flock_lock_file(filp, lock);
-		if ((error != -EAGAIN) || !can_sleep)
-			break;
-		error = wait_event_interruptible(lock->fl_wait, !lock->fl_next);
-		if (!error)
-			continue;
-
-		locks_delete_block(lock);
-		break;
-	}
+	if (filp->f_op && filp->f_op->flock)
+		error = filp->f_op->flock(filp,
+					  (can_sleep) ? F_SETLKW : F_SETLK,
+					  lock);
+	else
+		error = flock_lock_file_wait(filp, lock);
 
  out_free:
 	if (list_empty(&lock->fl_link)) {
@@ -1766,10 +1754,10 @@ void locks_remove_flock(struct file *fil
 	if (!inode->i_flock)
 		return;
 
-	if (filp->f_op && filp->f_op->lock) {
+	if (filp->f_op && filp->f_op->flock) {
 		struct file_lock fl = { .fl_flags = FL_FLOCK,
 					.fl_type = F_UNLCK };
-		filp->f_op->lock(filp, F_SETLKW, &fl);
+		filp->f_op->flock(filp, F_SETLKW, &fl);
 	}
 
 	lock_kernel();
diff -urN -p linux-2.6.9-rc1-mm2/include/linux/fs.h linux/include/linux/fs.h
--- linux-2.6.9-rc1-mm2/include/linux/fs.h	2004-08-31 16:44:58.386204800 -0500
+++ linux/include/linux/fs.h	2004-08-31 16:45:03.261092285 -0500
@@ -983,6 +983,7 @@ struct file_operations {
 	unsigned long (*get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
 	int (*check_flags)(int);
 	int (*dir_notify)(struct file *filp, unsigned long arg);
+	int (*flock) (struct file *, int, struct file_lock *);
 };
 
 struct inode_operations {

-- 
Ken Preslan <kpreslan@redhat.com>