lkml.org 
[lkml]   [2010]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 1/2] locks: let the caller free file_lock on ->setlease failure
    On Wed, Nov 03, 2010 at 04:41:48PM -0400, J. Bruce Fields wrote:
    > On Sun, Oct 31, 2010 at 08:35:10AM -0400, Christoph Hellwig wrote:
    > > Index: linux-2.6/fs/locks.c
    > > ===================================================================
    > > --- linux-2.6.orig/fs/locks.c 2010-10-31 07:10:07.649004084 -0400
    > > +++ linux-2.6/fs/locks.c 2010-10-31 07:34:10.102255587 -0400
    > > @@ -1428,8 +1425,9 @@ int generic_setlease(struct file *filp,
    > > goto out;
    > >
    > > if (my_before != NULL) {
    > > - *flp = *my_before;
    > > error = lease->fl_lmops->fl_change(my_before, arg);
    > > + if (!error)
    > > + *flp = *my_before;
    >
    > Argh, missed this: we're leaking the passed-in lease in this case.

    We could do something like this.

    The irritating thing is that the only lease user I understand is the
    nfsd code, and it doesn't want this lease-merging behavior; the only
    reason that fl_change is there is so it can just turn this case into an
    error every time.

    And I have no idea what the requirements are of any other users: do
    leases behave like this on purpose, or was it just an arbitrary choice,
    and does anyone depend on it now?

    In the end maybe it would be better just to leave leases as they are and
    define a new lock type for nfsd.

    We'd probably have to do that eventually anyway, and it'd save me trying
    to guess what the lease semantics are supposed to be....

    --b.

    From f5c6d51ae638af213d8bda31504f4b2287b8a801 Mon Sep 17 00:00:00 2001
    From: J. Bruce Fields <bfields@redhat.com>
    Date: Wed, 3 Nov 2010 16:49:44 -0400
    Subject: [PATCH 1/2] locks: fix leak on merging leases

    We must also free the passed-in lease in the case it wasn't used because
    an existing lease was upgrade/downgraded or already existed.

    Note the nfsd caller doesn't care because it's fl_change callback
    returns an error in those cases.

    Signed-off-by: J. Bruce Fields <bfields@redhat.com>
    ---
    fs/locks.c | 7 +++++--
    1 files changed, 5 insertions(+), 2 deletions(-)

    diff --git a/fs/locks.c b/fs/locks.c
    index 65765cb..61c22f7 100644
    --- a/fs/locks.c
    +++ b/fs/locks.c
    @@ -1504,7 +1504,7 @@ static int do_fcntl_delete_lease(struct file *filp)

    static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
    {
    - struct file_lock *fl;
    + struct file_lock *fl, *ret;
    struct fasync_struct *new;
    struct inode *inode = filp->f_path.dentry->d_inode;
    int error;
    @@ -1518,6 +1518,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
    locks_free_lock(fl);
    return -ENOMEM;
    }
    + ret = fl;
    lock_flocks();
    error = __vfs_setlease(filp, arg, &fl);
    if (error) {
    @@ -1525,6 +1526,8 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
    locks_free_lock(fl);
    goto out_free_fasync;
    }
    + if (ret != fl)
    + locks_free_lock(fl);

    /*
    * fasync_insert_entry() returns the old entry if any.
    @@ -1532,7 +1535,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
    * inserted it into the fasync list. Clear new so that
    * we don't release it here.
    */
    - if (!fasync_insert_entry(fd, filp, &fl->fl_fasync, new))
    + if (!fasync_insert_entry(fd, filp, &ret->fl_fasync, new))
    new = NULL;

    if (error < 0) {
    --
    1.7.1


    \
     
     \ /
      Last update: 2010-11-04 02:43    [W:0.023 / U:0.452 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site