[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-cifs
Subject: Re: [PATCH v3 1/5] CIFS: Move locks to cifsFileInfo structure
From: Pavel Shilovsky <piastry () etersoft ! ru>
Date: 2012-03-30 12:03:03
Message-ID: CAKywueR96=jtjbSUM5BFZY56egFqCxZNCEY9Uc06C1FzUpszWw () mail ! gmail ! com
[Download RAW message or body]
29 อมาิม 2012ว. 23:13 ะฯฬฺุฯืมิลฬุ Jeff Layton <jlayton@redhat.com> ฮมะษำมฬ:
> On Tue, 27 Mar 2012 15:38:35 +0400
> Pavel Shilovsky <piastry@etersoft.ru> 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 <piastry@etersoft.ru>
>
> I'm not sure I understand this. It seems like keeping them all as part
> of the inode would be simpler. How does it simplify things to put them
> in separate per-FID buckets?
>
> I suppose one argument is that you could open the file twice and get 2
> separate FIDs and 2 separate leases, and have different locks against
> each. Then the server might recall one lease but not the other. In that
> case, you'd want to push out only the locks associated with that FID?
>
> Is that the main concern, or am I missing the point here?
Two FIDS will have the same lease and when lease break comes they have
to push their locks to the server. To make this process faster we can
send a chunk of locks in one request, as we already do for cifs
mandatory locks. If we have all FID's lock in one place we don't need
to tun over the whole inode lock list to create groups when oplock
break comes.
The only difference between CIFS and SMB2 is that in SMB2 lock_element
structure in lock request doesn't contain pid element (that exists for
CIFS), assuming that all locks from the request associated with pid
from the header. So to handle this right we need further to replace
cifsFileInfo lock list (introduced by this patch) with "pid_lock" list
struct pid_lock {
struct list_head lock_list;
pid_t pid;
}
that group FID's locks by pid.
>
> > ---
> > fs/cifs/cifsfs.c | 1 -
> > fs/cifs/cifsglob.h | 3 +-
> > fs/cifs/file.c | 98 ++++++++++++++++++++++++++++++++--------------------
> > 3 files changed, 61 insertions(+), 41 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index eee522c..3ac61a7 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -939,7 +939,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 d5ccd46..4e095ae 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,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 held by this fid */
>
> It would be nice to comment that the above list is intended to be
> protected by the lock_mutex (right?)
>
> > unsigned int uid; /* allows finding which FileInfo structure */
> > __u32 pid; /* process id who opened file */
> > __u16 netfid; /* file id from remote */
> > @@ -613,7 +613,6 @@ 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 */
> > /* BB add in lists for dirty pages i.e. write caching info for oplock */
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 2bf04e9..cc54033 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);
> > @@ -687,19 +685,19 @@ cifs_locks_delete_block(struct file_lock *waiter)
> > }
> >
> > 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;
> >
> > - list_for_each_entry_safe(li, tmp, &cinode->llist, llist) {
> > + list_for_each_entry_safe(li, tmp, &cfile->llist, llist) {
>
> list_for_each_entry should be fine here since you're not doing
> gets/puts on these entries as you go.
>
> > 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;
> > @@ -710,11 +708,36 @@ __cifs_find_lock_conflict(struct cifsInodeInfo *cinode, \
> > __u64 offset, }
> >
> > static bool
> > +__cifs_find_lock_conflict(struct cifsInodeInfo *cinode, __u64 offset,
> > + __u64 length, __u8 type, __u16 netfid,
> > + struct cifsLockInfo **conf_lock)
> > +{
> > + bool rc;
> > + struct cifsFileInfo *fid, *tmp;
> > +
> > + spin_lock(&cifs_file_list_lock);
> > + list_for_each_entry_safe(fid, tmp, &cinode->openFileList, flist) {
> > + cifsFileInfo_get(fid);
> > + spin_unlock(&cifs_file_list_lock);
>
> NAK. list_for_each_entry_safe only protects you from removing the
> current entry of the list by preloading the next entry into "tmp". It's
> possible (even likely) that while this code is running, another thread
> could do the final cifsFileInfo_put on the entry that has already been
> preloaded into "tmp" and cause an oops or worse.
Yes, makes sense. Thanks!
>
> > + rc = __cifs_find_fid_lock_conflict(fid, offset, length, type,
> > + netfid, conf_lock);
> > + cifsFileInfo_put(fid);
> > + if (rc)
> > + return rc;
> > + 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,
> > - struct cifsLockInfo **conf_lock)
> > + __u16 netfid, struct cifsLockInfo **conf_lock)
>
> Do we really need this wrapper? There's only one caller of
> cifs_find_lock_conflict, AFAICT.
This makes a caller code smarter, I think.
>
> > {
> > +
> > return __cifs_find_lock_conflict(cinode, lock->offset, lock->length,
> > - lock->type, lock->netfid, conf_lock);
> > + lock->type, netfid, conf_lock);
> > }
> >
> > /*
> > @@ -725,17 +748,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;
> > @@ -754,10 +778,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);
> > }
> >
> > @@ -768,10 +793,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;
> >
> > @@ -779,9 +805,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, 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;
> > }
> > @@ -928,7 +955,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) {
> > if (li->type != types[i])
> > continue;
> > cur->Pid = cpu_to_le16(li->pid);
> > @@ -1144,7 +1171,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) {
> > @@ -1164,8 +1190,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;
> >
> > @@ -1252,15 +1277,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) {
> > @@ -1273,7 +1296,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);
> > @@ -1287,10 +1310,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
> > /*
> > @@ -1305,7 +1328,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);
> > @@ -1316,7 +1339,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);
> > @@ -1336,7 +1359,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) {
> > @@ -1363,11 +1385,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)
> > @@ -1380,7 +1402,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);
> >
>
>
> --
> Jeff Layton <jlayton@redhat.com>
--
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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic