[PATCH] Add support for flock over a cifs mount.

[PATCH] Add support for flock over a cifs mount.

[Sachin Prabhu]

Allow flock over a cifs mount.

Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 8f1fe32..56d3b36 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -830,6 +830,7 @@ const struct file_operations cifs_file_ops = {
 	.open = cifs_open,
 	.release = cifs_close,
 	.lock = cifs_lock,
+	.flock = cifs_flock,
 	.fsync = cifs_fsync,
 	.flush = cifs_flush,
 	.mmap  = cifs_file_mmap,
@@ -849,6 +850,7 @@ const struct file_operations cifs_file_strict_ops = {
 	.open = cifs_open,
 	.release = cifs_close,
 	.lock = cifs_lock,
+	.flock = cifs_flock,
 	.fsync = cifs_strict_fsync,
 	.flush = cifs_flush,
 	.mmap = cifs_file_strict_mmap,
@@ -869,6 +871,7 @@ const struct file_operations cifs_file_direct_ops = {
 	.open = cifs_open,
 	.release = cifs_close,
 	.lock = cifs_lock,
+	.flock = cifs_flock,
 	.fsync = cifs_fsync,
 	.flush = cifs_flush,
 	.mmap = cifs_file_mmap,
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 30ff560..e119e9d 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -87,6 +87,7 @@ extern ssize_t cifs_user_writev(struct kiocb *iocb, const struct iovec *iov,
 extern ssize_t cifs_strict_writev(struct kiocb *iocb, const struct iovec *iov,
 				  unsigned long nr_segs, loff_t pos);
 extern int cifs_lock(struct file *, int, struct file_lock *);
+extern int cifs_flock(struct file *, int, struct file_lock *);
 extern int cifs_fsync(struct file *, loff_t, loff_t, int);
 extern int cifs_strict_fsync(struct file *, loff_t, loff_t, int);
 extern int cifs_flush(struct file *, fl_owner_t id);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index c1f063c..dd34f0b 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -43,6 +43,23 @@
 #include "cifs_fs_sb.h"
 #include "fscache.h"
 
+static int do_vfs_lock(struct file *file, struct file_lock *fl)
+{
+	int res = 0;
+
+	switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
+	case FL_POSIX:
+		res = posix_lock_file_wait(file, fl);
+		break;
+	case FL_FLOCK:
+		res = flock_lock_file_wait(file, fl);
+		break;
+	default:
+		BUG();
+	}
+	return res;
+}
+
 static inline int cifs_convert_flags(unsigned int flags)
 {
 	if ((flags & O_ACCMODE) == O_RDONLY)
@@ -814,7 +831,7 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock)
 	struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
 	int rc = 1;
 
-	if ((flock->fl_flags & FL_POSIX) == 0)
+	if ((flock->fl_flags & (FL_POSIX | FL_FLOCK)) == 0)
 		return rc;
 
 	mutex_lock(&cinode->lock_mutex);
@@ -822,7 +839,7 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock)
 		mutex_unlock(&cinode->lock_mutex);
 		return rc;
 	}
-	rc = posix_lock_file_wait(file, flock);
+	rc = do_vfs_lock(file, flock);
 	mutex_unlock(&cinode->lock_mutex);
 	return rc;
 }
@@ -1231,12 +1248,27 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
 	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
 	struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
 	__u16 netfid = cfile->netfid;
+	unsigned char fl_flags = flock->fl_flags;
+
+	if (unlock) {
+		/*Check for existance of lock*/
+		flock->fl_flags |= FL_EXISTS;
+		if (do_vfs_lock(flock->fl_file, flock) == -ENOENT)
+			goto out;
+	} else {
+		/*Check if we can obtain lock*/
+		flock->fl_flags |= FL_ACCESS;
+		rc = do_vfs_lock(flock->fl_file, flock);
+		if (rc < 0)
+			goto out;
+	}
+	flock->fl_flags = fl_flags;
 
 	if (posix_lck) {
 		int posix_lock_type;
 
 		rc = cifs_posix_lock_set(file, flock);
-		if (!rc || rc < 0)
+		if (rc <= 0)
 			return rc;
 
 		if (type & LOCKING_ANDX_SHARED_LOCK)
@@ -1250,7 +1282,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
 		rc = CIFSSMBPosixLock(xid, tcon, netfid, current->tgid,
 				      0 /* set */, length, flock,
 				      posix_lock_type, wait_flag);
-		goto out;
+		goto out_lock;
 	}
 
 	if (lock) {
@@ -1259,7 +1291,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
 		if (rc < 0)
 			return rc;
 		else if (!rc)
-			goto out;
+			goto out_lock;
 
 		rc = CIFSSMBLock(xid, tcon, netfid, current->tgid, length,
 				 flock->fl_start, 0, 1, type, wait_flag, 0);
@@ -1271,9 +1303,14 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
 	} else if (unlock)
 		rc = cifs_unlock_range(cfile, flock, xid);
 
+out_lock:
+	if (rc == 0 && lock) {
+		/*We have to sleep here*/
+		flock->fl_flags |= FL_SLEEP;
+		do_vfs_lock(file, flock);
+	}
 out:
-	if (flock->fl_flags & FL_POSIX)
-		posix_lock_file_wait(file, flock);
+	flock->fl_flags = fl_flags;
 	return rc;
 }
 
@@ -1334,6 +1371,22 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *flock)
 	return rc;
 }
 
+int cifs_flock(struct file *file, int cmd, struct file_lock *flock)
+{
+	int rc;
+
+	rc = cifs_lock(file, cmd, flock);
+
+	/*
+	 * flock requires -EWOULDBLOCK in case of conflicting locks
+	 * and LOCK_NB is used
+	 */
+	if ((rc == -EACCES) && !(flock->fl_flags & FL_SLEEP))
+		rc = -EWOULDBLOCK;
+
+	return rc;
+}
+
 /* update the file size (if needed) after a write */
 void
 cifs_update_eof(struct cifsInodeInfo *cifsi, loff_t offset,

Re: [PATCH] Add support for flock over a cifs mount.

[Jeff Layton]

On Mon, 07 Nov 2011 20:05:40 +0000
Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> Allow flock over a cifs mount.
> 
> Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 8f1fe32..56d3b36 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -830,6 +830,7 @@ const struct file_operations cifs_file_ops = {
>  	.open = cifs_open,
>  	.release = cifs_close,
>  	.lock = cifs_lock,
> +	.flock = cifs_flock,
>  	.fsync = cifs_fsync,
>  	.flush = cifs_flush,
>  	.mmap  = cifs_file_mmap,
> @@ -849,6 +850,7 @@ const struct file_operations cifs_file_strict_ops = {
>  	.open = cifs_open,
>  	.release = cifs_close,
>  	.lock = cifs_lock,
> +	.flock = cifs_flock,
>  	.fsync = cifs_strict_fsync,
>  	.flush = cifs_flush,
>  	.mmap = cifs_file_strict_mmap,
> @@ -869,6 +871,7 @@ const struct file_operations cifs_file_direct_ops = {
>  	.open = cifs_open,
>  	.release = cifs_close,
>  	.lock = cifs_lock,
> +	.flock = cifs_flock,
>  	.fsync = cifs_fsync,
>  	.flush = cifs_flush,
>  	.mmap = cifs_file_mmap,
> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> index 30ff560..e119e9d 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -87,6 +87,7 @@ extern ssize_t cifs_user_writev(struct kiocb *iocb, const struct iovec *iov,
>  extern ssize_t cifs_strict_writev(struct kiocb *iocb, const struct iovec *iov,
>  				  unsigned long nr_segs, loff_t pos);
>  extern int cifs_lock(struct file *, int, struct file_lock *);
> +extern int cifs_flock(struct file *, int, struct file_lock *);
>  extern int cifs_fsync(struct file *, loff_t, loff_t, int);
>  extern int cifs_strict_fsync(struct file *, loff_t, loff_t, int);
>  extern int cifs_flush(struct file *, fl_owner_t id);
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index c1f063c..dd34f0b 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -43,6 +43,23 @@
>  #include "cifs_fs_sb.h"
>  #include "fscache.h"
>  
> +static int do_vfs_lock(struct file *file, struct file_lock *fl)
> +{
> +	int res = 0;
> +
> +	switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> +	case FL_POSIX:
> +		res = posix_lock_file_wait(file, fl);
> +		break;
> +	case FL_FLOCK:
> +		res = flock_lock_file_wait(file, fl);
> +		break;
> +	default:
> +		BUG();

	Is this really BUG-worthy? Maybe an error return with a WARN or
	something would be more appropriate. I generally prefer not to
	BUG unless there really is no other option as a lot of enterprise
	distros set panic_on_oops.

> +	}
> +	return res;
> +}
> +
>  static inline int cifs_convert_flags(unsigned int flags)
>  {
>  	if ((flags & O_ACCMODE) == O_RDONLY)
> @@ -814,7 +831,7 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock)
>  	struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>  	int rc = 1;
>  
> -	if ((flock->fl_flags & FL_POSIX) == 0)
> +	if ((flock->fl_flags & (FL_POSIX | FL_FLOCK)) == 0)
>  		return rc;
>  
>  	mutex_lock(&cinode->lock_mutex);
> @@ -822,7 +839,7 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock)
>  		mutex_unlock(&cinode->lock_mutex);
>  		return rc;
>  	}
> -	rc = posix_lock_file_wait(file, flock);
> +	rc = do_vfs_lock(file, flock);
>  	mutex_unlock(&cinode->lock_mutex);
>  	return rc;
>  }
> @@ -1231,12 +1248,27 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>  	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
>  	struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>  	__u16 netfid = cfile->netfid;
> +	unsigned char fl_flags = flock->fl_flags;
> +
> +	if (unlock) {
> +		/*Check for existance of lock*/
> +		flock->fl_flags |= FL_EXISTS;
> +		if (do_vfs_lock(flock->fl_file, flock) == -ENOENT)
> +			goto out;
> +	} else {
> +		/*Check if we can obtain lock*/
> +		flock->fl_flags |= FL_ACCESS;
> +		rc = do_vfs_lock(flock->fl_file, flock);
> +		if (rc < 0)
> +			goto out;
> +	}
> +	flock->fl_flags = fl_flags;
>  
>  	if (posix_lck) {
>  		int posix_lock_type;
>  
>  		rc = cifs_posix_lock_set(file, flock);
> -		if (!rc || rc < 0)
> +		if (rc <= 0)
>  			return rc;
>  
>  		if (type & LOCKING_ANDX_SHARED_LOCK)
> @@ -1250,7 +1282,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>  		rc = CIFSSMBPosixLock(xid, tcon, netfid, current->tgid,
>  				      0 /* set */, length, flock,
>  				      posix_lock_type, wait_flag);
> -		goto out;
> +		goto out_lock;
>  	}
>  
>  	if (lock) {
> @@ -1259,7 +1291,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>  		if (rc < 0)
>  			return rc;
>  		else if (!rc)
> -			goto out;
> +			goto out_lock;
>  
>  		rc = CIFSSMBLock(xid, tcon, netfid, current->tgid, length,
>  				 flock->fl_start, 0, 1, type, wait_flag, 0);
> @@ -1271,9 +1303,14 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>  	} else if (unlock)
>  		rc = cifs_unlock_range(cfile, flock, xid);
>  
> +out_lock:
> +	if (rc == 0 && lock) {
> +		/*We have to sleep here*/

		nit: there should be spaces between the *'s and
		comment. I'd imagine checkpatch.pl would catch that,
		but maybe not?

> +		flock->fl_flags |= FL_SLEEP;
> +		do_vfs_lock(file, flock);
> +	}
>  out:
> -	if (flock->fl_flags & FL_POSIX)
> -		posix_lock_file_wait(file, flock);
> +	flock->fl_flags = fl_flags;
>  	return rc;
>  }
>  
> @@ -1334,6 +1371,22 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *flock)
>  	return rc;
>  }
>  
> +int cifs_flock(struct file *file, int cmd, struct file_lock *flock)
> +{
> +	int rc;
> +
> +	rc = cifs_lock(file, cmd, flock);
> +
> +	/*
> +	 * flock requires -EWOULDBLOCK in case of conflicting locks
> +	 * and LOCK_NB is used
> +	 */
> +	if ((rc == -EACCES) && !(flock->fl_flags & FL_SLEEP))

	nit: these extra parens are not needed

> +		rc = -EWOULDBLOCK;
> +
> +	return rc;
> +}
> +
>  /* update the file size (if needed) after a write */
>  void
>  cifs_update_eof(struct cifsInodeInfo *cifsi, loff_t offset,
> 
> 

Aside from the nits and the questionable BUG call, I think this looks
like nice work.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] Add support for flock over a cifs mount.

[Pavel Shilovsky]

2011/11/8 Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Allow flock over a cifs mount.
>
> Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 8f1fe32..56d3b36 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -830,6 +830,7 @@ const struct file_operations cifs_file_ops = {
>        .open = cifs_open,
>        .release = cifs_close,
>        .lock = cifs_lock,
> +       .flock = cifs_flock,
>        .fsync = cifs_fsync,
>        .flush = cifs_flush,
>        .mmap  = cifs_file_mmap,
> @@ -849,6 +850,7 @@ const struct file_operations cifs_file_strict_ops = {
>        .open = cifs_open,
>        .release = cifs_close,
>        .lock = cifs_lock,
> +       .flock = cifs_flock,
>        .fsync = cifs_strict_fsync,
>        .flush = cifs_flush,
>        .mmap = cifs_file_strict_mmap,
> @@ -869,6 +871,7 @@ const struct file_operations cifs_file_direct_ops = {
>        .open = cifs_open,
>        .release = cifs_close,
>        .lock = cifs_lock,
> +       .flock = cifs_flock,
>        .fsync = cifs_fsync,
>        .flush = cifs_flush,
>        .mmap = cifs_file_mmap,
> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> index 30ff560..e119e9d 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -87,6 +87,7 @@ extern ssize_t cifs_user_writev(struct kiocb *iocb, const struct iovec *iov,
>  extern ssize_t cifs_strict_writev(struct kiocb *iocb, const struct iovec *iov,
>                                  unsigned long nr_segs, loff_t pos);
>  extern int cifs_lock(struct file *, int, struct file_lock *);
> +extern int cifs_flock(struct file *, int, struct file_lock *);
>  extern int cifs_fsync(struct file *, loff_t, loff_t, int);
>  extern int cifs_strict_fsync(struct file *, loff_t, loff_t, int);
>  extern int cifs_flush(struct file *, fl_owner_t id);
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index c1f063c..dd34f0b 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -43,6 +43,23 @@
>  #include "cifs_fs_sb.h"
>  #include "fscache.h"
>
> +static int do_vfs_lock(struct file *file, struct file_lock *fl)
> +{
> +       int res = 0;
> +
> +       switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> +       case FL_POSIX:
> +               res = posix_lock_file_wait(file, fl);
> +               break;
> +       case FL_FLOCK:
> +               res = flock_lock_file_wait(file, fl);
> +               break;
> +       default:
> +               BUG();
> +       }
> +       return res;
> +}
> +
>  static inline int cifs_convert_flags(unsigned int flags)
>  {
>        if ((flags & O_ACCMODE) == O_RDONLY)
> @@ -814,7 +831,7 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock)
>        struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>        int rc = 1;
>
> -       if ((flock->fl_flags & FL_POSIX) == 0)
> +       if ((flock->fl_flags & (FL_POSIX | FL_FLOCK)) == 0)
>                return rc;
>
>        mutex_lock(&cinode->lock_mutex);
> @@ -822,7 +839,7 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock)
>                mutex_unlock(&cinode->lock_mutex);
>                return rc;
>        }
> -       rc = posix_lock_file_wait(file, flock);
> +       rc = do_vfs_lock(file, flock);
>        mutex_unlock(&cinode->lock_mutex);
>        return rc;
>  }
> @@ -1231,12 +1248,27 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>        struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
>        struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>        __u16 netfid = cfile->netfid;
> +       unsigned char fl_flags = flock->fl_flags;
> +
> +       if (unlock) {
> +               /*Check for existance of lock*/
> +               flock->fl_flags |= FL_EXISTS;
> +               if (do_vfs_lock(flock->fl_file, flock) == -ENOENT)
> +                       goto out;
> +       } else {
> +               /*Check if we can obtain lock*/
> +               flock->fl_flags |= FL_ACCESS;
> +               rc = do_vfs_lock(flock->fl_file, flock);
> +               if (rc < 0)
> +                       goto out;
> +       }
> +       flock->fl_flags = fl_flags;

I think this should be skipped for non-posix locks. These locks have
their own cache (cifs_lock_* functions) associated with cifsInodeInfo
structure - so, we can't make a solution to 'goto out;' without
looking at this cache.

>
>        if (posix_lck) {
>                int posix_lock_type;
>
>                rc = cifs_posix_lock_set(file, flock);
> -               if (!rc || rc < 0)
> +               if (rc <= 0)
>                        return rc;
>
>                if (type & LOCKING_ANDX_SHARED_LOCK)
> @@ -1250,7 +1282,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>                rc = CIFSSMBPosixLock(xid, tcon, netfid, current->tgid,
>                                      0 /* set */, length, flock,
>                                      posix_lock_type, wait_flag);
> -               goto out;
> +               goto out_lock;
>        }
>
>        if (lock) {
> @@ -1259,7 +1291,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>                if (rc < 0)
>                        return rc;
>                else if (!rc)
> -                       goto out;
> +                       goto out_lock;
>
>                rc = CIFSSMBLock(xid, tcon, netfid, current->tgid, length,
>                                 flock->fl_start, 0, 1, type, wait_flag, 0);
> @@ -1271,9 +1303,14 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>        } else if (unlock)
>                rc = cifs_unlock_range(cfile, flock, xid);
>
> +out_lock:
> +       if (rc == 0 && lock) {
> +               /*We have to sleep here*/
> +               flock->fl_flags |= FL_SLEEP;

It's not clear why we should sleep here - the patch description
doesn't tell anything about it.

> +               do_vfs_lock(file, flock);
> +       }

What's about unlock case here?

>  out:
> -       if (flock->fl_flags & FL_POSIX)
> -               posix_lock_file_wait(file, flock);
> +       flock->fl_flags = fl_flags;
>        return rc;
>  }
>
> @@ -1334,6 +1371,22 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *flock)
>        return rc;
>  }
>
> +int cifs_flock(struct file *file, int cmd, struct file_lock *flock)
> +{
> +       int rc;
> +
> +       rc = cifs_lock(file, cmd, flock);
> +
> +       /*
> +        * flock requires -EWOULDBLOCK in case of conflicting locks
> +        * and LOCK_NB is used
> +        */
> +       if ((rc == -EACCES) && !(flock->fl_flags & FL_SLEEP))
> +               rc = -EWOULDBLOCK;
> +
> +       return rc;
> +}
> +
>  /* update the file size (if needed) after a write */
>  void
>  cifs_update_eof(struct cifsInodeInfo *cifsi, loff_t offset,
>
>
>

In general, I think that this patch does much more than it's mentioned
in the description and brings a lot of changes in the existing lock
code.

It seems like we should only change posix_file_lock_wait to your
do_vfs_lock in 'out' label and cifs_posix_lock_set for posix_lck case
and map flock to brlock from 0 to INF for !posix_lck case.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] Add support for flock over a cifs mount.

[Sachin Prabhu]

On Tue, 2011-11-08 at 11:57 +0400, Pavel Shilovsky wrote:
> 2011/11/8 Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > Allow flock over a cifs mount.
> >
> > Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 8f1fe32..56d3b36 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -830,6 +830,7 @@ const struct file_operations cifs_file_ops = {
> >        .open = cifs_open,
> >        .release = cifs_close,
> >        .lock = cifs_lock,
> > +       .flock = cifs_flock,
> >        .fsync = cifs_fsync,
> >        .flush = cifs_flush,
> >        .mmap  = cifs_file_mmap,
> > @@ -849,6 +850,7 @@ const struct file_operations cifs_file_strict_ops = {
> >        .open = cifs_open,
> >        .release = cifs_close,
> >        .lock = cifs_lock,
> > +       .flock = cifs_flock,
> >        .fsync = cifs_strict_fsync,
> >        .flush = cifs_flush,
> >        .mmap = cifs_file_strict_mmap,
> > @@ -869,6 +871,7 @@ const struct file_operations cifs_file_direct_ops = {
> >        .open = cifs_open,
> >        .release = cifs_close,
> >        .lock = cifs_lock,
> > +       .flock = cifs_flock,
> >        .fsync = cifs_fsync,
> >        .flush = cifs_flush,
> >        .mmap = cifs_file_mmap,
> > diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> > index 30ff560..e119e9d 100644
> > --- a/fs/cifs/cifsfs.h
> > +++ b/fs/cifs/cifsfs.h
> > @@ -87,6 +87,7 @@ extern ssize_t cifs_user_writev(struct kiocb *iocb, const struct iovec *iov,
> >  extern ssize_t cifs_strict_writev(struct kiocb *iocb, const struct iovec *iov,
> >                                  unsigned long nr_segs, loff_t pos);
> >  extern int cifs_lock(struct file *, int, struct file_lock *);
> > +extern int cifs_flock(struct file *, int, struct file_lock *);
> >  extern int cifs_fsync(struct file *, loff_t, loff_t, int);
> >  extern int cifs_strict_fsync(struct file *, loff_t, loff_t, int);
> >  extern int cifs_flush(struct file *, fl_owner_t id);
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index c1f063c..dd34f0b 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -43,6 +43,23 @@
> >  #include "cifs_fs_sb.h"
> >  #include "fscache.h"
> >
> > +static int do_vfs_lock(struct file *file, struct file_lock *fl)
> > +{
> > +       int res = 0;
> > +
> > +       switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > +       case FL_POSIX:
> > +               res = posix_lock_file_wait(file, fl);
> > +               break;
> > +       case FL_FLOCK:
> > +               res = flock_lock_file_wait(file, fl);
> > +               break;
> > +       default:
> > +               BUG();
> > +       }
> > +       return res;
> > +}
> > +
> >  static inline int cifs_convert_flags(unsigned int flags)
> >  {
> >        if ((flags & O_ACCMODE) == O_RDONLY)
> > @@ -814,7 +831,7 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock)
> >        struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
> >        int rc = 1;
> >
> > -       if ((flock->fl_flags & FL_POSIX) == 0)
> > +       if ((flock->fl_flags & (FL_POSIX | FL_FLOCK)) == 0)
> >                return rc;
> >
> >        mutex_lock(&cinode->lock_mutex);
> > @@ -822,7 +839,7 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock)
> >                mutex_unlock(&cinode->lock_mutex);
> >                return rc;
> >        }
> > -       rc = posix_lock_file_wait(file, flock);
> > +       rc = do_vfs_lock(file, flock);
> >        mutex_unlock(&cinode->lock_mutex);
> >        return rc;
> >  }
> > @@ -1231,12 +1248,27 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
> >        struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> >        struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
> >        __u16 netfid = cfile->netfid;
> > +       unsigned char fl_flags = flock->fl_flags;
> > +
> > +       if (unlock) {
> > +               /*Check for existance of lock*/
> > +               flock->fl_flags |= FL_EXISTS;
> > +               if (do_vfs_lock(flock->fl_file, flock) == -ENOENT)
> > +                       goto out;
> > +       } else {
> > +               /*Check if we can obtain lock*/
> > +               flock->fl_flags |= FL_ACCESS;
> > +               rc = do_vfs_lock(flock->fl_file, flock);
> > +               if (rc < 0)
> > +                       goto out;
> > +       }
> > +       flock->fl_flags = fl_flags;
> 
> I think this should be skipped for non-posix locks. These locks have
> their own cache (cifs_lock_* functions) associated with cifsInodeInfo
> structure - so, we can't make a solution to 'goto out;' without
> looking at this cache.

I could follow Jeff's suggestion here and take the BUG out of
do_vfs_lock(). In case of non posix/non flocks, the do_vfs_lock will
simply return 0 and not trigger the goto out and proceed normally.


> 
> >
> >        if (posix_lck) {
> >                int posix_lock_type;
> >
> >                rc = cifs_posix_lock_set(file, flock);
> > -               if (!rc || rc < 0)
> > +               if (rc <= 0)
> >                        return rc;
> >
> >                if (type & LOCKING_ANDX_SHARED_LOCK)
> > @@ -1250,7 +1282,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
> >                rc = CIFSSMBPosixLock(xid, tcon, netfid, current->tgid,
> >                                      0 /* set */, length, flock,
> >                                      posix_lock_type, wait_flag);
> > -               goto out;
> > +               goto out_lock;
> >        }
> >
> >        if (lock) {
> > @@ -1259,7 +1291,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
> >                if (rc < 0)
> >                        return rc;
> >                else if (!rc)
> > -                       goto out;
> > +                       goto out_lock;
> >
> >                rc = CIFSSMBLock(xid, tcon, netfid, current->tgid, length,
> >                                 flock->fl_start, 0, 1, type, wait_flag, 0);
> > @@ -1271,9 +1303,14 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
> >        } else if (unlock)
> >                rc = cifs_unlock_range(cfile, flock, xid);
> >
> > +out_lock:
> > +       if (rc == 0 && lock) {
> > +               /*We have to sleep here*/
> > +               flock->fl_flags |= FL_SLEEP;
> 
> It's not clear why we should sleep here - the patch description
> doesn't tell anything about it.

At this stage you have a lock on the remote server. You need to make
sure you have a lock on the local machine before you proceed. This check
is similar to the NFS client code.

> 
> > +               do_vfs_lock(file, flock);
> > +       }
> 
> What's about unlock case here?

Unlock is done in the lines above in the if(unlock) case. We set flag
FL_EXISTS and then do a unlock. In case the lock to be unlocked doesn't
exist, we error with a ENOENT and exit. If it does find a lock, it
unlocks and then proceeds to unlock on the remote server too.

This is required because the file close routine will call unlock calls
for both posix and flock without actually determining what sort of lock
is held. Without this code, my test module was making 2 unlock requests
ie. one for posix locks and one for flocks for same same flock to the
remote server.

> 
> >  out:
> > -       if (flock->fl_flags & FL_POSIX)
> > -               posix_lock_file_wait(file, flock);
> > +       flock->fl_flags = fl_flags;
> >        return rc;
> >  }
> >
> > @@ -1334,6 +1371,22 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *flock)
> >        return rc;
> >  }
> >
> > +int cifs_flock(struct file *file, int cmd, struct file_lock *flock)
> > +{
> > +       int rc;
> > +
> > +       rc = cifs_lock(file, cmd, flock);
> > +
> > +       /*
> > +        * flock requires -EWOULDBLOCK in case of conflicting locks
> > +        * and LOCK_NB is used
> > +        */
> > +       if ((rc == -EACCES) && !(flock->fl_flags & FL_SLEEP))
> > +               rc = -EWOULDBLOCK;
> > +
> > +       return rc;
> > +}
> > +
> >  /* update the file size (if needed) after a write */
> >  void
> >  cifs_update_eof(struct cifsInodeInfo *cifsi, loff_t offset,
> >
> >
> >
> 
> In general, I think that this patch does much more than it's mentioned
> in the description and brings a lot of changes in the existing lock
> code.
> 
> It seems like we should only change posix_file_lock_wait to your
> do_vfs_lock in 'out' label and cifs_posix_lock_set for posix_lck case
> and map flock to brlock from 0 to INF for !posix_lck case.
> 

When coding this, I followed the NFS client code which adds more checks
and balances to ensure that the local and remote locks do not go out of
sync. 

Sachin Prabhu

Re: [PATCH] Add support for flock over a cifs mount.

[Pavel Shilovsky]

2011/11/8 Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Tue, 2011-11-08 at 11:57 +0400, Pavel Shilovsky wrote:
>> 2011/11/8 Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> > Allow flock over a cifs mount.
>> >
>> > Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> >
>> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> > index 8f1fe32..56d3b36 100644
>> > --- a/fs/cifs/cifsfs.c
>> > +++ b/fs/cifs/cifsfs.c
>> > @@ -830,6 +830,7 @@ const struct file_operations cifs_file_ops = {
>> >        .open = cifs_open,
>> >        .release = cifs_close,
>> >        .lock = cifs_lock,
>> > +       .flock = cifs_flock,
>> >        .fsync = cifs_fsync,
>> >        .flush = cifs_flush,
>> >        .mmap  = cifs_file_mmap,
>> > @@ -849,6 +850,7 @@ const struct file_operations cifs_file_strict_ops = {
>> >        .open = cifs_open,
>> >        .release = cifs_close,
>> >        .lock = cifs_lock,
>> > +       .flock = cifs_flock,
>> >        .fsync = cifs_strict_fsync,
>> >        .flush = cifs_flush,
>> >        .mmap = cifs_file_strict_mmap,
>> > @@ -869,6 +871,7 @@ const struct file_operations cifs_file_direct_ops = {
>> >        .open = cifs_open,
>> >        .release = cifs_close,
>> >        .lock = cifs_lock,
>> > +       .flock = cifs_flock,
>> >        .fsync = cifs_fsync,
>> >        .flush = cifs_flush,
>> >        .mmap = cifs_file_mmap,
>> > diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
>> > index 30ff560..e119e9d 100644
>> > --- a/fs/cifs/cifsfs.h
>> > +++ b/fs/cifs/cifsfs.h
>> > @@ -87,6 +87,7 @@ extern ssize_t cifs_user_writev(struct kiocb *iocb, const struct iovec *iov,
>> >  extern ssize_t cifs_strict_writev(struct kiocb *iocb, const struct iovec *iov,
>> >                                  unsigned long nr_segs, loff_t pos);
>> >  extern int cifs_lock(struct file *, int, struct file_lock *);
>> > +extern int cifs_flock(struct file *, int, struct file_lock *);
>> >  extern int cifs_fsync(struct file *, loff_t, loff_t, int);
>> >  extern int cifs_strict_fsync(struct file *, loff_t, loff_t, int);
>> >  extern int cifs_flush(struct file *, fl_owner_t id);
>> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> > index c1f063c..dd34f0b 100644
>> > --- a/fs/cifs/file.c
>> > +++ b/fs/cifs/file.c
>> > @@ -43,6 +43,23 @@
>> >  #include "cifs_fs_sb.h"
>> >  #include "fscache.h"
>> >
>> > +static int do_vfs_lock(struct file *file, struct file_lock *fl)
>> > +{
>> > +       int res = 0;
>> > +
>> > +       switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
>> > +       case FL_POSIX:
>> > +               res = posix_lock_file_wait(file, fl);
>> > +               break;
>> > +       case FL_FLOCK:
>> > +               res = flock_lock_file_wait(file, fl);
>> > +               break;
>> > +       default:
>> > +               BUG();
>> > +       }
>> > +       return res;
>> > +}
>> > +
>> >  static inline int cifs_convert_flags(unsigned int flags)
>> >  {
>> >        if ((flags & O_ACCMODE) == O_RDONLY)
>> > @@ -814,7 +831,7 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock)
>> >        struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>> >        int rc = 1;
>> >
>> > -       if ((flock->fl_flags & FL_POSIX) == 0)
>> > +       if ((flock->fl_flags & (FL_POSIX | FL_FLOCK)) == 0)
>> >                return rc;
>> >
>> >        mutex_lock(&cinode->lock_mutex);
>> > @@ -822,7 +839,7 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock)
>> >                mutex_unlock(&cinode->lock_mutex);
>> >                return rc;
>> >        }
>> > -       rc = posix_lock_file_wait(file, flock);
>> > +       rc = do_vfs_lock(file, flock);
>> >        mutex_unlock(&cinode->lock_mutex);
>> >        return rc;
>> >  }
>> > @@ -1231,12 +1248,27 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>> >        struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
>> >        struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>> >        __u16 netfid = cfile->netfid;
>> > +       unsigned char fl_flags = flock->fl_flags;
>> > +
>> > +       if (unlock) {
>> > +               /*Check for existance of lock*/
>> > +               flock->fl_flags |= FL_EXISTS;
>> > +               if (do_vfs_lock(flock->fl_file, flock) == -ENOENT)
>> > +                       goto out;
>> > +       } else {
>> > +               /*Check if we can obtain lock*/
>> > +               flock->fl_flags |= FL_ACCESS;
>> > +               rc = do_vfs_lock(flock->fl_file, flock);
>> > +               if (rc < 0)
>> > +                       goto out;
>> > +       }
>> > +       flock->fl_flags = fl_flags;
>>
>> I think this should be skipped for non-posix locks. These locks have
>> their own cache (cifs_lock_* functions) associated with cifsInodeInfo
>> structure - so, we can't make a solution to 'goto out;' without
>> looking at this cache.
>
> I could follow Jeff's suggestion here and take the BUG out of
> do_vfs_lock(). In case of non posix/non flocks, the do_vfs_lock will
> simply return 0 and not trigger the goto out and proceed normally.

Sorry, I think I wasn't clear. In CIFS we have two locks variant
according to whether we use POSIX extensions or not. In the case of
POSIX extensions enabled we use VFS brlock cache and it is mirrors the
locks we have on the remote side. So, everything is good in this case.

Let's look at the case when POSIX extensions are disabled - Windows
locking style. The brlocks behavior changes and we use our own cache
for them but we need to call posix_file_lock_wait at the end of
cifs_setlk  to make some use-cases work (e.g. unlock locks of the
child process when it finishes, etc) . And these locks are still
FL_POSIX locks from VFS point of view.

So, I am ok with your changes for POSIX extensions enabled case. Let's move
>> > +       if (unlock) {
>> > +               /*Check for existance of lock*/
>> > +               flock->fl_flags |= FL_EXISTS;
>> > +               if (do_vfs_lock(flock->fl_file, flock) == -ENOENT)
>> > +                       goto out;
>> > +       } else {
>> > +               /*Check if we can obtain lock*/
>> > +               flock->fl_flags |= FL_ACCESS;
>> > +               rc = do_vfs_lock(flock->fl_file, flock);
>> > +               if (rc < 0)
>> > +                       goto out;
>> > +       }
>> > +       flock->fl_flags = fl_flags;

to the beginning of 'if (posix_lck) {}' and

> > +       if (rc == 0 && lock) {
> > +               /*We have to sleep here*/
> > +               flock->fl_flags |= FL_SLEEP;
> > +               do_vfs_lock(file, flock);
> > +       }
> > +       flock->fl_flags = fl_flags;
              return rc;

to the end of it.

>
>
>>
>> >
>> >        if (posix_lck) {
>> >                int posix_lock_type;
>> >
>> >                rc = cifs_posix_lock_set(file, flock);
>> > -               if (!rc || rc < 0)
>> > +               if (rc <= 0)
>> >                        return rc;
>> >
>> >                if (type & LOCKING_ANDX_SHARED_LOCK)
>> > @@ -1250,7 +1282,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>> >                rc = CIFSSMBPosixLock(xid, tcon, netfid, current->tgid,
>> >                                      0 /* set */, length, flock,
>> >                                      posix_lock_type, wait_flag);
>> > -               goto out;
>> > +               goto out_lock;
>> >        }
>> >
>> >        if (lock) {
>> > @@ -1259,7 +1291,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>> >                if (rc < 0)
>> >                        return rc;
>> >                else if (!rc)
>> > -                       goto out;
>> > +                       goto out_lock;
>> >
>> >                rc = CIFSSMBLock(xid, tcon, netfid, current->tgid, length,
>> >                                 flock->fl_start, 0, 1, type, wait_flag, 0);
>> > @@ -1271,9 +1303,14 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>> >        } else if (unlock)
>> >                rc = cifs_unlock_range(cfile, flock, xid);
>> >
>> > +out_lock:
>> > +       if (rc == 0 && lock) {
>> > +               /*We have to sleep here*/
>> > +               flock->fl_flags |= FL_SLEEP;
>>
>> It's not clear why we should sleep here - the patch description
>> doesn't tell anything about it.
>
> At this stage you have a lock on the remote server. You need to make
> sure you have a lock on the local machine before you proceed. This check
> is similar to the NFS client code.
>
>>
>> > +               do_vfs_lock(file, flock);
>> > +       }
>>
>> What's about unlock case here?
>
> Unlock is done in the lines above in the if(unlock) case. We set flag
> FL_EXISTS and then do a unlock. In case the lock to be unlocked doesn't
> exist, we error with a ENOENT and exit. If it does find a lock, it
> unlocks and then proceeds to unlock on the remote server too.
>
> This is required because the file close routine will call unlock calls
> for both posix and flock without actually determining what sort of lock
> is held. Without this code, my test module was making 2 unlock requests
> ie. one for posix locks and one for flocks for same same flock to the
> remote server.
>
>>
>> >  out:
>> > -       if (flock->fl_flags & FL_POSIX)
>> > -               posix_lock_file_wait(file, flock);

And simply call do_vfs_lock() rather than posix_lock_file_wait() here
for Windows locking case.

>> > +       flock->fl_flags = fl_flags;
>> >        return rc;
>> >  }
>> >
>> > @@ -1334,6 +1371,22 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *flock)
>> >        return rc;
>> >  }
>> >
>> > +int cifs_flock(struct file *file, int cmd, struct file_lock *flock)
>> > +{
>> > +       int rc;
>> > +
>> > +       rc = cifs_lock(file, cmd, flock);
>> > +
>> > +       /*
>> > +        * flock requires -EWOULDBLOCK in case of conflicting locks
>> > +        * and LOCK_NB is used
>> > +        */
>> > +       if ((rc == -EACCES) && !(flock->fl_flags & FL_SLEEP))
>> > +               rc = -EWOULDBLOCK;
>> > +
>> > +       return rc;
>> > +}
>> > +
>> >  /* update the file size (if needed) after a write */
>> >  void
>> >  cifs_update_eof(struct cifsInodeInfo *cifsi, loff_t offset,
>> >
>> >
>> >
>>
>> In general, I think that this patch does much more than it's mentioned
>> in the description and brings a lot of changes in the existing lock
>> code.
>>
>> It seems like we should only change posix_file_lock_wait to your
>> do_vfs_lock in 'out' label and cifs_posix_lock_set for posix_lck case
>> and map flock to brlock from 0 to INF for !posix_lck case.
>>
>
> When coding this, I followed the NFS client code which adds more checks
> and balances to ensure that the local and remote locks do not go out of
> sync.
>
> Sachin Prabhu
>
>



-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] Add support for flock over a cifs mount.

[Sachin Prabhu]

On Tue, 2011-11-08 at 16:21 +0400, Pavel Shilovsky wrote:

> >> > @@ -1231,12 +1248,27 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
> >> >        struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> >> >        struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
> >> >        __u16 netfid = cfile->netfid;
> >> > +       unsigned char fl_flags = flock->fl_flags;
> >> > +
> >> > +       if (unlock) {
> >> > +               /*Check for existance of lock*/
> >> > +               flock->fl_flags |= FL_EXISTS;
> >> > +               if (do_vfs_lock(flock->fl_file, flock) == -ENOENT)
> >> > +                       goto out;
> >> > +       } else {
> >> > +               /*Check if we can obtain lock*/
> >> > +               flock->fl_flags |= FL_ACCESS;
> >> > +               rc = do_vfs_lock(flock->fl_file, flock);
> >> > +               if (rc < 0)
> >> > +                       goto out;
> >> > +       }
> >> > +       flock->fl_flags = fl_flags;
> >>
> >> I think this should be skipped for non-posix locks. These locks have
> >> their own cache (cifs_lock_* functions) associated with cifsInodeInfo
> >> structure - so, we can't make a solution to 'goto out;' without
> >> looking at this cache.
> >
> > I could follow Jeff's suggestion here and take the BUG out of
> > do_vfs_lock(). In case of non posix/non flocks, the do_vfs_lock will
> > simply return 0 and not trigger the goto out and proceed normally.
> 
> Sorry, I think I wasn't clear. In CIFS we have two locks variant
> according to whether we use POSIX extensions or not. In the case of
> POSIX extensions enabled we use VFS brlock cache and it is mirrors the
> locks we have on the remote side. So, everything is good in this case.
> 
> Let's look at the case when POSIX extensions are disabled - Windows
> locking style. The brlocks behavior changes and we use our own cache
> for them but we need to call posix_file_lock_wait at the end of
> cifs_setlk  to make some use-cases work (e.g. unlock locks of the
> child process when it finishes, etc) . And these locks are still
> FL_POSIX locks from VFS point of view.

Pavel,

The code behaves in a similar manner to the previous versions of the
code in case of non posix locks. This simply adds support for FL_FLOCK
in addition to Posix lock. 

Let me explain what has to happen for the goto out to be called in the 2
if conditions which seem to be the problem.

1) 
        if (unlock) {
                /*Check for existance of lock*/
                flock->fl_flags |= FL_EXISTS; 
                if (do_vfs_lock(flock->fl_file, flock) == -ENOENT)
                        goto out;

The comment associated with FL_EXISTS in include/linux/fs.h is 
#define FL_EXISTS       16      /* when unlocking, test for existence */

If this was an unlock request and the call to unlock the file reports
that the lock doesn't exists then goto out and exit with -ENOENT. This
is correct since there is no lock on this file to unlock.

If the lock did exist, then it unlocks the lock, proceeds normally and
calls cifs_unlock_range(cfile, flock, xid) further below which unlocks
the lock held in the local brlocks cache.

2)        
	 } else {
                /*Check if we can obtain lock*/
                flock->fl_flags |= FL_ACCESS;
                rc = do_vfs_lock(flock->fl_file, flock);
                if (rc < 0)
                        goto out;


The comment associated with FL_ACCESS in include/linux/fs.h is 
#define FL_ACCESS       8       /* not trying to lock, just looking */

It attempts to lock the file. If this attempt fails, it refuses to do
the remote lock call since we will not be able to get a local lock in
this case. In this case, exit with the error message returned by
do_vfs_lock().

If it face no problems obtaining a lock locally then proceed to perform
normal locking operations as before and finally obtain a lock on the
local system with 

out_lock:
        if (rc == 0 && lock) {
                /*We have to sleep here*/
                flock->fl_flags |= FL_SLEEP;
                do_vfs_lock(file, flock);
        }

We have to use do_vfs_lock() in this case since we are now dealing with
both flocks as well as posix locks. The same use case you explained
earlier holds where any existing flocks and posix locks on the file have
to be cleared when the file is closed. The old call to
posix_lock_file_wait will only end up adding a posix lock to the file
when we instead need a flock.


Sachin Prabhu

Re: [PATCH] Add support for flock over a cifs mount.

[Pavel Shilovsky]

2011/11/8 Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Pavel,
>
> The code behaves in a similar manner to the previous versions of the
> code in case of non posix locks. This simply adds support for FL_FLOCK
> in addition to Posix lock.
>
> Let me explain what has to happen for the goto out to be called in the 2
> if conditions which seem to be the problem.
>
> 1)
>        if (unlock) {
>                /*Check for existance of lock*/
>                flock->fl_flags |= FL_EXISTS;
>                if (do_vfs_lock(flock->fl_file, flock) == -ENOENT)
>                        goto out;
>
> The comment associated with FL_EXISTS in include/linux/fs.h is
> #define FL_EXISTS       16      /* when unlocking, test for existence */
>
> If this was an unlock request and the call to unlock the file reports
> that the lock doesn't exists then goto out and exit with -ENOENT. This
> is correct since there is no lock on this file to unlock.
>
> If the lock did exist, then it unlocks the lock, proceeds normally and
> calls cifs_unlock_range(cfile, flock, xid) further below which unlocks
> the lock held in the local brlocks cache.

Let's predict the following situation:

1) f1 = open(filename)
2) f2 = open(filename) # the same process
3) f1.lock(0, 1) # lock file from 0 to 1
4) f2.lock(1, 2) # lock file from 1 to 2
5) f1.unlock(0, 2)

it unlocks both locks (3) and (4) from the VFS cache but unlock only
(3) lock from CIFS-lock cache and server side (see cifs_unlock_range -
it unlocks only locks held by the same fd, not the same process) - the
lock (4) is still there.

6) f2.unlock(1, 2)

it appears into if (unlock) {}, fails on FL_EXISTS with -ENOENT and
goto out. The result we can't unlock the existing lock - the problem!

>
> 2)
>         } else {
>                /*Check if we can obtain lock*/
>                flock->fl_flags |= FL_ACCESS;
>                rc = do_vfs_lock(flock->fl_file, flock);
>                if (rc < 0)
>                        goto out;
>
>
> The comment associated with FL_ACCESS in include/linux/fs.h is
> #define FL_ACCESS       8       /* not trying to lock, just looking */
>
> It attempts to lock the file. If this attempt fails, it refuses to do
> the remote lock call since we will not be able to get a local lock in
> this case. In this case, exit with the error message returned by
> do_vfs_lock().
>
> If it face no problems obtaining a lock locally then proceed to perform
> normal locking operations as before and finally obtain a lock on the
> local system with
>
> out_lock:
>        if (rc == 0 && lock) {
>                /*We have to sleep here*/
>                flock->fl_flags |= FL_SLEEP;
>                do_vfs_lock(file, flock);
>        }
>
> We have to use do_vfs_lock() in this case since we are now dealing with
> both flocks as well as posix locks. The same use case you explained
> earlier holds where any existing flocks and posix locks on the file have
> to be cleared when the file is closed. The old call to
> posix_lock_file_wait will only end up adding a posix lock to the file
> when we instead need a flock.
>
>
> Sachin Prabhu
>
>



-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] Add support for flock over a cifs mount.

[Sachin Prabhu]

On Tue, 2011-11-08 at 23:08 +0400, Pavel Shilovsky wrote:

> Let's predict the following situation:
> 
> 1) f1 = open(filename)
> 2) f2 = open(filename) # the same process
> 3) f1.lock(0, 1) # lock file from 0 to 1
> 4) f2.lock(1, 2) # lock file from 1 to 2
> 5) f1.unlock(0, 2)
> 
> it unlocks both locks (3) and (4) from the VFS cache but unlock only
> (3) lock from CIFS-lock cache and server side (see cifs_unlock_range -
> it unlocks only locks held by the same fd, not the same process) - the
> lock (4) is still there.
> 
> 6) f2.unlock(1, 2)
> 
> it appears into if (unlock) {}, fails on FL_EXISTS with -ENOENT and
> goto out. The result we can't unlock the existing lock - the problem!
> 

Hello Pavel,

Thanks for your reply.

In my opinion this behaviour is wrong. The application running on the
Linux box will expect the locking behaviour to follow Posix locking
semantics. The current behaviour will break these applications which
expect the system to work in this manner.

After step 5, the client should unlock the whole range and send 2
separate unlock requests to the file server using 2 different netfids if
required.


Sachin Prabhu

Re: [PATCH] Add support for flock over a cifs mount.

[Pavel Shilovsky]

2011/11/11 Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Tue, 2011-11-08 at 23:08 +0400, Pavel Shilovsky wrote:
>
>> Let's predict the following situation:
>>
>> 1) f1 = open(filename)
>> 2) f2 = open(filename) # the same process
>> 3) f1.lock(0, 1) # lock file from 0 to 1
>> 4) f2.lock(1, 2) # lock file from 1 to 2
>> 5) f1.unlock(0, 2)
>>
>> it unlocks both locks (3) and (4) from the VFS cache but unlock only
>> (3) lock from CIFS-lock cache and server side (see cifs_unlock_range -
>> it unlocks only locks held by the same fd, not the same process) - the
>> lock (4) is still there.
>>
>> 6) f2.unlock(1, 2)
>>
>> it appears into if (unlock) {}, fails on FL_EXISTS with -ENOENT and
>> goto out. The result we can't unlock the existing lock - the problem!
>>
>
> Hello Pavel,
>
> Thanks for your reply.
>
> In my opinion this behaviour is wrong. The application running on the
> Linux box will expect the locking behaviour to follow Posix locking
> semantics. The current behaviour will break these applications which
> expect the system to work in this manner.
>
> After step 5, the client should unlock the whole range and send 2
> separate unlock requests to the file server using 2 different netfids if
> required.
>
>

I agree with you - that's wrong according to POSIX. I only wanted to
point that your patch can't live with the existing code behavior. May
be it is the time to change this long life bug.

I think flock patch should not bring any new behavior (even if it
fixes something) and should be split into several patches:
1) add flock support only (no changes into the existing code behavior);
2), 3), ...) - any other changes (bugfixes) you want to make one-by-one.

It lets us bisect possible problems if they appear to be.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] Add support for flock over a cifs mount.

[Sachin Prabhu]

On Mon, 2011-11-14 at 16:54 +0400, Pavel Shilovsky wrote:
> 2011/11/11 Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > On Tue, 2011-11-08 at 23:08 +0400, Pavel Shilovsky wrote:
> >
> >> Let's predict the following situation:
> >>
> >> 1) f1 = open(filename)
> >> 2) f2 = open(filename) # the same process
> >> 3) f1.lock(0, 1) # lock file from 0 to 1
> >> 4) f2.lock(1, 2) # lock file from 1 to 2
> >> 5) f1.unlock(0, 2)
> >>
> >> it unlocks both locks (3) and (4) from the VFS cache but unlock only
> >> (3) lock from CIFS-lock cache and server side (see cifs_unlock_range -
> >> it unlocks only locks held by the same fd, not the same process) - the
> >> lock (4) is still there.
> >>
> >> 6) f2.unlock(1, 2)
> >>
> >> it appears into if (unlock) {}, fails on FL_EXISTS with -ENOENT and
> >> goto out. The result we can't unlock the existing lock - the problem!
> >>
> >
> > Hello Pavel,
> >
> > Thanks for your reply.
> >
> > In my opinion this behaviour is wrong. The application running on the
> > Linux box will expect the locking behaviour to follow Posix locking
> > semantics. The current behaviour will break these applications which
> > expect the system to work in this manner.
> >
> > After step 5, the client should unlock the whole range and send 2
> > separate unlock requests to the file server using 2 different netfids if
> > required.
> >
> >
> 
> I agree with you - that's wrong according to POSIX. I only wanted to
> point that your patch can't live with the existing code behavior. May
> be it is the time to change this long life bug.
> 
> I think flock patch should not bring any new behavior (even if it
> fixes something) and should be split into several patches:
> 1) add flock support only (no changes into the existing code behavior);
> 2), 3), ...) - any other changes (bugfixes) you want to make one-by-one.
> 
> It lets us bisect possible problems if they appear to be.
> 

I have a proposed patch but need some additional time to test it before
I send it up. I'll do it as soon as it is ready.

Sachin Prabhu

RFC: Attach locks to cifsFileInfo instead of cifsInodeInfo

[Sachin Prabhu]

Pavel,

I've made some changes to the locking code which makes it easier to
unlock all locks on an inode. This fixes the earlier problem where
unlock  requests would only process the locks for that open file
instance while ignoring the locks on other open file instances for that
inode.

However we still have one major issue. In case of unlocks, the current
code will only return locks which are completely within the range
specified by the unlock command. If we get an unlock request which
divides a  brlock into 2, we ignore the unlock request for that brlock.
This will result in a situation where the application finds a lock on a
part of the file it expected to be unlocked. Should we check for these
cases and return an EFAULT in cases where the unlock request will not
completely unlock the entire range?

Sachin Prabhu

---- 

Attach locks to cifsFileInfo instead of cifsInodeInfo

Locks in CIFS are attached to an open file instances ie. all
lock/unlock requests made to the file server should contain the fileid
which represents the open file instance. Grouping all brlocks according
to the fileid therefore makes it easier to process especially when
multiple lock or unlock requests for a file are to be made.

Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 8f1fe32..8a71b0d 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -949,7 +949,6 @@ cifs_init_once(void *inode)
 	struct cifsInodeInfo *cifsi = inode;
 
 	inode_init_once(&cifsi->vfs_inode);
-	INIT_LIST_HEAD(&cifsi->llist);
 	mutex_init(&cifsi->lock_mutex);
 }
 
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 8238aa1..409d932 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -527,6 +527,7 @@ struct cifs_search_info {
 struct cifsFileInfo {
 	struct list_head tlist;	/* pointer to next fid owned by tcon */
 	struct list_head flist;	/* next fid (file instance) for this inode */
+	struct list_head llist;	/* brlocks for this file */
 	unsigned int uid;	/* allows finding which FileInfo structure */
 	__u32 pid;		/* process id who opened file */
 	__u16 netfid;		/* file id from remote */
@@ -567,9 +568,9 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file);
  */
 
 struct cifsInodeInfo {
-	struct list_head llist;		/* brlocks for this inode */
 	bool can_cache_brlcks;
-	struct mutex lock_mutex;	/* protect two fields above */
+	struct mutex lock_mutex;	/* protect cifsFileInfo->llist
+					   & cifsInodeInfo->can_cache_brlcks */
 	/* BB add in lists for dirty pages i.e. write caching info for oplock */
 	struct list_head openFileList;
 	__u32 cifsAttrs; /* e.g. DOS archive bit, sparse, compressed, system */
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index cf0b153..3f6b169 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -264,6 +264,7 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 	pCifsFile->tlink = cifs_get_tlink(tlink);
 	mutex_init(&pCifsFile->fh_mutex);
 	INIT_WORK(&pCifsFile->oplock_break, cifs_oplock_break);
+	INIT_LIST_HEAD(&pCifsFile->llist);
 
 	spin_lock(&cifs_file_list_lock);
 	list_add(&pCifsFile->tlist, &(tlink_tcon(tlink)->openFileList));
@@ -333,15 +334,11 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	/* Delete any outstanding lock records. We'll lose them when the file
 	 * is closed anyway.
 	 */
-	mutex_lock(&cifsi->lock_mutex);
-	list_for_each_entry_safe(li, tmp, &cifsi->llist, llist) {
-		if (li->netfid != cifs_file->netfid)
-			continue;
+	list_for_each_entry_safe(li, tmp, &cifs_file->llist, llist) {
 		list_del(&li->llist);
 		cifs_del_lock_waiters(li);
 		kfree(li);
 	}
-	mutex_unlock(&cifsi->lock_mutex);
 
 	cifs_put_tlink(cifs_file->tlink);
 	dput(cifs_file->dentry);
@@ -672,47 +669,82 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock)
 }
 
 static bool
-__cifs_find_lock_conflict(struct cifsInodeInfo *cinode, __u64 offset,
-			__u64 length, __u8 type, __u16 netfid,
-			struct cifsLockInfo **conf_lock)
+__cifs_find_lock_conflict_file(struct cifsFileInfo *cifs_file, __u64 offset,
+			       __u64 length, __u8 type,
+			       struct cifsLockInfo **conf_lock)
 {
-	struct cifsLockInfo *li, *tmp;
+	struct cifsLockInfo *li;
 
-	list_for_each_entry_safe(li, tmp, &cinode->llist, llist) {
+	list_for_each_entry(li, &cifs_file->llist, llist) {
+		/* Check if range matches */
 		if (offset + length <= li->offset ||
 		    offset >= li->offset + li->length)
 			continue;
-		else if ((type & LOCKING_ANDX_SHARED_LOCK) &&
-			 ((netfid == li->netfid && current->tgid == li->pid) ||
-			  type == li->type))
-			continue;
-		else {
-			*conf_lock = li;
-			return true;
+		/* Is the request for a shared lock */
+		if (type == LOCKING_ANDX_SHARED_LOCK) {
+			/* Does this process already posses a lock? */
+			if (current->tgid == li->pid)
+				return false;
+			/* Is the existing lock a shared lock too? */
+			if (li->type == LOCKING_ANDX_SHARED_LOCK)
+				return false;
 		}
+		/* We have found a conflicting lock */
+		*conf_lock = li;
+		return true;
+	}
+	return false;
+}
+
+static bool
+__cifs_find_lock_conflict(struct cifsFileInfo *cifs_file, __u64 offset,
+			  __u64 length, __u8 type,
+			  struct cifsLockInfo **conf_lock)
+{
+	struct cifsFileInfo *file, *tmp;
+	bool rc;
+	struct cifsInodeInfo *cifs_inode = CIFS_I(cifs_file->dentry->d_inode);
+
+	/* We need to check across every open instance of the inode */
+	spin_lock(&cifs_file_list_lock);
+	list_for_each_entry_safe(file, tmp, &cifs_inode->openFileList, flist) {
+		cifsFileInfo_get(file);
+		spin_unlock(&cifs_file_list_lock);
+
+		rc = __cifs_find_lock_conflict_file(file, offset, length,
+						    type, conf_lock);
+		if (rc)
+			return true;
+
+		cifsFileInfo_put(file);
+		spin_lock(&cifs_file_list_lock);
 	}
+	spin_unlock(&cifs_file_list_lock);
+
 	return false;
 }
 
 static bool
-cifs_find_lock_conflict(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock,
+cifs_find_lock_conflict(struct cifsFileInfo *cifs_file,
+			struct cifsLockInfo *lock,
 			struct cifsLockInfo **conf_lock)
 {
-	return __cifs_find_lock_conflict(cinode, lock->offset, lock->length,
-					 lock->type, lock->netfid, conf_lock);
+	return __cifs_find_lock_conflict(cifs_file, lock->offset, lock->length,
+					 lock->type, conf_lock);
 }
 
 static int
-cifs_lock_test(struct cifsInodeInfo *cinode, __u64 offset, __u64 length,
-	       __u8 type, __u16 netfid, struct file_lock *flock)
+cifs_lock_test(struct cifsFileInfo *cifs_file, __u64 offset, __u64 length,
+	       __u8 type, struct file_lock *flock)
 {
 	int rc = 0;
 	struct cifsLockInfo *conf_lock;
 	bool exist;
+	struct cifsInodeInfo *cinode = CIFS_I(cifs_file->dentry->d_inode);
 
 	mutex_lock(&cinode->lock_mutex);
 
-	exist = __cifs_find_lock_conflict(cinode, offset, length, type, netfid,
+	exist = __cifs_find_lock_conflict(cifs_file, offset, length, type,
 					  &conf_lock);
 	if (exist) {
 		flock->fl_start = conf_lock->offset;
@@ -732,18 +764,21 @@ cifs_lock_test(struct cifsInodeInfo *cinode, __u64 offset, __u64 length,
 }
 
 static void
-cifs_lock_add(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock)
+cifs_lock_add(struct cifsFileInfo *cifs_file, struct cifsLockInfo *lock)
 {
+	struct cifsInodeInfo *cinode = CIFS_I(cifs_file->dentry->d_inode);
+
 	mutex_lock(&cinode->lock_mutex);
-	list_add_tail(&lock->llist, &cinode->llist);
+	list_add_tail(&lock->llist, &cifs_file->llist);
 	mutex_unlock(&cinode->lock_mutex);
 }
 
 static int
-cifs_lock_add_if(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock,
+cifs_lock_add_if(struct cifsFileInfo *cifs_file, struct cifsLockInfo *lock,
 		 bool wait)
 {
 	struct cifsLockInfo *conf_lock;
+	struct cifsInodeInfo *cinode = CIFS_I(cifs_file->dentry->d_inode);
 	bool exist;
 	int rc = 0;
 
@@ -751,9 +786,9 @@ try_again:
 	exist = false;
 	mutex_lock(&cinode->lock_mutex);
 
-	exist = cifs_find_lock_conflict(cinode, lock, &conf_lock);
+	exist = cifs_find_lock_conflict(cifs_file, lock, &conf_lock);
 	if (!exist && cinode->can_cache_brlcks) {
-		list_add_tail(&lock->llist, &cinode->llist);
+		list_add_tail(&lock->llist, &cifs_file->llist);
 		mutex_unlock(&cinode->lock_mutex);
 		return rc;
 	}
@@ -823,7 +858,7 @@ static int
 cifs_push_mandatory_locks(struct cifsFileInfo *cfile)
 {
 	int xid, rc = 0, stored_rc;
-	struct cifsLockInfo *li, *tmp;
+	struct cifsLockInfo *li;
 	struct cifs_tcon *tcon;
 	struct cifsInodeInfo *cinode = CIFS_I(cfile->dentry->d_inode);
 	unsigned int num, max_num;
@@ -854,7 +889,7 @@ cifs_push_mandatory_locks(struct cifsFileInfo *cfile)
 	for (i = 0; i < 2; i++) {
 		cur = buf;
 		num = 0;
-		list_for_each_entry_safe(li, tmp, &cinode->llist, llist) {
+		list_for_each_entry(li, &cfile->llist, llist) {
 			if (li->type != types[i])
 				continue;
 			cur->Pid = cpu_to_le16(li->pid);
@@ -865,8 +900,10 @@ cifs_push_mandatory_locks(struct cifsFileInfo *cfile)
 			if (++num == max_num) {
 				stored_rc = cifs_lockv(xid, tcon, cfile->netfid,
 						       li->type, 0, num, buf);
-				if (stored_rc)
+				if (stored_rc) {
+					cERROR(1, "Error while syncing locks");
 					rc = stored_rc;
+				}
 				cur = buf;
 				num = 0;
 			} else
@@ -876,8 +913,10 @@ cifs_push_mandatory_locks(struct cifsFileInfo *cfile)
 		if (num) {
 			stored_rc = cifs_lockv(xid, tcon, cfile->netfid,
 					       types[i], 0, num, buf);
-			if (stored_rc)
+			if (stored_rc) {
+				cERROR(1, "Error while syncing locks");
 				rc = stored_rc;
+			}
 		}
 	}
 
@@ -1026,7 +1065,6 @@ cifs_getlk(struct file *file, struct file_lock *flock, __u8 type,
 	__u64 length = 1 + flock->fl_end - flock->fl_start;
 	struct cifsFileInfo *cfile = (struct cifsFileInfo *)file->private_data;
 	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
-	struct cifsInodeInfo *cinode = CIFS_I(cfile->dentry->d_inode);
 	__u16 netfid = cfile->netfid;
 
 	if (posix_lck) {
@@ -1046,8 +1084,7 @@ cifs_getlk(struct file *file, struct file_lock *flock, __u8 type,
 		return rc;
 	}
 
-	rc = cifs_lock_test(cinode, flock->fl_start, length, type, netfid,
-			    flock);
+	rc = cifs_lock_test(cfile, flock->fl_start, length, type, flock);
 	if (!rc)
 		return rc;
 
@@ -1108,7 +1145,8 @@ cifs_free_llist(struct list_head *llist)
 }
 
 static int
-cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, int xid)
+cifs_unlock_range_file(struct cifsFileInfo *cfile, struct file_lock *flock,
+		       int xid)
 {
 	int rc = 0, stored_rc;
 	int types[] = {LOCKING_ANDX_LARGE_FILES,
@@ -1134,15 +1172,11 @@ cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, int xid)
 	for (i = 0; i < 2; i++) {
 		cur = buf;
 		num = 0;
-		list_for_each_entry_safe(li, tmp, &cinode->llist, llist) {
+		list_for_each_entry_safe(li, tmp, &cfile->llist, llist) {
 			if (flock->fl_start > li->offset ||
 			    (flock->fl_start + length) <
 			    (li->offset + li->length))
 				continue;
-			if (current->tgid != li->pid)
-				continue;
-			if (cfile->netfid != li->netfid)
-				continue;
 			if (types[i] != li->type)
 				continue;
 			if (!cinode->can_cache_brlcks) {
@@ -1172,7 +1206,7 @@ cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, int xid)
 						 * the inode list.
 						 */
 						cifs_move_llist(&tmp_llist,
-								&cinode->llist);
+								&cfile->llist);
 						rc = stored_rc;
 					} else
 						/*
@@ -1198,7 +1232,7 @@ cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, int xid)
 			stored_rc = cifs_lockv(xid, tcon, cfile->netfid,
 					       types[i], num, 0, buf);
 			if (stored_rc) {
-				cifs_move_llist(&tmp_llist, &cinode->llist);
+				cifs_move_llist(&tmp_llist, &cfile->llist);
 				rc = stored_rc;
 			} else
 				cifs_free_llist(&tmp_llist);
@@ -1211,6 +1245,30 @@ cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, int xid)
 }
 
 static int
+cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, int xid)
+{
+	struct cifsFileInfo *file, *tmp;
+	struct cifsInodeInfo *cifs_inode = CIFS_I(cfile->dentry->d_inode);
+	int rc = 0;
+
+	spin_lock(&cifs_file_list_lock);
+	list_for_each_entry_safe(file, tmp, &cifs_inode->openFileList, flist) {
+		if (file->pid != current->pid)
+			continue;
+		cifsFileInfo_get(file);
+		spin_unlock(&cifs_file_list_lock);
+		rc = cifs_unlock_range_file(file, flock, xid);
+		if (rc < 0)
+			cFYI(1, "VFS is out of sync with lock on server!");
+		cifsFileInfo_put(file);
+		spin_lock(&cifs_file_list_lock);
+	}
+	spin_unlock(&cifs_file_list_lock);
+
+	return rc;
+}
+
+static int
 cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
 	   bool wait_flag, bool posix_lck, int lock, int unlock, int xid)
 {
@@ -1218,7 +1276,6 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
 	__u64 length = 1 + flock->fl_end - flock->fl_start;
 	struct cifsFileInfo *cfile = (struct cifsFileInfo *)file->private_data;
 	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
-	struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
 	__u16 netfid = cfile->netfid;
 
 	if (posix_lck) {
@@ -1249,7 +1306,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
 		if (!lock)
 			return -ENOMEM;
 
-		rc = cifs_lock_add_if(cinode, lock, wait_flag);
+		rc = cifs_lock_add_if(cfile, lock, wait_flag);
 		if (rc < 0)
 			kfree(lock);
 		if (rc <= 0)
@@ -1262,7 +1319,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
 			goto out;
 		}
 
-		cifs_lock_add(cinode, lock);
+		cifs_lock_add(cfile, lock);
 	} else if (unlock)
 		rc = cifs_unlock_range(cfile, flock, xid);
 

Re: RFC: Attach locks to cifsFileInfo instead of cifsInodeInfo

[Pavel Shilovsky]

2011/11/17 Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Pavel,
>
> I've made some changes to the locking code which makes it easier to
> unlock all locks on an inode. This fixes the earlier problem where
> unlock  requests would only process the locks for that open file
> instance while ignoring the locks on other open file instances for that
> inode.
>
> However we still have one major issue. In case of unlocks, the current
> code will only return locks which are completely within the range
> specified by the unlock command. If we get an unlock request which
> divides a  brlock into 2, we ignore the unlock request for that brlock.
> This will result in a situation where the application finds a lock on a
> part of the file it expected to be unlocked. Should we check for these
> cases and return an EFAULT in cases where the unlock request will not
> completely unlock the entire range?

Yes, I think that we should report to the userspace to let the
application know about the failure. I found this on fcntl man page:

ENOLCK
Too many segment locks open, lock table is full, or a remote locking
protocol failed (e.g., locking over NFS).

I think this fits our scenario.

>
> Sachin Prabhu
>
> ----
>
> Attach locks to cifsFileInfo instead of cifsInodeInfo
>
> Locks in CIFS are attached to an open file instances ie. all
> lock/unlock requests made to the file server should contain the fileid
> which represents the open file instance. Grouping all brlocks according
> to the fileid therefore makes it easier to process especially when
> multiple lock or unlock requests for a file are to be made.
>
> Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 8f1fe32..8a71b0d 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -949,7 +949,6 @@ cifs_init_once(void *inode)
>        struct cifsInodeInfo *cifsi = inode;
>
>        inode_init_once(&cifsi->vfs_inode);
> -       INIT_LIST_HEAD(&cifsi->llist);
>        mutex_init(&cifsi->lock_mutex);
>  }
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 8238aa1..409d932 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -527,6 +527,7 @@ struct cifs_search_info {
>  struct cifsFileInfo {
>        struct list_head tlist; /* pointer to next fid owned by tcon */
>        struct list_head flist; /* next fid (file instance) for this inode */
> +       struct list_head llist; /* brlocks for this file */
>        unsigned int uid;       /* allows finding which FileInfo structure */
>        __u32 pid;              /* process id who opened file */
>        __u16 netfid;           /* file id from remote */
> @@ -567,9 +568,9 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file);
>  */
>
>  struct cifsInodeInfo {
> -       struct list_head llist;         /* brlocks for this inode */
>        bool can_cache_brlcks;
> -       struct mutex lock_mutex;        /* protect two fields above */
> +       struct mutex lock_mutex;        /* protect cifsFileInfo->llist
> +                                          & cifsInodeInfo->can_cache_brlcks */
>        /* BB add in lists for dirty pages i.e. write caching info for oplock */
>        struct list_head openFileList;
>        __u32 cifsAttrs; /* e.g. DOS archive bit, sparse, compressed, system */
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index cf0b153..3f6b169 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -264,6 +264,7 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
>        pCifsFile->tlink = cifs_get_tlink(tlink);
>        mutex_init(&pCifsFile->fh_mutex);
>        INIT_WORK(&pCifsFile->oplock_break, cifs_oplock_break);
> +       INIT_LIST_HEAD(&pCifsFile->llist);
>
>        spin_lock(&cifs_file_list_lock);
>        list_add(&pCifsFile->tlist, &(tlink_tcon(tlink)->openFileList));
> @@ -333,15 +334,11 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>        /* Delete any outstanding lock records. We'll lose them when the file
>         * is closed anyway.
>         */
> -       mutex_lock(&cifsi->lock_mutex);
> -       list_for_each_entry_safe(li, tmp, &cifsi->llist, llist) {
> -               if (li->netfid != cifs_file->netfid)
> -                       continue;
> +       list_for_each_entry_safe(li, tmp, &cifs_file->llist, llist) {
>                list_del(&li->llist);
>                cifs_del_lock_waiters(li);
>                kfree(li);
>        }
> -       mutex_unlock(&cifsi->lock_mutex);
>
>        cifs_put_tlink(cifs_file->tlink);
>        dput(cifs_file->dentry);
> @@ -672,47 +669,82 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock)
>  }
>
>  static bool
> -__cifs_find_lock_conflict(struct cifsInodeInfo *cinode, __u64 offset,
> -                       __u64 length, __u8 type, __u16 netfid,
> -                       struct cifsLockInfo **conf_lock)
> +__cifs_find_lock_conflict_file(struct cifsFileInfo *cifs_file, __u64 offset,
> +                              __u64 length, __u8 type,
> +                              struct cifsLockInfo **conf_lock)
>  {
> -       struct cifsLockInfo *li, *tmp;
> +       struct cifsLockInfo *li;
>
> -       list_for_each_entry_safe(li, tmp, &cinode->llist, llist) {
> +       list_for_each_entry(li, &cifs_file->llist, llist) {
> +               /* Check if range matches */
>                if (offset + length <= li->offset ||
>                    offset >= li->offset + li->length)
>                        continue;
> -               else if ((type & LOCKING_ANDX_SHARED_LOCK) &&
> -                        ((netfid == li->netfid && current->tgid == li->pid) ||
> -                         type == li->type))
> -                       continue;
> -               else {
> -                       *conf_lock = li;
> -                       return true;
> +               /* Is the request for a shared lock */
> +               if (type == LOCKING_ANDX_SHARED_LOCK) {
> +                       /* Does this process already posses a lock? */
> +                       if (current->tgid == li->pid)
> +                               return false;
You should check if netfids match too, because it is not allowed to
set a shared lock by one handle to an exclusive lock by another.

> +                       /* Is the existing lock a shared lock too? */
> +                       if (li->type == LOCKING_ANDX_SHARED_LOCK)
> +                               return false;
>                }
> +               /* We have found a conflicting lock */
> +               *conf_lock = li;
> +               return true;
> +       }
> +       return false;
> +}
> +
> +static bool
> +__cifs_find_lock_conflict(struct cifsFileInfo *cifs_file, __u64 offset,
> +                         __u64 length, __u8 type,
> +                         struct cifsLockInfo **conf_lock)
> +{
> +       struct cifsFileInfo *file, *tmp;
> +       bool rc;
> +       struct cifsInodeInfo *cifs_inode = CIFS_I(cifs_file->dentry->d_inode);
> +
> +       /* We need to check across every open instance of the inode */
> +       spin_lock(&cifs_file_list_lock);
> +       list_for_each_entry_safe(file, tmp, &cifs_inode->openFileList, flist) {
> +               cifsFileInfo_get(file);
> +               spin_unlock(&cifs_file_list_lock);
> +
> +               rc = __cifs_find_lock_conflict_file(file, offset, length,
> +                                                   type, conf_lock);
> +               if (rc)
> +                       return true;
> +
> +               cifsFileInfo_put(file);
> +               spin_lock(&cifs_file_list_lock);
>        }
> +       spin_unlock(&cifs_file_list_lock);
> +
>        return false;
>  }
>
>  static bool
> -cifs_find_lock_conflict(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock,
> +cifs_find_lock_conflict(struct cifsFileInfo *cifs_file,
> +                       struct cifsLockInfo *lock,
>                        struct cifsLockInfo **conf_lock)
>  {
> -       return __cifs_find_lock_conflict(cinode, lock->offset, lock->length,
> -                                        lock->type, lock->netfid, conf_lock);
> +       return __cifs_find_lock_conflict(cifs_file, lock->offset, lock->length,
> +                                        lock->type, conf_lock);
>  }
>
>  static int
> -cifs_lock_test(struct cifsInodeInfo *cinode, __u64 offset, __u64 length,
> -              __u8 type, __u16 netfid, struct file_lock *flock)
> +cifs_lock_test(struct cifsFileInfo *cifs_file, __u64 offset, __u64 length,
> +              __u8 type, struct file_lock *flock)
>  {
>        int rc = 0;
>        struct cifsLockInfo *conf_lock;
>        bool exist;
> +       struct cifsInodeInfo *cinode = CIFS_I(cifs_file->dentry->d_inode);
>
>        mutex_lock(&cinode->lock_mutex);
>
> -       exist = __cifs_find_lock_conflict(cinode, offset, length, type, netfid,
> +       exist = __cifs_find_lock_conflict(cifs_file, offset, length, type,
>                                          &conf_lock);
>        if (exist) {
>                flock->fl_start = conf_lock->offset;
> @@ -732,18 +764,21 @@ cifs_lock_test(struct cifsInodeInfo *cinode, __u64 offset, __u64 length,
>  }
>
>  static void
> -cifs_lock_add(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock)
> +cifs_lock_add(struct cifsFileInfo *cifs_file, struct cifsLockInfo *lock)
>  {
> +       struct cifsInodeInfo *cinode = CIFS_I(cifs_file->dentry->d_inode);
> +
>        mutex_lock(&cinode->lock_mutex);
> -       list_add_tail(&lock->llist, &cinode->llist);
> +       list_add_tail(&lock->llist, &cifs_file->llist);
>        mutex_unlock(&cinode->lock_mutex);
>  }
>
>  static int
> -cifs_lock_add_if(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock,
> +cifs_lock_add_if(struct cifsFileInfo *cifs_file, struct cifsLockInfo *lock,
>                 bool wait)
>  {
>        struct cifsLockInfo *conf_lock;
> +       struct cifsInodeInfo *cinode = CIFS_I(cifs_file->dentry->d_inode);
>        bool exist;
>        int rc = 0;
>
> @@ -751,9 +786,9 @@ try_again:
>        exist = false;
>        mutex_lock(&cinode->lock_mutex);
>
> -       exist = cifs_find_lock_conflict(cinode, lock, &conf_lock);
> +       exist = cifs_find_lock_conflict(cifs_file, lock, &conf_lock);
>        if (!exist && cinode->can_cache_brlcks) {
> -               list_add_tail(&lock->llist, &cinode->llist);
> +               list_add_tail(&lock->llist, &cifs_file->llist);
>                mutex_unlock(&cinode->lock_mutex);
>                return rc;
>        }
> @@ -823,7 +858,7 @@ static int
>  cifs_push_mandatory_locks(struct cifsFileInfo *cfile)
>  {
>        int xid, rc = 0, stored_rc;
> -       struct cifsLockInfo *li, *tmp;
> +       struct cifsLockInfo *li;
>        struct cifs_tcon *tcon;
>        struct cifsInodeInfo *cinode = CIFS_I(cfile->dentry->d_inode);
>        unsigned int num, max_num;
> @@ -854,7 +889,7 @@ cifs_push_mandatory_locks(struct cifsFileInfo *cfile)
>        for (i = 0; i < 2; i++) {
>                cur = buf;
>                num = 0;
> -               list_for_each_entry_safe(li, tmp, &cinode->llist, llist) {
> +               list_for_each_entry(li, &cfile->llist, llist) {
>                        if (li->type != types[i])
>                                continue;
>                        cur->Pid = cpu_to_le16(li->pid);
> @@ -865,8 +900,10 @@ cifs_push_mandatory_locks(struct cifsFileInfo *cfile)
>                        if (++num == max_num) {
>                                stored_rc = cifs_lockv(xid, tcon, cfile->netfid,
>                                                       li->type, 0, num, buf);
> -                               if (stored_rc)
> +                               if (stored_rc) {
> +                                       cERROR(1, "Error while syncing locks");
>                                        rc = stored_rc;
> +                               }
>                                cur = buf;
>                                num = 0;
>                        } else
> @@ -876,8 +913,10 @@ cifs_push_mandatory_locks(struct cifsFileInfo *cfile)
>                if (num) {
>                        stored_rc = cifs_lockv(xid, tcon, cfile->netfid,
>                                               types[i], 0, num, buf);
> -                       if (stored_rc)
> +                       if (stored_rc) {
> +                               cERROR(1, "Error while syncing locks");
>                                rc = stored_rc;
> +                       }
>                }
>        }
>
> @@ -1026,7 +1065,6 @@ cifs_getlk(struct file *file, struct file_lock *flock, __u8 type,
>        __u64 length = 1 + flock->fl_end - flock->fl_start;
>        struct cifsFileInfo *cfile = (struct cifsFileInfo *)file->private_data;
>        struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> -       struct cifsInodeInfo *cinode = CIFS_I(cfile->dentry->d_inode);
>        __u16 netfid = cfile->netfid;
>
>        if (posix_lck) {
> @@ -1046,8 +1084,7 @@ cifs_getlk(struct file *file, struct file_lock *flock, __u8 type,
>                return rc;
>        }
>
> -       rc = cifs_lock_test(cinode, flock->fl_start, length, type, netfid,
> -                           flock);
> +       rc = cifs_lock_test(cfile, flock->fl_start, length, type, flock);
>        if (!rc)
>                return rc;
>
> @@ -1108,7 +1145,8 @@ cifs_free_llist(struct list_head *llist)
>  }
>
>  static int
> -cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, int xid)
> +cifs_unlock_range_file(struct cifsFileInfo *cfile, struct file_lock *flock,
> +                      int xid)
>  {
>        int rc = 0, stored_rc;
>        int types[] = {LOCKING_ANDX_LARGE_FILES,
> @@ -1134,15 +1172,11 @@ cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, int xid)
>        for (i = 0; i < 2; i++) {
>                cur = buf;
>                num = 0;
> -               list_for_each_entry_safe(li, tmp, &cinode->llist, llist) {
> +               list_for_each_entry_safe(li, tmp, &cfile->llist, llist) {
>                        if (flock->fl_start > li->offset ||
>                            (flock->fl_start + length) <
>                            (li->offset + li->length))
>                                continue;
> -                       if (current->tgid != li->pid)
> -                               continue;
> -                       if (cfile->netfid != li->netfid)
> -                               continue;
>                        if (types[i] != li->type)
>                                continue;
>                        if (!cinode->can_cache_brlcks) {
> @@ -1172,7 +1206,7 @@ cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, int xid)
>                                                 * the inode list.
>                                                 */
>                                                cifs_move_llist(&tmp_llist,
> -                                                               &cinode->llist);
> +                                                               &cfile->llist);
>                                                rc = stored_rc;
>                                        } else
>                                                /*
> @@ -1198,7 +1232,7 @@ cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, int xid)
>                        stored_rc = cifs_lockv(xid, tcon, cfile->netfid,
>                                               types[i], num, 0, buf);
>                        if (stored_rc) {
> -                               cifs_move_llist(&tmp_llist, &cinode->llist);
> +                               cifs_move_llist(&tmp_llist, &cfile->llist);
>                                rc = stored_rc;
>                        } else
>                                cifs_free_llist(&tmp_llist);
> @@ -1211,6 +1245,30 @@ cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, int xid)
>  }
>
>  static int
> +cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, int xid)
> +{
> +       struct cifsFileInfo *file, *tmp;
> +       struct cifsInodeInfo *cifs_inode = CIFS_I(cfile->dentry->d_inode);
> +       int rc = 0;
> +
> +       spin_lock(&cifs_file_list_lock);
> +       list_for_each_entry_safe(file, tmp, &cifs_inode->openFileList, flist) {
> +               if (file->pid != current->pid)
> +                       continue;
The same cifsFileInfo structure can be shared between several
processes and every can set a brlock on it. But file->pid still has
the pid only of the process who opened the file. So, such a check
should be done in cifs_unlock_range_file with lock->pid.

> +               cifsFileInfo_get(file);
> +               spin_unlock(&cifs_file_list_lock);
> +               rc = cifs_unlock_range_file(file, flock, xid);
> +               if (rc < 0)
> +                       cFYI(1, "VFS is out of sync with lock on server!");
> +               cifsFileInfo_put(file);
> +               spin_lock(&cifs_file_list_lock);
> +       }
> +       spin_unlock(&cifs_file_list_lock);
> +
> +       return rc;
> +}
> +
> +static int
>  cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>           bool wait_flag, bool posix_lck, int lock, int unlock, int xid)
>  {
> @@ -1218,7 +1276,6 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>        __u64 length = 1 + flock->fl_end - flock->fl_start;
>        struct cifsFileInfo *cfile = (struct cifsFileInfo *)file->private_data;
>        struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> -       struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>        __u16 netfid = cfile->netfid;
>
>        if (posix_lck) {
> @@ -1249,7 +1306,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>                if (!lock)
>                        return -ENOMEM;
>
> -               rc = cifs_lock_add_if(cinode, lock, wait_flag);
> +               rc = cifs_lock_add_if(cfile, lock, wait_flag);
>                if (rc < 0)
>                        kfree(lock);
>                if (rc <= 0)
> @@ -1262,7 +1319,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>                        goto out;
>                }
>
> -               cifs_lock_add(cinode, lock);
> +               cifs_lock_add(cfile, lock);
>        } else if (unlock)
>                rc = cifs_unlock_range(cfile, flock, xid);
>
>
>

In general, I like your idea to group brlocks to the file structure
(that reverts my previous change) but this should be done in two steps
to make the history bisectable:
1) move locks to cifsFileInfo,
2) change cifs_unlock_range to unlock all process locks that are in
the requested unlock range.

-- 
Best regards,
Pavel Shilovsky.