* Re: [PATCH 2/2] cifs: Handle OFD locks [not found] ` <20180629205500.7950-2-paulo@paulo.ac> @ 2018-06-29 23:41 ` Steve French 2018-06-30 4:05 ` Eryu Guan 0 siblings, 1 reply; 3+ messages in thread From: Steve French @ 2018-06-29 23:41 UTC (permalink / raw) To: Paulo Alcantara, fstests; +Cc: linux-fsdevel Paulo, Did you find out if there were any xfstests for OFD locks, or whether you could add one of yours or Jeff's to xfstests for this? On Fri, Jun 29, 2018 at 3:55 PM Paulo Alcantara <paulo@paulo.ac> wrote: > > Correctly check for OFD lock conflicts. > > Signed-off-by: Paulo Alcantara <palcantara@suse.de> > --- > fs/cifs/cifsglob.h | 1 + > fs/cifs/cifsproto.h | 4 -- > fs/cifs/file.c | 93 ++++++++++++++++++++++++++++++--------------- > 3 files changed, 63 insertions(+), 35 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index a2962fd41c6f..a627559be72e 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -1093,6 +1093,7 @@ struct cifsLockInfo { > __u64 length; > __u32 pid; > __u32 type; > + unsigned int flags; /* file lock flags*/ > }; > > /* > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > index 1890f534c88b..870ff791a170 100644 > --- a/fs/cifs/cifsproto.h > +++ b/fs/cifs/cifsproto.h > @@ -214,10 +214,6 @@ extern void cifs_umount(struct cifs_sb_info *); > 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, > - struct cifsLockInfo **conf_lock, > - int rw_check); > extern void cifs_add_pending_open(struct cifs_fid *fid, > struct tcon_link *tlink, > struct cifs_pending_open *open); > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index 259167f32580..7c7c505fab3e 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -864,16 +864,17 @@ int cifs_closedir(struct inode *inode, struct file *file) > } > > static struct cifsLockInfo * > -cifs_lock_init(__u64 offset, __u64 length, __u8 type) > +cifs_lock_init(struct file_lock *flock, __u64 length, __u8 type) > { > struct cifsLockInfo *lock = > kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL); > if (!lock) > return lock; > - lock->offset = offset; > + lock->offset = flock->fl_start; > lock->length = length; > lock->type = type; > lock->pid = current->tgid; > + lock->flags = flock->fl_flags; > INIT_LIST_HEAD(&lock->blist); > init_waitqueue_head(&lock->block_q); > return lock; > @@ -893,31 +894,56 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock) > #define CIFS_READ_OP 1 > #define CIFS_WRITE_OP 2 > > -/* @rw_check : 0 - no op, 1 - read, 2 - write */ > +static inline bool locks_overlap(struct cifsLockInfo *li, __u64 offset, > + __u64 length) > +{ > + return offset + length > li->offset && > + li->offset + li->length > offset; > +} > + > static bool > -cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset, > - __u64 length, __u8 type, struct cifsFileInfo *cfile, > - struct cifsLockInfo **conf_lock, int rw_check) > +locks_conflict(struct cifsFileInfo *cfile, struct cifsFileInfo *cur_cfile, > + struct cifsLockInfo *li, __u8 type, unsigned int flags, > + struct TCP_Server_Info *server, int rw_check) > +{ > + if (rw_check == CIFS_LOCK_OP) { > + if (!server->ops->compare_fids(cfile, cur_cfile)) > + return true; > + if ((flags & FL_FLOCK) && current->tgid == li->pid) > + return false; > + if ((flags & FL_OFDLCK) && (li->flags & FL_OFDLCK)) > + return false; > + if (li->type & server->vals->exclusive_lock_type) > + return true; > + if (type & server->vals->exclusive_lock_type) > + return true; > + return false; > + } > + > + if (server->ops->compare_fids(cfile, cur_cfile) && > + current->tgid == li->pid && > + (!(li->type & server->vals->shared_lock_type) || > + rw_check != CIFS_WRITE_OP)) > + return false; > + > + return true; > +} > + > +static inline bool > +__fid_lock_conflicts(struct cifs_fid_locks *fdlocks, __u64 offset, __u64 length, > + __u8 type, unsigned int flags, struct cifsFileInfo *cfile, > + struct cifsLockInfo **conf_lock, int rw_check) > { > struct cifsLockInfo *li; > struct cifsFileInfo *cur_cfile = fdlocks->cfile; > struct TCP_Server_Info *server = tlink_tcon(cfile->tlink)->ses->server; > > list_for_each_entry(li, &fdlocks->locks, llist) { > - if (offset + length <= li->offset || > - offset >= li->offset + li->length) > - continue; > - if (rw_check != CIFS_LOCK_OP && current->tgid == li->pid && > - server->ops->compare_fids(cfile, cur_cfile)) { > - /* shared lock prevents write op through the same fid */ > - if (!(li->type & server->vals->shared_lock_type) || > - rw_check != CIFS_WRITE_OP) > - continue; > - } > - if ((type & server->vals->shared_lock_type) && > - ((server->ops->compare_fids(cfile, cur_cfile) && > - current->tgid == li->pid) || type == li->type)) > + if (!locks_overlap(li, offset, length) || > + !locks_conflict(cfile, cur_cfile, li, type, flags, server, > + rw_check)) > continue; > + > if (conf_lock) > *conf_lock = li; > return true; > @@ -925,18 +951,19 @@ cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset, > return false; > } > > -bool > +static bool > cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length, > - __u8 type, struct cifsLockInfo **conf_lock, > - int rw_check) > + __u8 type, unsigned int flags, > + struct cifsLockInfo **conf_lock, int rw_check) > { > bool rc = false; > struct cifs_fid_locks *cur; > struct cifsInodeInfo *cinode = CIFS_I(d_inode(cfile->dentry)); > > list_for_each_entry(cur, &cinode->llist, llist) { > - rc = cifs_find_fid_lock_conflict(cur, offset, length, type, > - cfile, conf_lock, rw_check); > + rc = __fid_lock_conflicts(cur, offset, length, type, > + flags, cfile, conf_lock, > + rw_check); > if (rc) > break; > } > @@ -964,7 +991,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 +1039,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->flags, lock->type, &conf_lock, > + CIFS_LOCK_OP); > if (!exist && cinode->can_cache_brlcks) { > list_add_tail(&lock->llist, &cfile->llist->locks); > up_write(&cinode->lock_sem); > @@ -1307,7 +1336,7 @@ static int cifs_read_flock(struct file_lock *flock, __u32 *type, bool *lock, > bool *wait_flag, struct TCP_Server_Info *server) > { > unsigned int flags = FL_POSIX | FL_FLOCK | FL_SLEEP | FL_ACCESS | > - FL_LEASE | FL_CLOSE; > + FL_LEASE | FL_CLOSE | FL_OFDLCK; > > if (flock->fl_flags & ~flags) { > cifs_dbg(FYI, "Unknown lock flags 0x%x\n", flock->fl_flags); > @@ -1318,7 +1347,9 @@ static int cifs_read_flock(struct file_lock *flock, __u32 *type, bool *lock, > if (flock->fl_flags & FL_POSIX) > cifs_dbg(FYI, "Posix\n"); > if (flock->fl_flags & FL_FLOCK) > - cifs_dbg(FYI, "Flock\n"); > + cifs_dbg(FYI, "File lock\n"); > + if (flock->fl_flags & FL_OFDLCK) > + cifs_dbg(FYI, "OFD lock\n"); > if (flock->fl_flags & FL_ACCESS) > cifs_dbg(FYI, "Process suspended by mandatory locking - not implemented yet\n"); > if (flock->fl_flags & FL_LEASE) > @@ -1587,7 +1618,7 @@ static int 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, length, type); > if (!lock) > return -ENOMEM; > > @@ -2814,7 +2845,7 @@ 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, > + server->vals->exclusive_lock_type, 0, NULL, > CIFS_WRITE_OP)) > rc = __generic_file_write_iter(iocb, from); > else > @@ -3385,7 +3416,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; > -- > 2.17.1 > -- Thanks, Steve ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] cifs: Handle OFD locks 2018-06-29 23:41 ` [PATCH 2/2] cifs: Handle OFD locks Steve French @ 2018-06-30 4:05 ` Eryu Guan 2018-06-30 18:42 ` Paulo Alcantara 0 siblings, 1 reply; 3+ messages in thread From: Eryu Guan @ 2018-06-30 4:05 UTC (permalink / raw) To: Steve French; +Cc: Paulo Alcantara, fstests, linux-fsdevel On Fri, Jun 29, 2018 at 06:41:40PM -0500, Steve French wrote: > Paulo, > Did you find out if there were any xfstests for OFD locks, or whether > you could add > one of yours or Jeff's to xfstests for this? generic/478 is an OFD lock test, but I'm not sure if the CIFS bug could be covered by it. Good to have new tests if not already covered :) Thanks, Eryu > On Fri, Jun 29, 2018 at 3:55 PM Paulo Alcantara <paulo@paulo.ac> wrote: > > > > Correctly check for OFD lock conflicts. > > > > Signed-off-by: Paulo Alcantara <palcantara@suse.de> > > --- > > fs/cifs/cifsglob.h | 1 + > > fs/cifs/cifsproto.h | 4 -- > > fs/cifs/file.c | 93 ++++++++++++++++++++++++++++++--------------- > > 3 files changed, 63 insertions(+), 35 deletions(-) > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > > index a2962fd41c6f..a627559be72e 100644 > > --- a/fs/cifs/cifsglob.h > > +++ b/fs/cifs/cifsglob.h > > @@ -1093,6 +1093,7 @@ struct cifsLockInfo { > > __u64 length; > > __u32 pid; > > __u32 type; > > + unsigned int flags; /* file lock flags*/ > > }; > > > > /* > > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > > index 1890f534c88b..870ff791a170 100644 > > --- a/fs/cifs/cifsproto.h > > +++ b/fs/cifs/cifsproto.h > > @@ -214,10 +214,6 @@ extern void cifs_umount(struct cifs_sb_info *); > > 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, > > - struct cifsLockInfo **conf_lock, > > - int rw_check); > > extern void cifs_add_pending_open(struct cifs_fid *fid, > > struct tcon_link *tlink, > > struct cifs_pending_open *open); > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > > index 259167f32580..7c7c505fab3e 100644 > > --- a/fs/cifs/file.c > > +++ b/fs/cifs/file.c > > @@ -864,16 +864,17 @@ int cifs_closedir(struct inode *inode, struct file *file) > > } > > > > static struct cifsLockInfo * > > -cifs_lock_init(__u64 offset, __u64 length, __u8 type) > > +cifs_lock_init(struct file_lock *flock, __u64 length, __u8 type) > > { > > struct cifsLockInfo *lock = > > kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL); > > if (!lock) > > return lock; > > - lock->offset = offset; > > + lock->offset = flock->fl_start; > > lock->length = length; > > lock->type = type; > > lock->pid = current->tgid; > > + lock->flags = flock->fl_flags; > > INIT_LIST_HEAD(&lock->blist); > > init_waitqueue_head(&lock->block_q); > > return lock; > > @@ -893,31 +894,56 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock) > > #define CIFS_READ_OP 1 > > #define CIFS_WRITE_OP 2 > > > > -/* @rw_check : 0 - no op, 1 - read, 2 - write */ > > +static inline bool locks_overlap(struct cifsLockInfo *li, __u64 offset, > > + __u64 length) > > +{ > > + return offset + length > li->offset && > > + li->offset + li->length > offset; > > +} > > + > > static bool > > -cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset, > > - __u64 length, __u8 type, struct cifsFileInfo *cfile, > > - struct cifsLockInfo **conf_lock, int rw_check) > > +locks_conflict(struct cifsFileInfo *cfile, struct cifsFileInfo *cur_cfile, > > + struct cifsLockInfo *li, __u8 type, unsigned int flags, > > + struct TCP_Server_Info *server, int rw_check) > > +{ > > + if (rw_check == CIFS_LOCK_OP) { > > + if (!server->ops->compare_fids(cfile, cur_cfile)) > > + return true; > > + if ((flags & FL_FLOCK) && current->tgid == li->pid) > > + return false; > > + if ((flags & FL_OFDLCK) && (li->flags & FL_OFDLCK)) > > + return false; > > + if (li->type & server->vals->exclusive_lock_type) > > + return true; > > + if (type & server->vals->exclusive_lock_type) > > + return true; > > + return false; > > + } > > + > > + if (server->ops->compare_fids(cfile, cur_cfile) && > > + current->tgid == li->pid && > > + (!(li->type & server->vals->shared_lock_type) || > > + rw_check != CIFS_WRITE_OP)) > > + return false; > > + > > + return true; > > +} > > + > > +static inline bool > > +__fid_lock_conflicts(struct cifs_fid_locks *fdlocks, __u64 offset, __u64 length, > > + __u8 type, unsigned int flags, struct cifsFileInfo *cfile, > > + struct cifsLockInfo **conf_lock, int rw_check) > > { > > struct cifsLockInfo *li; > > struct cifsFileInfo *cur_cfile = fdlocks->cfile; > > struct TCP_Server_Info *server = tlink_tcon(cfile->tlink)->ses->server; > > > > list_for_each_entry(li, &fdlocks->locks, llist) { > > - if (offset + length <= li->offset || > > - offset >= li->offset + li->length) > > - continue; > > - if (rw_check != CIFS_LOCK_OP && current->tgid == li->pid && > > - server->ops->compare_fids(cfile, cur_cfile)) { > > - /* shared lock prevents write op through the same fid */ > > - if (!(li->type & server->vals->shared_lock_type) || > > - rw_check != CIFS_WRITE_OP) > > - continue; > > - } > > - if ((type & server->vals->shared_lock_type) && > > - ((server->ops->compare_fids(cfile, cur_cfile) && > > - current->tgid == li->pid) || type == li->type)) > > + if (!locks_overlap(li, offset, length) || > > + !locks_conflict(cfile, cur_cfile, li, type, flags, server, > > + rw_check)) > > continue; > > + > > if (conf_lock) > > *conf_lock = li; > > return true; > > @@ -925,18 +951,19 @@ cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset, > > return false; > > } > > > > -bool > > +static bool > > cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length, > > - __u8 type, struct cifsLockInfo **conf_lock, > > - int rw_check) > > + __u8 type, unsigned int flags, > > + struct cifsLockInfo **conf_lock, int rw_check) > > { > > bool rc = false; > > struct cifs_fid_locks *cur; > > struct cifsInodeInfo *cinode = CIFS_I(d_inode(cfile->dentry)); > > > > list_for_each_entry(cur, &cinode->llist, llist) { > > - rc = cifs_find_fid_lock_conflict(cur, offset, length, type, > > - cfile, conf_lock, rw_check); > > + rc = __fid_lock_conflicts(cur, offset, length, type, > > + flags, cfile, conf_lock, > > + rw_check); > > if (rc) > > break; > > } > > @@ -964,7 +991,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 +1039,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->flags, lock->type, &conf_lock, > > + CIFS_LOCK_OP); > > if (!exist && cinode->can_cache_brlcks) { > > list_add_tail(&lock->llist, &cfile->llist->locks); > > up_write(&cinode->lock_sem); > > @@ -1307,7 +1336,7 @@ static int cifs_read_flock(struct file_lock *flock, __u32 *type, bool *lock, > > bool *wait_flag, struct TCP_Server_Info *server) > > { > > unsigned int flags = FL_POSIX | FL_FLOCK | FL_SLEEP | FL_ACCESS | > > - FL_LEASE | FL_CLOSE; > > + FL_LEASE | FL_CLOSE | FL_OFDLCK; > > > > if (flock->fl_flags & ~flags) { > > cifs_dbg(FYI, "Unknown lock flags 0x%x\n", flock->fl_flags); > > @@ -1318,7 +1347,9 @@ static int cifs_read_flock(struct file_lock *flock, __u32 *type, bool *lock, > > if (flock->fl_flags & FL_POSIX) > > cifs_dbg(FYI, "Posix\n"); > > if (flock->fl_flags & FL_FLOCK) > > - cifs_dbg(FYI, "Flock\n"); > > + cifs_dbg(FYI, "File lock\n"); > > + if (flock->fl_flags & FL_OFDLCK) > > + cifs_dbg(FYI, "OFD lock\n"); > > if (flock->fl_flags & FL_ACCESS) > > cifs_dbg(FYI, "Process suspended by mandatory locking - not implemented yet\n"); > > if (flock->fl_flags & FL_LEASE) > > @@ -1587,7 +1618,7 @@ static int 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, length, type); > > if (!lock) > > return -ENOMEM; > > > > @@ -2814,7 +2845,7 @@ 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, > > + server->vals->exclusive_lock_type, 0, NULL, > > CIFS_WRITE_OP)) > > rc = __generic_file_write_iter(iocb, from); > > else > > @@ -3385,7 +3416,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; > > -- > > 2.17.1 > > > > > -- > Thanks, > > Steve > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] cifs: Handle OFD locks 2018-06-30 4:05 ` Eryu Guan @ 2018-06-30 18:42 ` Paulo Alcantara 0 siblings, 0 replies; 3+ messages in thread From: Paulo Alcantara @ 2018-06-30 18:42 UTC (permalink / raw) To: Eryu Guan, Steve French; +Cc: fstests, linux-fsdevel Eryu Guan <guaneryu@gmail.com> writes: > On Fri, Jun 29, 2018 at 06:41:40PM -0500, Steve French wrote: >> Paulo, >> Did you find out if there were any xfstests for OFD locks, or whether >> you could add >> one of yours or Jeff's to xfstests for this? Nope. I didn't know there was a xfstest for that. I just performed my own quick tests (based on Jeff's) to test the POSIX and OFD locks after the changes. > generic/478 is an OFD lock test, but I'm not sure if the CIFS bug could > be covered by it. Good to have new tests if not already covered :) Cool! I'll give it a try and see whether it can be covered by the CIFS bug. Otherwise, I'll make sure to add new test cases to it (if time allows). Thanks guys! Paulo ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-06-30 18:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20180629205500.7950-1-paulo@paulo.ac>
[not found] ` <20180629205500.7950-2-paulo@paulo.ac>
2018-06-29 23:41 ` [PATCH 2/2] cifs: Handle OFD locks Steve French
2018-06-30 4:05 ` Eryu Guan
2018-06-30 18:42 ` Paulo Alcantara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox