On Mon, 10 Mar 2014 10:37:00 -0700 Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote: > On Mon, Mar 10, 2014 at 10:31 AM, Jeffrey Layton <jlayton@xxxxxxxxxx> > wrote: > > On Mon, 10 Mar 2014 10:07:10 -0700 > > Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote: > > > >> On Mon, Mar 10, 2014 at 6:36 AM, Jeff Layton <jlayton@xxxxxxxxxx> > >> wrote: > >> > Allow locks_mandatory_area() to handle file-private locks > >> > correctly. If there is a file-private lock set on an open file > >> > and we're doing I/O via the same, then that should not cause > >> > anything to block. > >> > > >> > Handle this by first doing a non-blocking FL_ACCESS check for a > >> > file-private lock, and then fall back to checking for a classic > >> > POSIX lock (and possibly blocking). > >> > >> This is ugly, but it doesn't seem much worse than the existing > >> code. :) > >> > >> --Andy > > > > > > My sentiments exactly. The alternative would to add some mechanism > > to check for locks that are owned by more than one owner without > > dropping the i_lock in between. That's doable but it'd be pretty > > ugly too, and you'd still have the existing races to contend with. > > > > FWIW, now that I've looked at this code it seems possible to make > > mandatory locking more or less race-free by having reads and writes > > both implicitly set a special sort of lock on the file: > > > > - a read lock (for both the read and write cases) > > > > - one that wouldn't conflict with existing locks held by same > > process or with file-private locks set on the same fd > > > > - but...the same lock could not be merged with either of the above > > (and would therefore need a different sort of ownership model). > > > > Have rw_verify_area set such a blocking lock on the file and then > > remove it once the read or write was done. It doesn't sound too hard > > to do, but performance would likely suck. > > > > That said, I don't feel terribly motivated to fix mandatory locking > > aside from doing my best to avoid the problem that Trond pointed > > out. > > I think the current patch is fine. It'll hurt performance for -omand > users a little, but this is the least of their problems. Let someone > who actually needs the feature fix it :) > > --Andy In the interest of full disclosure: There is potentially a new race here but it'd be tricky to hit it. It's possible that you could have one thread doing a read or write, check the inode for file-private locks and then not find any. Then, just before doing the blocking check for classic locks, another thread sets a fp lock on the same open file. At that point, you could potentially deadlock if the removal of that lock ended up blocked on the thread finishing that read or write. That does require an especially pathological program *and* mandatory locking. Hmm...maybe I'll do a v2 of this patch that adds a comment to outline that race for posterity however... -- Jeff Layton <jlayton@xxxxxxxxxx> -- 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
