2011/11/1 Jeff Layton <jlayton@xxxxxxxxx>:
> On Mon, 31 Oct 2011 14:04:00 +0300
> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>
>> 2011/10/31 Pavel Shilovsky <piastry@xxxxxxxxxxx>:
>> > 2011/10/31 Jeff Layton <jlayton@xxxxxxxxxxxxxxx>:
>> >> On Sat, 29 Oct 2011 17:17:58 +0400
>> >> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>> >>
>> >>> Now we allocate a lock structure at first, then we request to the server
>> >>> and save the lock if server returned OK though void function - it prevents
>> >>> the situation when we locked a file on the server and then return -ENOMEM
>> >>> from setlk.
>> >>>
>> >>> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
>> >>> ---
>> >>> fs/cifs/file.c | 64 ++++++++++++++++++++++++++++----------------------------
>> >>> 1 files changed, 32 insertions(+), 32 deletions(-)
>> >>>
>> >>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> >>> index c1f063c..d9cc07f 100644
>> >>> --- a/fs/cifs/file.c
>> >>> +++ b/fs/cifs/file.c
>> >>> @@ -672,7 +672,7 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock)
>> >>> }
>> >>>
>> >>> static bool
>> >>> -cifs_find_lock_conflict(struct cifsInodeInfo *cinode, __u64 offset,
>> >>> +__cifs_find_lock_conflict(struct cifsInodeInfo *cinode, __u64 offset,
>> >>> __u64 length, __u8 type, __u16 netfid,
>> >>> struct cifsLockInfo **conf_lock)
>> >>> {
>> >>> @@ -694,6 +694,14 @@ cifs_find_lock_conflict(struct cifsInodeInfo *cinode, __u64 offset,
>> >>> return false;
>> >>> }
>> >>>
>> >>> +static bool
>> >>> +cifs_find_lock_conflict(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock,
>> >>> + struct cifsLockInfo **conf_lock)
>> >>> +{
>> >>> + return __cifs_find_lock_conflict(cinode, lock->offset, lock->length,
>> >>> + lock->type, lock->netfid, conf_lock);
>> >>> +}
>> >>> +
>> >>> static int
>> >>> cifs_lock_test(struct cifsInodeInfo *cinode, __u64 offset, __u64 length,
>> >>> __u8 type, __u16 netfid, struct file_lock *flock)
>> >>> @@ -704,8 +712,8 @@ cifs_lock_test(struct cifsInodeInfo *cinode, __u64 offset, __u64 length,
>> >>>
>> >>> 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, netfid,
>> >>> + &conf_lock);
>> >>> if (exist) {
>> >>> flock->fl_start = conf_lock->offset;
>> >>> flock->fl_end = conf_lock->offset + conf_lock->length - 1;
>> >>> @@ -723,40 +731,27 @@ cifs_lock_test(struct cifsInodeInfo *cinode, __u64 offset, __u64 length,
>> >>> return rc;
>> >>> }
>> >>>
>> >>> -static int
>> >>> -cifs_lock_add(struct cifsInodeInfo *cinode, __u64 len, __u64 offset,
>> >>> - __u8 type, __u16 netfid)
>> >>> +static void
>> >>> +cifs_lock_add(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock)
>> >>> {
>> >>> - struct cifsLockInfo *li;
>> >>> -
>> >>> - li = cifs_lock_init(len, offset, type, netfid);
>> >>> - if (!li)
>> >>> - return -ENOMEM;
>> >>> -
>> >>> mutex_lock(&cinode->lock_mutex);
>> >>> - list_add_tail(&li->llist, &cinode->llist);
>> >>> + list_add_tail(&lock->llist, &cinode->llist);
>> >>> mutex_unlock(&cinode->lock_mutex);
>> >>> - return 0;
>> >>> }
>> >>>
>> >>> static int
>> >>> -cifs_lock_add_if(struct cifsInodeInfo *cinode, __u64 offset, __u64 length,
>> >>> - __u8 type, __u16 netfid, bool wait)
>> >>> +cifs_lock_add_if(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock,
>> >>> + bool wait)
>> >>> {
>> >>> - struct cifsLockInfo *lock, *conf_lock;
>> >>> + struct cifsLockInfo *conf_lock;
>> >>> bool exist;
>> >>> int rc = 0;
>> >>>
>> >>> - lock = cifs_lock_init(length, offset, type, netfid);
>> >>> - if (!lock)
>> >>> - return -ENOMEM;
>> >>> -
>> >>> try_again:
>> >>> exist = false;
>> >>> mutex_lock(&cinode->lock_mutex);
>> >>>
>> >>> - exist = cifs_find_lock_conflict(cinode, offset, length, type, netfid,
>> >>> - &conf_lock);
>> >>> + exist = cifs_find_lock_conflict(cinode, lock, &conf_lock);
>> >>> if (!exist && cinode->can_cache_brlcks) {
>> >>> list_add_tail(&lock->llist, &cinode->llist);
>> >>> mutex_unlock(&cinode->lock_mutex);
>> >>> @@ -781,7 +776,6 @@ try_again:
>> >>> }
>> >>> }
>> >>>
>> >>> - kfree(lock);
>> >>> mutex_unlock(&cinode->lock_mutex);
>> >>> return rc;
>> >>> }
>> >>> @@ -1254,20 +1248,26 @@ cifs_setlk(struct file *file, struct file_lock *flock, __u8 type,
>> >>> }
>> >>>
>> >>> if (lock) {
>> >>> - rc = cifs_lock_add_if(cinode, flock->fl_start, length,
>> >>> - type, netfid, wait_flag);
>> >>> + struct cifsLockInfo *lock;
>> >>> +
>> >>> + lock = cifs_lock_init(length, flock->fl_start, type, netfid);
>> >>> + if (!lock)
>> >>> + return -ENOMEM;
>> >>> +
>> >>> + rc = cifs_lock_add_if(cinode, lock, wait_flag);
>> >>
>> >> Here, you're adding "lock" to the list...
>> >
>> > If we added the lock to the list cifs_lock_add_if returns 0 and we
>> > will jump to out label.
>> >
>> >>
>> >>> if (rc < 0)
>> >>> - return rc;
>> >>> - else if (!rc)
>> >>> + kfree(lock);
>> >>> + if (rc <= 0)
>> >>> goto out;
>> >>>
>> >>> rc = CIFSSMBLock(xid, tcon, netfid, current->tgid, length,
>> >>> flock->fl_start, 0, 1, type, wait_flag, 0);
>> >>> - if (rc == 0) {
>> >>> - /* For Windows locks we must store them. */
>> >>> - rc = cifs_lock_add(cinode, length, flock->fl_start,
>> >>> - type, netfid);
>> >>> + if (rc) {
>> >>> + kfree(lock);
>> >>
>> >> ...and here you're freeing "lock" without removing it from the list.
>> >> Isn't that like to cause a problem?
>> >
>> > So, if CIFSSMBLock returns a error we free the lock that hasn't been
>> > added to the list. If CIFSSMBLock returns ok, we will add it to the
>> > list with cifs_lock_add void function.
>> >
>> > Seems no problem with it.
>> >
>> > --
>> > Best regards,
>> > Pavel Shilovsky.
>> >
>>
>> So, to be clear - brlock cache function cifs_lock_add_if returns:
>>
>> "0" if we store the lock in the cache and have an oplock for batching
>> brlocks - no need to request to the server.
>>
>> "1" if there is no locks preventing us to set this lock but we haven't
>> an oplock for batching brlocks - need to request to the server and
>> then add the lock to the list if server accepts.
>>
>> "-EACCESS" if we have a lock on the client that prevent us and this
>> call is non-blocked.
>>
>
> Ok, it looks like this patch is probably ok then, but it would be nice
> to have the above explanation enshrined in a comment over
> cifs_lock_add_if to ensure that other people working in this code
> understand it.
>
> Can you spin up a patch to add that when you get time? For this patch
> though...
Your are right - I will do it when I get time.
>
>
> Acked-by: Jeff Layton <jlayton@xxxxxxxxx>
Thank you for the review.
--
Best regards,
Pavel Shilovsky.
--
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
