diff mbox

cifs: OFD locks do not conflict with eachothers

Message ID 20180627034540.30762-2-lsahlber@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ronnie Sahlberg June 27, 2018, 3:45 a.m. UTC
RHBZ 1484130

Update cifs_find_fid_lock_conflict() to recognize that
ODF locks do not conflict with eachother.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cifsglob.h  |  3 ++-
 fs/cifs/cifsproto.h |  2 +-
 fs/cifs/file.c      | 34 +++++++++++++++++++++-------------
 3 files changed, 24 insertions(+), 15 deletions(-)

Comments

Jeff Layton June 27, 2018, 4:15 a.m. UTC | #1
On Wed, 2018-06-27 at 13:45 +1000, Ronnie Sahlberg wrote:
> RHBZ 1484130
> 
> Update cifs_find_fid_lock_conflict() to recognize that
> ODF locks do not conflict with eachother.
> 
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/cifsglob.h  |  3 ++-
>  fs/cifs/cifsproto.h |  2 +-
>  fs/cifs/file.c      | 34 +++++++++++++++++++++-------------
>  3 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 0543187fe707..a0c92ed656c5 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1092,7 +1092,8 @@ struct cifsLockInfo {
>  	__u64 offset;
>  	__u64 length;
>  	__u32 pid;
> -	__u32 type;
> +	__u16 type;
> +	__u16 flags;
>  };
>  
>  /*
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index e23eec5372df..b8a6a228b1a7 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -214,7 +214,7 @@ extern void cifs_mark_open_files_invalid(struct cifs_tcon *tcon);
>  extern void cifs_reopen_persistent_handles(struct cifs_tcon *tcon);
>  
>  extern bool cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset,
> -				    __u64 length, __u8 type,
> +				    __u64 length, __u8 type, __u16 flags,
>  				    struct cifsLockInfo **conf_lock,
>  				    int rw_check);
>  extern void cifs_add_pending_open(struct cifs_fid *fid,
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 8d41ca7bfcf1..3efd150c61b3 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -864,7 +864,7 @@ int cifs_closedir(struct inode *inode, struct file *file)
>  }
>  
>  static struct cifsLockInfo *
> -cifs_lock_init(__u64 offset, __u64 length, __u8 type)
> +cifs_lock_init(__u64 offset, __u64 length, __u8 type, __u16 flags)
>  {
>  	struct cifsLockInfo *lock =
>  		kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL);
> @@ -874,6 +874,7 @@ cifs_lock_init(__u64 offset, __u64 length, __u8 type)
>  	lock->length = length;
>  	lock->type = type;
>  	lock->pid = current->tgid;
> +	lock->flags = flags;
>  	INIT_LIST_HEAD(&lock->blist);
>  	init_waitqueue_head(&lock->block_q);
>  	return lock;
> @@ -896,7 +897,8 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock)
>  /* @rw_check : 0 - no op, 1 - read, 2 - write */
>  static bool
>  cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
> -			    __u64 length, __u8 type, struct cifsFileInfo *cfile,
> +			    __u64 length, __u8 type, __u16 flags,
> +			    struct cifsFileInfo *cfile,
>  			    struct cifsLockInfo **conf_lock, int rw_check)
>  {
>  	struct cifsLockInfo *li;
> @@ -918,6 +920,9 @@ cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
>  		    ((server->ops->compare_fids(cfile, cur_cfile) &&
>  		     current->tgid == li->pid) || type == li->type))
>  			continue;
> +		if (rw_check == CIFS_LOCK_OP &&
> +		    flags & FL_OFDLCK && li->flags & FL_OFDLCK)

Maybe some parenthesis around those flags checks? I can never remember
whether && or & takes precedence.

> +			continue;

This looks like it'll fix the reported problem, but the logic in this
function just keeps getting more complex with all of the special
casing. It might be nice to start by documenting the logic with some
comments in this code and then maybe rework this function to be more
understandable.

Also, don't classic POSIX locks have the same problem here? If you do
s/F_OFD_/F_/ in Laura's test program, wouldn't it fail in the same way?

>  		if (conf_lock)
>  			*conf_lock = li;
>  		return true;
> @@ -927,8 +932,8 @@ cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
>  
>  bool
>  cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length,
> -			__u8 type, struct cifsLockInfo **conf_lock,
> -			int rw_check)
> +			__u8 type, __u16 flags,
> +			struct cifsLockInfo **conf_lock, int rw_check)
>  {
>  	bool rc = false;
>  	struct cifs_fid_locks *cur;
> @@ -936,7 +941,8 @@ cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length,
>  
>  	list_for_each_entry(cur, &cinode->llist, llist) {
>  		rc = cifs_find_fid_lock_conflict(cur, offset, length, type,
> -						 cfile, conf_lock, rw_check);
> +						 flags, cfile, conf_lock,
> +						 rw_check);
>  		if (rc)
>  			break;
>  	}
> @@ -964,7 +970,8 @@ cifs_lock_test(struct cifsFileInfo *cfile, __u64 offset, __u64 length,
>  	down_read(&cinode->lock_sem);
>  
>  	exist = cifs_find_lock_conflict(cfile, offset, length, type,
> -					&conf_lock, CIFS_LOCK_OP);
> +					flock->fl_flags, &conf_lock,
> +					CIFS_LOCK_OP);
>  	if (exist) {
>  		flock->fl_start = conf_lock->offset;
>  		flock->fl_end = conf_lock->offset + conf_lock->length - 1;
> @@ -1011,7 +1018,8 @@ cifs_lock_add_if(struct cifsFileInfo *cfile, struct cifsLockInfo *lock,
>  	down_write(&cinode->lock_sem);
>  
>  	exist = cifs_find_lock_conflict(cfile, lock->offset, lock->length,
> -					lock->type, &conf_lock, CIFS_LOCK_OP);
> +					lock->type, lock->flags, &conf_lock,
> +					CIFS_LOCK_OP);
>  	if (!exist && cinode->can_cache_brlcks) {
>  		list_add_tail(&lock->llist, &cfile->llist->locks);
>  		up_write(&cinode->lock_sem);
> @@ -1321,7 +1329,7 @@ cifs_read_flock(struct file_lock *flock, __u32 *type, int *lock, int *unlock,
>  		cifs_dbg(FYI, "Lease on file - not implemented yet\n");
>  	if (flock->fl_flags &
>  	    (~(FL_POSIX | FL_FLOCK | FL_SLEEP |
> -	       FL_ACCESS | FL_LEASE | FL_CLOSE)))
> +	       FL_ACCESS | FL_LEASE | FL_CLOSE | FL_OFDLCK)))
>  		cifs_dbg(FYI, "Unknown lock flags 0x%x\n", flock->fl_flags);
>  
>  	*type = server->vals->large_lock_type;
> @@ -1584,7 +1592,8 @@ cifs_setlk(struct file *file, struct file_lock *flock, __u32 type,
>  	if (lock) {
>  		struct cifsLockInfo *lock;
>  
> -		lock = cifs_lock_init(flock->fl_start, length, type);
> +		lock = cifs_lock_init(flock->fl_start, length, type,
> +				      flock->fl_flags);
>  		if (!lock)
>  			return -ENOMEM;
>  
> @@ -1653,7 +1662,6 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *flock)
>  
>  	cifs_read_flock(flock, &type, &lock, &unlock, &wait_flag,
>  			tcon->ses->server);
> -
>  	cifs_sb = CIFS_FILE_SB(file);
>  	netfid = cfile->fid.netfid;
>  	cinode = CIFS_I(file_inode(file));
> @@ -2817,8 +2825,8 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
>  		goto out;
>  
>  	if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from),
> -				     server->vals->exclusive_lock_type, NULL,
> -				     CIFS_WRITE_OP))
> +				     server->vals->exclusive_lock_type, 0,
> +				     NULL, CIFS_WRITE_OP))
>  		rc = __generic_file_write_iter(iocb, from);
>  	else
>  		rc = -EACCES;
> @@ -3388,7 +3396,7 @@ cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to)
>  	down_read(&cinode->lock_sem);
>  	if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(to),
>  				     tcon->ses->server->vals->shared_lock_type,
> -				     NULL, CIFS_READ_OP))
> +				     0, NULL, CIFS_READ_OP))
>  		rc = generic_file_read_iter(iocb, to);
>  	up_read(&cinode->lock_sem);
>  	return rc;
Pavel Shilovsky June 27, 2018, 12:08 p.m. UTC | #2
вт, 26 июн. 2018 г. в 22:03, Jeff Layton <jlayton@redhat.com>:
>
> On Wed, 2018-06-27 at 13:45 +1000, Ronnie Sahlberg wrote:
> > RHBZ 1484130
> >
> > Update cifs_find_fid_lock_conflict() to recognize that
> > ODF locks do not conflict with eachother.
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/cifsglob.h  |  3 ++-
> >  fs/cifs/cifsproto.h |  2 +-
> >  fs/cifs/file.c      | 34 +++++++++++++++++++++-------------
> >  3 files changed, 24 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 0543187fe707..a0c92ed656c5 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -1092,7 +1092,8 @@ struct cifsLockInfo {
> >       __u64 offset;
> >       __u64 length;
> >       __u32 pid;
> > -     __u32 type;
> > +     __u16 type;
> > +     __u16 flags;
> >  };
> >
> >  /*
> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> > index e23eec5372df..b8a6a228b1a7 100644
> > --- a/fs/cifs/cifsproto.h
> > +++ b/fs/cifs/cifsproto.h
> > @@ -214,7 +214,7 @@ extern void cifs_mark_open_files_invalid(struct cifs_tcon *tcon);
> >  extern void cifs_reopen_persistent_handles(struct cifs_tcon *tcon);
> >
> >  extern bool cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset,
> > -                                 __u64 length, __u8 type,
> > +                                 __u64 length, __u8 type, __u16 flags,
> >                                   struct cifsLockInfo **conf_lock,
> >                                   int rw_check);
> >  extern void cifs_add_pending_open(struct cifs_fid *fid,
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 8d41ca7bfcf1..3efd150c61b3 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -864,7 +864,7 @@ int cifs_closedir(struct inode *inode, struct file *file)
> >  }
> >
> >  static struct cifsLockInfo *
> > -cifs_lock_init(__u64 offset, __u64 length, __u8 type)
> > +cifs_lock_init(__u64 offset, __u64 length, __u8 type, __u16 flags)
> >  {
> >       struct cifsLockInfo *lock =
> >               kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL);
> > @@ -874,6 +874,7 @@ cifs_lock_init(__u64 offset, __u64 length, __u8 type)
> >       lock->length = length;
> >       lock->type = type;
> >       lock->pid = current->tgid;
> > +     lock->flags = flags;
> >       INIT_LIST_HEAD(&lock->blist);
> >       init_waitqueue_head(&lock->block_q);
> >       return lock;
> > @@ -896,7 +897,8 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock)
> >  /* @rw_check : 0 - no op, 1 - read, 2 - write */
> >  static bool
> >  cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
> > -                         __u64 length, __u8 type, struct cifsFileInfo *cfile,
> > +                         __u64 length, __u8 type, __u16 flags,
> > +                         struct cifsFileInfo *cfile,
> >                           struct cifsLockInfo **conf_lock, int rw_check)
> >  {
> >       struct cifsLockInfo *li;
> > @@ -918,6 +920,9 @@ cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
> >                   ((server->ops->compare_fids(cfile, cur_cfile) &&
> >                    current->tgid == li->pid) || type == li->type))
> >                       continue;
> > +             if (rw_check == CIFS_LOCK_OP &&
> > +                 flags & FL_OFDLCK && li->flags & FL_OFDLCK)
>
> Maybe some parenthesis around those flags checks? I can never remember
> whether && or & takes precedence.
>
> > +                     continue;
>
> This looks like it'll fix the reported problem, but the logic in this
> function just keeps getting more complex with all of the special
> casing. It might be nice to start by documenting the logic with some
> comments in this code and then maybe rework this function to be more
> understandable.
>
> Also, don't classic POSIX locks have the same problem here? If you do
> s/F_OFD_/F_/ in Laura's test program, wouldn't it fail in the same way?

As far as I remember cifs_find_fid_lock_conflict() does the same logic
the SMB server would do if the client decided to skip internal checks
and proceeded to the remote server directly. So, for classic POSIX
locks, the proposed test program should fail too. I don't think there
is much we can do in general case to fix it, but we can fix the case
where locks are set/get within the same mount.

Anyway, this check:

> > +             if (rw_check == CIFS_LOCK_OP &&
> > +                 flags & FL_OFDLCK && li->flags & FL_OFDLCK)
> > +                     continue;

doesn't look right: it basically allows any FD with a WRITE lock to
overwrite the existing READ lock held by a different FD. It seems that
we need to check if file ids are equal here.

--
Best regards,
Pavel Shilovsky
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 0543187fe707..a0c92ed656c5 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1092,7 +1092,8 @@  struct cifsLockInfo {
 	__u64 offset;
 	__u64 length;
 	__u32 pid;
-	__u32 type;
+	__u16 type;
+	__u16 flags;
 };
 
 /*
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index e23eec5372df..b8a6a228b1a7 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -214,7 +214,7 @@  extern void cifs_mark_open_files_invalid(struct cifs_tcon *tcon);
 extern void cifs_reopen_persistent_handles(struct cifs_tcon *tcon);
 
 extern bool cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset,
-				    __u64 length, __u8 type,
+				    __u64 length, __u8 type, __u16 flags,
 				    struct cifsLockInfo **conf_lock,
 				    int rw_check);
 extern void cifs_add_pending_open(struct cifs_fid *fid,
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 8d41ca7bfcf1..3efd150c61b3 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -864,7 +864,7 @@  int cifs_closedir(struct inode *inode, struct file *file)
 }
 
 static struct cifsLockInfo *
-cifs_lock_init(__u64 offset, __u64 length, __u8 type)
+cifs_lock_init(__u64 offset, __u64 length, __u8 type, __u16 flags)
 {
 	struct cifsLockInfo *lock =
 		kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL);
@@ -874,6 +874,7 @@  cifs_lock_init(__u64 offset, __u64 length, __u8 type)
 	lock->length = length;
 	lock->type = type;
 	lock->pid = current->tgid;
+	lock->flags = flags;
 	INIT_LIST_HEAD(&lock->blist);
 	init_waitqueue_head(&lock->block_q);
 	return lock;
@@ -896,7 +897,8 @@  cifs_del_lock_waiters(struct cifsLockInfo *lock)
 /* @rw_check : 0 - no op, 1 - read, 2 - write */
 static bool
 cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
-			    __u64 length, __u8 type, struct cifsFileInfo *cfile,
+			    __u64 length, __u8 type, __u16 flags,
+			    struct cifsFileInfo *cfile,
 			    struct cifsLockInfo **conf_lock, int rw_check)
 {
 	struct cifsLockInfo *li;
@@ -918,6 +920,9 @@  cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
 		    ((server->ops->compare_fids(cfile, cur_cfile) &&
 		     current->tgid == li->pid) || type == li->type))
 			continue;
+		if (rw_check == CIFS_LOCK_OP &&
+		    flags & FL_OFDLCK && li->flags & FL_OFDLCK)
+			continue;
 		if (conf_lock)
 			*conf_lock = li;
 		return true;
@@ -927,8 +932,8 @@  cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
 
 bool
 cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length,
-			__u8 type, struct cifsLockInfo **conf_lock,
-			int rw_check)
+			__u8 type, __u16 flags,
+			struct cifsLockInfo **conf_lock, int rw_check)
 {
 	bool rc = false;
 	struct cifs_fid_locks *cur;
@@ -936,7 +941,8 @@  cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length,
 
 	list_for_each_entry(cur, &cinode->llist, llist) {
 		rc = cifs_find_fid_lock_conflict(cur, offset, length, type,
-						 cfile, conf_lock, rw_check);
+						 flags, cfile, conf_lock,
+						 rw_check);
 		if (rc)
 			break;
 	}
@@ -964,7 +970,8 @@  cifs_lock_test(struct cifsFileInfo *cfile, __u64 offset, __u64 length,
 	down_read(&cinode->lock_sem);
 
 	exist = cifs_find_lock_conflict(cfile, offset, length, type,
-					&conf_lock, CIFS_LOCK_OP);
+					flock->fl_flags, &conf_lock,
+					CIFS_LOCK_OP);
 	if (exist) {
 		flock->fl_start = conf_lock->offset;
 		flock->fl_end = conf_lock->offset + conf_lock->length - 1;
@@ -1011,7 +1018,8 @@  cifs_lock_add_if(struct cifsFileInfo *cfile, struct cifsLockInfo *lock,
 	down_write(&cinode->lock_sem);
 
 	exist = cifs_find_lock_conflict(cfile, lock->offset, lock->length,
-					lock->type, &conf_lock, CIFS_LOCK_OP);
+					lock->type, lock->flags, &conf_lock,
+					CIFS_LOCK_OP);
 	if (!exist && cinode->can_cache_brlcks) {
 		list_add_tail(&lock->llist, &cfile->llist->locks);
 		up_write(&cinode->lock_sem);
@@ -1321,7 +1329,7 @@  cifs_read_flock(struct file_lock *flock, __u32 *type, int *lock, int *unlock,
 		cifs_dbg(FYI, "Lease on file - not implemented yet\n");
 	if (flock->fl_flags &
 	    (~(FL_POSIX | FL_FLOCK | FL_SLEEP |
-	       FL_ACCESS | FL_LEASE | FL_CLOSE)))
+	       FL_ACCESS | FL_LEASE | FL_CLOSE | FL_OFDLCK)))
 		cifs_dbg(FYI, "Unknown lock flags 0x%x\n", flock->fl_flags);
 
 	*type = server->vals->large_lock_type;
@@ -1584,7 +1592,8 @@  cifs_setlk(struct file *file, struct file_lock *flock, __u32 type,
 	if (lock) {
 		struct cifsLockInfo *lock;
 
-		lock = cifs_lock_init(flock->fl_start, length, type);
+		lock = cifs_lock_init(flock->fl_start, length, type,
+				      flock->fl_flags);
 		if (!lock)
 			return -ENOMEM;
 
@@ -1653,7 +1662,6 @@  int cifs_lock(struct file *file, int cmd, struct file_lock *flock)
 
 	cifs_read_flock(flock, &type, &lock, &unlock, &wait_flag,
 			tcon->ses->server);
-
 	cifs_sb = CIFS_FILE_SB(file);
 	netfid = cfile->fid.netfid;
 	cinode = CIFS_I(file_inode(file));
@@ -2817,8 +2825,8 @@  cifs_writev(struct kiocb *iocb, struct iov_iter *from)
 		goto out;
 
 	if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from),
-				     server->vals->exclusive_lock_type, NULL,
-				     CIFS_WRITE_OP))
+				     server->vals->exclusive_lock_type, 0,
+				     NULL, CIFS_WRITE_OP))
 		rc = __generic_file_write_iter(iocb, from);
 	else
 		rc = -EACCES;
@@ -3388,7 +3396,7 @@  cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to)
 	down_read(&cinode->lock_sem);
 	if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(to),
 				     tcon->ses->server->vals->shared_lock_type,
-				     NULL, CIFS_READ_OP))
+				     0, NULL, CIFS_READ_OP))
 		rc = generic_file_read_iter(iocb, to);
 	up_read(&cinode->lock_sem);
 	return rc;