On Sat, 3 Mar 2012 06:42:50 -0500
Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Sat, 3 Mar 2012 10:50:28 +0300
> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>
> > Reorganize the code to make the memory already allocated before
> > spinlock'ed loop.
> >
> > Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
> > ---
> > fs/cifs/file.c | 65 ++++++++++++++++++++++++++++++++++++++++++++-----------
> > 1 files changed, 52 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 4dd9283..c90f7f6 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -920,16 +920,26 @@ cifs_push_mandatory_locks(struct cifsFileInfo *cfile)
> > for (lockp = &inode->i_flock; *lockp != NULL; \
> > lockp = &(*lockp)->fl_next)
> >
> > +struct lock_to_push {
> > + struct list_head llist;
> > + __u64 offset;
> > + __u64 length;
> > + __u32 pid;
> > + __u16 netfid;
> > + __u8 type;
> > +};
> > +
> > static int
> > cifs_push_posix_locks(struct cifsFileInfo *cfile)
> > {
> > struct cifsInodeInfo *cinode = CIFS_I(cfile->dentry->d_inode);
> > struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > struct file_lock *flock, **before;
> > - struct cifsLockInfo *lck, *tmp;
> > + unsigned int count = 0, i = 0;
> > int rc = 0, xid, type;
> > + struct list_head locks_to_send, *el;
> > + struct lock_to_push *lck, *tmp;
> > __u64 length;
> > - struct list_head locks_to_send;
> >
> > xid = GetXid();
> >
> > @@ -940,29 +950,51 @@ cifs_push_posix_locks(struct cifsFileInfo *cfile)
> > return rc;
> > }
> >
> > + lock_flocks();
> > + cifs_for_each_lock(cfile->dentry->d_inode, before) {
> > + if ((*before)->fl_flags & FL_POSIX)
> > + count++;
> > + }
> > + unlock_flocks();
> > +
> > INIT_LIST_HEAD(&locks_to_send);
> >
> > + /* We can safely allocate count locks with holding lock_mutex. */
> > + for (; i < count; i++) {
> > + lck = kmalloc(sizeof(struct lock_to_push), GFP_KERNEL);
> > + if (!lck) {
> > + rc = -ENOMEM;
> > + goto err_out;
> > + }
> > + list_add_tail(&lck->llist, &locks_to_send);
> > + }
> > +
> > + i = 0;
> > + el = locks_to_send.next;
> > lock_flocks();
> > cifs_for_each_lock(cfile->dentry->d_inode, before) {
> > + if (el == &locks_to_send) {
> > + /* something is really wrong */
> > + cERROR(1, "Can't push all brlocks!");
> > + break;
> > + }
> > flock = *before;
> > + if ((flock->fl_flags & FL_POSIX) == 0)
> > + continue;
> > length = 1 + flock->fl_end - flock->fl_start;
> > if (flock->fl_type == F_RDLCK || flock->fl_type == F_SHLCK)
> > type = CIFS_RDLCK;
> > else
> > type = CIFS_WRLCK;
> > -
> > - lck = cifs_lock_init(flock->fl_start, length, type,
> > - cfile->netfid);
> > - if (!lck) {
> > - rc = -ENOMEM;
> > - goto send_locks;
> > - }
> > + lck = list_entry(el, struct lock_to_push, llist);
> > lck->pid = flock->fl_pid;
> > -
> > - list_add_tail(&lck->llist, &locks_to_send);
> > + lck->netfid = cfile->netfid;
> > + lck->length = length;
> > + lck->type = type;
> > + lck->offset = flock->fl_start;
> > + i++;
> > + el = el->next;
> > }
> > -
> > -send_locks:
> > unlock_flocks();
> >
> > list_for_each_entry_safe(lck, tmp, &locks_to_send, llist) {
> > @@ -979,11 +1011,18 @@ send_locks:
> > kfree(lck);
> > }
> >
> > +out:
> > cinode->can_cache_brlcks = false;
> > mutex_unlock(&cinode->lock_mutex);
> >
> > FreeXid(xid);
> > return rc;
> > +err_out:
> > + list_for_each_entry_safe(lck, tmp, &locks_to_send, llist) {
> > + list_del(&lck->llist);
> > + kfree(lck);
> > + }
> > + goto out;
> > }
> >
> > static int
>
> Looks fine. I would however like to see some comments in this function
> that explain why the list of locks won't change after you
> unlock_flocks(). It's fairly clear now, but in a year or two we won't
> remember. That can be done in a later patch though.
>
> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
Oh, and by the way -- this probably ought to go to stable as well. It
would be nice to add those comments there too.
Pavel, could you respin this patch and add that comment, my Reviewed-by
and Cc: stable@xxxxxxxxxxxxxxx ?
Thanks,
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
