On Sat, 31 Mar 2012 00:25:01 +0400 Pavel Shilovsky <[email protected]> wrote:
> CIFS brlock cache can be used by several file handles if we have a > write-caching lease on the file that is supported by SMB2 protocol. > Prepate the code to handle this situation correctly by sorting brlocks > by a fid to easily push them in portions when lease break comes. > > Signed-off-by: Pavel Shilovsky <[email protected]> > --- > fs/cifs/cifsfs.c | 1 - > fs/cifs/cifsglob.h | 12 +++++-- > fs/cifs/file.c | 89 ++++++++++++++++++++++++++++----------------------- > 3 files changed, 58 insertions(+), 44 deletions(-) > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > index d342128..5105f23 100644 > --- a/fs/cifs/cifsfs.c > +++ b/fs/cifs/cifsfs.c > @@ -941,7 +941,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 4ff6313..de1a80d 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -548,7 +548,6 @@ struct cifsLockInfo { > __u64 length; > __u32 pid; > __u8 type; > - __u16 netfid; > }; > > /* > @@ -573,6 +572,10 @@ 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 held by this fid, protected by > + * lock_mutex from cifsInodeInfo structure > + */ > unsigned int uid; /* allows finding which FileInfo structure */ > __u32 pid; /* process id who opened file */ > __u16 netfid; /* file id from remote */ > @@ -615,9 +618,12 @@ 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 the field above and llist > from > + * every cifsFileInfo structure from > + * openFileList > + */ > /* 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 fae765d..f258373 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)); > @@ -334,9 +335,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_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); > @@ -645,7 +644,7 @@ int cifs_closedir(struct inode *inode, struct file *file) > } > > static struct cifsLockInfo * > -cifs_lock_init(__u64 offset, __u64 length, __u8 type, __u16 netfid) > +cifs_lock_init(__u64 offset, __u64 length, __u8 type) > { > struct cifsLockInfo *lock = > kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL); > @@ -654,7 +653,6 @@ cifs_lock_init(__u64 offset, __u64 length, __u8 type, > __u16 netfid) > lock->offset = offset; > lock->length = length; > lock->type = type; > - lock->netfid = netfid; > lock->pid = current->tgid; > INIT_LIST_HEAD(&lock->blist); > init_waitqueue_head(&lock->block_q); > @@ -672,19 +670,19 @@ 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_fid_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, > + __u64 length, __u8 type, __u16 netfid, > + 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, &cfile->llist, llist) { > 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)) > + ((netfid == cfile->netfid && current->tgid == li->pid) > + || type == li->type)) > continue; > else { > *conf_lock = li; > @@ -695,11 +693,23 @@ __cifs_find_lock_conflict(struct cifsInodeInfo *cinode, > __u64 offset, > } > > static bool > -cifs_find_lock_conflict(struct cifsInodeInfo *cinode, struct cifsLockInfo > *lock, > +cifs_find_lock_conflict(struct cifsInodeInfo *cinode, __u64 offset, > + __u64 length, __u8 type, __u16 netfid, > struct cifsLockInfo **conf_lock) > { > - return __cifs_find_lock_conflict(cinode, lock->offset, lock->length, > - lock->type, lock->netfid, conf_lock); > + bool rc = false; > + struct cifsFileInfo *fid, *tmp; > + > + spin_lock(&cifs_file_list_lock); > + list_for_each_entry_safe(fid, tmp, &cinode->openFileList, flist) { No need to use list_for_each_entry_safe here. You aren't altering the list. (Right?) > + rc = cifs_find_fid_lock_conflict(fid, offset, length, type, > + netfid, conf_lock); > + if (rc) > + break; > + } > + spin_unlock(&cifs_file_list_lock); > + > + return rc; > } > > /* > @@ -710,17 +720,18 @@ cifs_find_lock_conflict(struct cifsInodeInfo *cinode, > struct cifsLockInfo *lock, > * the server or 1 otherwise. > */ > 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 *cfile, __u64 offset, __u64 length, > + __u8 type, struct file_lock *flock) > { > int rc = 0; > struct cifsLockInfo *conf_lock; > + struct cifsInodeInfo *cinode = CIFS_I(cfile->dentry->d_inode); > bool exist; > > mutex_lock(&cinode->lock_mutex); > > - exist = __cifs_find_lock_conflict(cinode, offset, length, type, netfid, > - &conf_lock); > + exist = cifs_find_lock_conflict(cinode, offset, length, type, > + cfile->netfid, &conf_lock); > if (exist) { > flock->fl_start = conf_lock->offset; > flock->fl_end = conf_lock->offset + conf_lock->length - 1; > @@ -739,10 +750,11 @@ 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 *cfile, struct cifsLockInfo *lock) > { > + struct cifsInodeInfo *cinode = CIFS_I(cfile->dentry->d_inode); > mutex_lock(&cinode->lock_mutex); > - list_add_tail(&lock->llist, &cinode->llist); > + list_add_tail(&lock->llist, &cfile->llist); > mutex_unlock(&cinode->lock_mutex); > } > > @@ -753,10 +765,11 @@ cifs_lock_add(struct cifsInodeInfo *cinode, struct > cifsLockInfo *lock) > * 3) -EACCESS, if there is a lock that prevents us and wait is false. > */ > static int > -cifs_lock_add_if(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock, > +cifs_lock_add_if(struct cifsFileInfo *cfile, struct cifsLockInfo *lock, > bool wait) > { > struct cifsLockInfo *conf_lock; > + struct cifsInodeInfo *cinode = CIFS_I(cfile->dentry->d_inode); > bool exist; > int rc = 0; > > @@ -764,9 +777,10 @@ try_again: > exist = false; > mutex_lock(&cinode->lock_mutex); > > - exist = cifs_find_lock_conflict(cinode, lock, &conf_lock); > + exist = cifs_find_lock_conflict(cinode, lock->offset, lock->length, > + lock->type, cfile->netfid, &conf_lock); > if (!exist && cinode->can_cache_brlcks) { > - list_add_tail(&lock->llist, &cinode->llist); > + list_add_tail(&lock->llist, &cfile->llist); > mutex_unlock(&cinode->lock_mutex); > return rc; > } > @@ -888,7 +902,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_safe(li, tmp, &cfile->llist, llist) { ...ditto -- the non-safe variant should be fine here. > if (li->type != types[i]) > continue; > cur->Pid = cpu_to_le16(li->pid); > @@ -1104,7 +1118,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) { > @@ -1124,8 +1137,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; > > @@ -1212,15 +1224,13 @@ 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) { > @@ -1233,7 +1243,7 @@ cifs_unlock_range(struct cifsFileInfo *cfile, struct > file_lock *flock, int xid) > cpu_to_le32((u32)(li->offset>>32)); > /* > * We need to save a lock here to let us add > - * it again to the inode list if the unlock > + * it again to the file's list if the unlock > * range request fails on the server. > */ > list_move(&li->llist, &tmp_llist); > @@ -1247,10 +1257,10 @@ cifs_unlock_range(struct cifsFileInfo *cfile, struct > file_lock *flock, int xid) > * We failed on the unlock range > * request - add all locks from > * the tmp list to the head of > - * the inode list. > + * the file's list. > */ > cifs_move_llist(&tmp_llist, > - &cinode->llist); > + &cfile->llist); > rc = stored_rc; > } else > /* > @@ -1265,7 +1275,7 @@ cifs_unlock_range(struct cifsFileInfo *cfile, struct > file_lock *flock, int xid) > } else { > /* > * We can cache brlock requests - simply remove > - * a lock from the inode list. > + * a lock from the file's list. > */ > list_del(&li->llist); > cifs_del_lock_waiters(li); > @@ -1276,7 +1286,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); > @@ -1296,7 +1306,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) { > @@ -1323,11 +1332,11 @@ cifs_setlk(struct file *file, struct file_lock > *flock, __u8 type, > if (lock) { > struct cifsLockInfo *lock; > > - lock = cifs_lock_init(flock->fl_start, length, type, netfid); > + lock = cifs_lock_init(flock->fl_start, length, 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) > @@ -1340,7 +1349,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 any case, it's harmless to use the *_safe variant in those places, just slightly less efficient. I'd be fine if you want to clean those up in a later patch. Since I haven't seen where the later SMB2 patches require this change, I'll just have to take your word for it that it's necessary. So... Acked-by: Jeff Layton <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
