On Fri, 2006-03-31 at 13:27 -0500, Trond Myklebust wrote: > On Fri, 2006-03-31 at 19:30 +0200, Miklos Szeredi wrote: > > posix_lock_file() was too cautious, failing operations on OOM, even if > > they didn't actually require an allocation. > > > > This has the disadvantage, that a failing unlock on process exit could > > lead to a memory leak. There are two possibilites for this: > > > > - filesystem implements .lock() and calls back to posix_lock_file(). > > On cleanup of files_struct locks_remove_posix() is called which should > > remove all locks belonging to files_struct. However if filesystem > > calls posix_lock_file() which fails, then those locks will never be > > freed. > > > > - if a file is closed while a lock is blocked, then after acquiring > > fcntl_setlk() will undo the lock. But this unlock itself might fail > > on OOM, again possibly leaking the lock. > > > > The solution is to move the checking of the allocations until after it > > is sure that they will be needed. This will solve the above problem > > since unlock will always succeed unless it splits an existing region. > > > > Signed-off-by: Miklos Szeredi <miklos@xxxxxxxxxx> > > > > Index: linux/fs/locks.c > > =================================================================== > > --- linux.orig/fs/locks.c 2006-03-31 18:55:33.000000000 +0200 > > +++ linux/fs/locks.c 2006-03-31 18:55:33.000000000 +0200 > > @@ -835,14 +835,7 @@ int __posix_lock_file(struct inode *inod > > if (request->fl_flags & FL_ACCESS) > > goto out; > > > > - error = -ENOLCK; /* "no luck" */ > > - if (!(new_fl && new_fl2)) > > - goto out; > > - > > /* > > - * We've allocated the new locks in advance, so there are no > > - * errors possible (and no blocking operations) from here on. > > - * > > * Find the first old lock with the same owner as the new lock. > > */ > > > > @@ -939,6 +932,18 @@ int __posix_lock_file(struct inode *inod > > before = &fl->fl_next; > > } > > > > + /* > > + * The above code only modifies existing locks in case of > > + * merging or replacing. If new lock(s) need to be inserted > > + * all modifications are done bellow this, so it's safe yet to > > + * bail out. > > + */ > > + error = -ENOLCK; /* "no luck" */ > > + if (!added && request->fl_type != F_UNLCK && !new_fl) > > + goto out; > > + if (right && left == right && !new_fl2) > > + goto out; > > + > > error = 0; > > if (!added) { > > if (request->fl_type == F_UNLCK) > > NACK. > > This changes the behaviour of F_UNLCK. Currently, if the allocation > fails, the inode locking state remains unchanged. With your change, an > unlock request may end up unlocking part of the inode, but not the rest. > > Furthermore, AFAICS you are now able to Oops if (!added && !new_fl). Sorry: the Oops turned out to be a mirage. However you are also changing the behaviour of F_SETLK for the case where the user is trying to up/downgrade a set of existing READ/WRITE locks. Again you may end up with a situation where some of the existing locks get up/downgraded, and yet the lock request fails. Cheers, Trond - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html