lkml.org 
[lkml]   [2011]   [Dec]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: file locking fix for 3.2
    On Sun, Dec 25, 2011 at 12:05:42AM +0000, Al Viro wrote:
    > On Sat, Dec 24, 2011 at 06:50:35PM -0500, J. Bruce Fields wrote:
    >
    > > Then you're returning -ENOMEM in a case when we really didn't need to do
    > > an allocation, but is that really a problem? It's a rare case, and
    > > opens can already fail with -ENOMEM for other reasons, and I'd rather
    > > not have the extra hair.
    >
    > I'm certainly OK with that variant; if the folks maintaining fs/locks.c

    I've been more-or-less assuming that's me, not that I've been doing much
    real maintenance to speak of.

    > are happy with it, I'd suggest going for it. Note that you don't need
    > to touch locks_conflict() call at all if you bail out early on allocation
    > failure and it's definitely simpler and cleaner that way.

    Yep.

    With no more -rc, and no chance to test anything myself till I'm back
    from the holidays, my preference would be for Linus to merge the
    already-posted one-liner. Then I can queue up the below for 3.3.

    --b.

    commit 72acf27f6c20573d555d6b4450a7a9d41c4c9d5a
    Author: J. Bruce Fields <bfields@redhat.com>
    Date: Sun Dec 25 10:51:37 2011 -0700

    locks: simplify allocation in break_lease

    The code bends over backwards to avoid returning -ENOMEM in cases where
    the allocation wasn't really necessary.

    But there's nothing really *wrong* with returning -ENOMEM in those
    cases: break_lease callers can already return -ENOMEM for other reasons.
    So let's not take so much trouble over a rare case, and keep the code
    simpler.

    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

    diff --git a/fs/locks.c b/fs/locks.c
    index 96a487a..0bd1745 100644
    --- a/fs/locks.c
    +++ b/fs/locks.c
    @@ -1205,6 +1205,8 @@ int __break_lease(struct inode *inode, unsigned int mode)
    int want_write = (mode & O_ACCMODE) != O_RDONLY;

    new_fl = lease_alloc(NULL, want_write ? F_WRLCK : F_RDLCK);
    + if (IS_ERR(new_fl))
    + return PTR_ERR(new_fl);

    lock_flocks();

    @@ -1214,19 +1216,13 @@ int __break_lease(struct inode *inode, unsigned int mode)
    if ((flock == NULL) || !IS_LEASE(flock))
    goto out;

    - if (flock->fl_type == F_RDLCK && !want_write)
    - goto out; /* no conflict */
    + if (!locks_conflict(flock, new_fl))
    + goto out;

    for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next)
    if (fl->fl_owner == current->files)
    i_have_this_lease = 1;

    - if (IS_ERR(new_fl) && !i_have_this_lease
    - && ((mode & O_NONBLOCK) == 0)) {
    - error = PTR_ERR(new_fl);
    - goto out;
    - }
    -
    break_time = 0;
    if (lease_break_time > 0) {
    break_time = jiffies + lease_break_time * HZ;

    \
     
     \ /
      Last update: 2011-12-25 19:23    [W:0.022 / U:63.416 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site