* [PATCH] locks: Ability to test for flock presence on fd @ 2014-09-02 17:17 Pavel Emelyanov 2014-09-02 18:44 ` J. Bruce Fields 0 siblings, 1 reply; 14+ messages in thread From: Pavel Emelyanov @ 2014-09-02 17:17 UTC (permalink / raw) To: Jeff Layton, J. Bruce Fields, Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api Hi, There's a problem with getting information about who has a flock on a specific file. The thing is that the "owner" field, that is shown in /proc/locks is the pid of the task who created the flock, not the one who _may_ hold it. If the flock creator shared the file with some other task (by forking or via scm_rights) and then died or closed the file, the information shown in proc no longer corresponds to the reality. This is critical for CRIU project, that tries to dump (and restore) the state of running tasks. For example, let's take two tasks A and B both opened a file "/foo", one of tasks places a LOCK_SH lock on the file and then "obfuscated" the owner field in /proc/locks. After this we have no ways to find out who is the lock holder. I'd like to note, that for LOCK_EX this problem is not critical -- we may go to both tasks and "ask" them to LOCK_EX the file again (we can do it in CRIU, I can tell more if required). The one who succeeds is the lock holder. With LOCK_SH this doesn't work. Trying to drop the lock doesn't work either, as flock(LOCK_UN) reports 0 in both cases: if the file is locked and if it is not. That said, I'd like to propose the LOCK_TEST flag to the flock call, that would check whether the lock of the given type (LOCK_SH or LOCK_EX) exists on the file we test. It's not the same as the existing in-kernel FL_ACCESS flag, which checks whether the new lock is possible, but it's a new FL_TEST flag, that checks whether the existing lock is there. What do you think? Signed-off-by: Pavel Emelyanov <xemul@parallels.com> --- diff --git a/fs/locks.c b/fs/locks.c index bb08857..50842bf 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -830,7 +830,7 @@ static int flock_lock_file(struct file *filp, struct file_lock *request) int found = 0; LIST_HEAD(dispose); - if (!(request->fl_flags & FL_ACCESS) && (request->fl_type != F_UNLCK)) { + if (!(request->fl_flags & (FL_ACCESS|FL_TEST)) && (request->fl_type != F_UNLCK)) { new_fl = locks_alloc_lock(); if (!new_fl) return -ENOMEM; @@ -850,11 +850,18 @@ static int flock_lock_file(struct file *filp, struct file_lock *request) continue; if (request->fl_type == fl->fl_type) goto out; + if (request->fl_flags & FL_TEST) + break; found = 1; locks_delete_lock(before, &dispose); break; } + if (request->fl_flags & FL_TEST) { + error = -ENOENT; + goto out; + } + if (request->fl_type == F_UNLCK) { if ((request->fl_flags & FL_EXISTS) && !found) error = -ENOENT; @@ -1852,15 +1859,16 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) { struct fd f = fdget(fd); struct file_lock *lock; - int can_sleep, unlock; + int can_sleep, unlock, test; int error; error = -EBADF; if (!f.file) goto out; + test = (cmd & LOCK_TEST); can_sleep = !(cmd & LOCK_NB); - cmd &= ~LOCK_NB; + cmd &= ~(LOCK_NB|LOCK_TEST); unlock = (cmd == LOCK_UN); if (!unlock && !(cmd & LOCK_MAND) && @@ -1872,6 +1880,8 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) goto out_putf; if (can_sleep) lock->fl_flags |= FL_SLEEP; + if (test) + lock->fl_flags |= FL_TEST; error = security_file_lock(f.file, lock->fl_type); if (error) diff --git a/include/linux/fs.h b/include/linux/fs.h index 9418772..9230e1d 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -844,6 +844,7 @@ static inline struct file *get_file(struct file *f) #define FL_DOWNGRADE_PENDING 256 /* Lease is being downgraded */ #define FL_UNLOCK_PENDING 512 /* Lease is being broken */ #define FL_OFDLCK 1024 /* lock is "owned" by struct file */ +#define FL_TEST 2048 /* * Special return value from posix_lock_file() and vfs_lock_file() for diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h index 7543b3e..7302e36 100644 --- a/include/uapi/asm-generic/fcntl.h +++ b/include/uapi/asm-generic/fcntl.h @@ -184,6 +184,7 @@ struct f_owner_ex { #define LOCK_READ 64 /* which allows concurrent read operations */ #define LOCK_WRITE 128 /* which allows concurrent write operations */ #define LOCK_RW 192 /* which allows concurrent read & write ops */ +#define LOCK_TEST 256 /* check for my SH|EX locks present */ #define F_LINUX_SPECIFIC_BASE 1024 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-02 17:17 [PATCH] locks: Ability to test for flock presence on fd Pavel Emelyanov @ 2014-09-02 18:44 ` J. Bruce Fields 2014-09-02 19:07 ` Pavel Emelyanov 0 siblings, 1 reply; 14+ messages in thread From: J. Bruce Fields @ 2014-09-02 18:44 UTC (permalink / raw) To: Pavel Emelyanov Cc: Jeff Layton, Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api On Tue, Sep 02, 2014 at 09:17:34PM +0400, Pavel Emelyanov wrote: > Hi, > > There's a problem with getting information about who has a flock on > a specific file. The thing is that the "owner" field, that is shown in > /proc/locks is the pid of the task who created the flock, not the one > who _may_ hold it. > > If the flock creator shared the file with some other task (by forking > or via scm_rights) and then died or closed the file, the information > shown in proc no longer corresponds to the reality. > > This is critical for CRIU project, that tries to dump (and restore) > the state of running tasks. For example, let's take two tasks A and B > both opened a file "/foo", one of tasks places a LOCK_SH lock on the > file and then "obfuscated" the owner field in /proc/locks. After this > we have no ways to find out who is the lock holder. > > I'd like to note, that for LOCK_EX this problem is not critical -- we > may go to both tasks and "ask" them to LOCK_EX the file again (we can > do it in CRIU, I can tell more if required). The one who succeeds is > the lock holder. It could be both, actually, right? > With LOCK_SH this doesn't work. Trying to drop the > lock doesn't work either, as flock(LOCK_UN) reports 0 in both cases: > if the file is locked and if it is not. > > That said, I'd like to propose the LOCK_TEST flag to the flock call, > that would check whether the lock of the given type (LOCK_SH or LOCK_EX) > exists on the file we test. It's not the same as the existing in-kernel > FL_ACCESS flag, which checks whether the new lock is possible, but > it's a new FL_TEST flag, that checks whether the existing lock is there. > > What do you think? I guess I can't see anything really wrong with it. It ignores the (poorly documented) LOCK_MAND case, but maybe that's OK. Would it make sense to return the lock type held instead, so you could do one flock(fd, LOCK_TEST) instead of flock(fd, LOCK_TEST|LOCK_SH) and flock(fd, LOCK_TEST|LOCK_EX) ? It'd be nice if we could fix /proc/locks. (You'd think I'd know better, but I've certainly been confused before when /proc/locks told me a lock was owned by a nonexistant PID.) But as long as multiple PIDs can "own" a flock and as long as there's no simple ID we can use to refer to a given struct file, I don't see an easy solution. --b. > > Signed-off-by: Pavel Emelyanov <xemul@parallels.com> > > --- > > diff --git a/fs/locks.c b/fs/locks.c > index bb08857..50842bf 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -830,7 +830,7 @@ static int flock_lock_file(struct file *filp, struct file_lock *request) > int found = 0; > LIST_HEAD(dispose); > > - if (!(request->fl_flags & FL_ACCESS) && (request->fl_type != F_UNLCK)) { > + if (!(request->fl_flags & (FL_ACCESS|FL_TEST)) && (request->fl_type != F_UNLCK)) { > new_fl = locks_alloc_lock(); > if (!new_fl) > return -ENOMEM; > @@ -850,11 +850,18 @@ static int flock_lock_file(struct file *filp, struct file_lock *request) > continue; > if (request->fl_type == fl->fl_type) > goto out; > + if (request->fl_flags & FL_TEST) > + break; > found = 1; > locks_delete_lock(before, &dispose); > break; > } > > + if (request->fl_flags & FL_TEST) { > + error = -ENOENT; > + goto out; > + } > + > if (request->fl_type == F_UNLCK) { > if ((request->fl_flags & FL_EXISTS) && !found) > error = -ENOENT; > @@ -1852,15 +1859,16 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) > { > struct fd f = fdget(fd); > struct file_lock *lock; > - int can_sleep, unlock; > + int can_sleep, unlock, test; > int error; > > error = -EBADF; > if (!f.file) > goto out; > > + test = (cmd & LOCK_TEST); > can_sleep = !(cmd & LOCK_NB); > - cmd &= ~LOCK_NB; > + cmd &= ~(LOCK_NB|LOCK_TEST); > unlock = (cmd == LOCK_UN); > > if (!unlock && !(cmd & LOCK_MAND) && > @@ -1872,6 +1880,8 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) > goto out_putf; > if (can_sleep) > lock->fl_flags |= FL_SLEEP; > + if (test) > + lock->fl_flags |= FL_TEST; > > error = security_file_lock(f.file, lock->fl_type); > if (error) > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 9418772..9230e1d 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -844,6 +844,7 @@ static inline struct file *get_file(struct file *f) > #define FL_DOWNGRADE_PENDING 256 /* Lease is being downgraded */ > #define FL_UNLOCK_PENDING 512 /* Lease is being broken */ > #define FL_OFDLCK 1024 /* lock is "owned" by struct file */ > +#define FL_TEST 2048 > > /* > * Special return value from posix_lock_file() and vfs_lock_file() for > diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h > index 7543b3e..7302e36 100644 > --- a/include/uapi/asm-generic/fcntl.h > +++ b/include/uapi/asm-generic/fcntl.h > @@ -184,6 +184,7 @@ struct f_owner_ex { > #define LOCK_READ 64 /* which allows concurrent read operations */ > #define LOCK_WRITE 128 /* which allows concurrent write operations */ > #define LOCK_RW 192 /* which allows concurrent read & write ops */ > +#define LOCK_TEST 256 /* check for my SH|EX locks present */ > > #define F_LINUX_SPECIFIC_BASE 1024 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-02 18:44 ` J. Bruce Fields @ 2014-09-02 19:07 ` Pavel Emelyanov 2014-09-02 19:43 ` J. Bruce Fields 2014-09-09 16:18 ` J. Bruce Fields 0 siblings, 2 replies; 14+ messages in thread From: Pavel Emelyanov @ 2014-09-02 19:07 UTC (permalink / raw) To: J. Bruce Fields Cc: Jeff Layton, Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api On 09/02/2014 10:44 PM, J. Bruce Fields wrote: > On Tue, Sep 02, 2014 at 09:17:34PM +0400, Pavel Emelyanov wrote: >> Hi, >> >> There's a problem with getting information about who has a flock on >> a specific file. The thing is that the "owner" field, that is shown in >> /proc/locks is the pid of the task who created the flock, not the one >> who _may_ hold it. >> >> If the flock creator shared the file with some other task (by forking >> or via scm_rights) and then died or closed the file, the information >> shown in proc no longer corresponds to the reality. >> >> This is critical for CRIU project, that tries to dump (and restore) >> the state of running tasks. For example, let's take two tasks A and B >> both opened a file "/foo", one of tasks places a LOCK_SH lock on the >> file and then "obfuscated" the owner field in /proc/locks. After this >> we have no ways to find out who is the lock holder. >> >> I'd like to note, that for LOCK_EX this problem is not critical -- we >> may go to both tasks and "ask" them to LOCK_EX the file again (we can >> do it in CRIU, I can tell more if required). The one who succeeds is >> the lock holder. > > It could be both, actually, right? Two succeeding with LOCK_EX? AFAIU no. Am I missing something? >> With LOCK_SH this doesn't work. Trying to drop the >> lock doesn't work either, as flock(LOCK_UN) reports 0 in both cases: >> if the file is locked and if it is not. >> >> That said, I'd like to propose the LOCK_TEST flag to the flock call, >> that would check whether the lock of the given type (LOCK_SH or LOCK_EX) >> exists on the file we test. It's not the same as the existing in-kernel >> FL_ACCESS flag, which checks whether the new lock is possible, but >> it's a new FL_TEST flag, that checks whether the existing lock is there. >> >> What do you think? > > I guess I can't see anything really wrong with it. > > It ignores the (poorly documented) LOCK_MAND case, but maybe that's OK. I actually checked it and it seemed to work. What problems do you see with this case? > Would it make sense to return the lock type held instead, so you could > do one flock(fd, LOCK_TEST) instead of flock(fd, LOCK_TEST|LOCK_SH) and > flock(fd, LOCK_TEST|LOCK_EX) ? Well, in our case we parse /proc/locks anyway to see what files at least to test for being locked. But what you propose looks even better. I'll look what can be done here. > It'd be nice if we could fix /proc/locks. (You'd think I'd know better, > but I've certainly been confused before when /proc/locks told me a lock > was owned by a nonexistant PID.) But as long as multiple PIDs can "own" > a flock and as long as there's no simple ID we can use to refer to a > given struct file, I don't see an easy solution. > > --b. > > >> >> Signed-off-by: Pavel Emelyanov <xemul@parallels.com> >> >> --- >> >> diff --git a/fs/locks.c b/fs/locks.c >> index bb08857..50842bf 100644 >> --- a/fs/locks.c >> +++ b/fs/locks.c >> @@ -830,7 +830,7 @@ static int flock_lock_file(struct file *filp, struct file_lock *request) >> int found = 0; >> LIST_HEAD(dispose); >> >> - if (!(request->fl_flags & FL_ACCESS) && (request->fl_type != F_UNLCK)) { >> + if (!(request->fl_flags & (FL_ACCESS|FL_TEST)) && (request->fl_type != F_UNLCK)) { >> new_fl = locks_alloc_lock(); >> if (!new_fl) >> return -ENOMEM; >> @@ -850,11 +850,18 @@ static int flock_lock_file(struct file *filp, struct file_lock *request) >> continue; >> if (request->fl_type == fl->fl_type) >> goto out; >> + if (request->fl_flags & FL_TEST) >> + break; >> found = 1; >> locks_delete_lock(before, &dispose); >> break; >> } >> >> + if (request->fl_flags & FL_TEST) { >> + error = -ENOENT; >> + goto out; >> + } >> + >> if (request->fl_type == F_UNLCK) { >> if ((request->fl_flags & FL_EXISTS) && !found) >> error = -ENOENT; >> @@ -1852,15 +1859,16 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) >> { >> struct fd f = fdget(fd); >> struct file_lock *lock; >> - int can_sleep, unlock; >> + int can_sleep, unlock, test; >> int error; >> >> error = -EBADF; >> if (!f.file) >> goto out; >> >> + test = (cmd & LOCK_TEST); >> can_sleep = !(cmd & LOCK_NB); >> - cmd &= ~LOCK_NB; >> + cmd &= ~(LOCK_NB|LOCK_TEST); >> unlock = (cmd == LOCK_UN); >> >> if (!unlock && !(cmd & LOCK_MAND) && >> @@ -1872,6 +1880,8 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) >> goto out_putf; >> if (can_sleep) >> lock->fl_flags |= FL_SLEEP; >> + if (test) >> + lock->fl_flags |= FL_TEST; >> >> error = security_file_lock(f.file, lock->fl_type); >> if (error) >> diff --git a/include/linux/fs.h b/include/linux/fs.h >> index 9418772..9230e1d 100644 >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -844,6 +844,7 @@ static inline struct file *get_file(struct file *f) >> #define FL_DOWNGRADE_PENDING 256 /* Lease is being downgraded */ >> #define FL_UNLOCK_PENDING 512 /* Lease is being broken */ >> #define FL_OFDLCK 1024 /* lock is "owned" by struct file */ >> +#define FL_TEST 2048 >> >> /* >> * Special return value from posix_lock_file() and vfs_lock_file() for >> diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h >> index 7543b3e..7302e36 100644 >> --- a/include/uapi/asm-generic/fcntl.h >> +++ b/include/uapi/asm-generic/fcntl.h >> @@ -184,6 +184,7 @@ struct f_owner_ex { >> #define LOCK_READ 64 /* which allows concurrent read operations */ >> #define LOCK_WRITE 128 /* which allows concurrent write operations */ >> #define LOCK_RW 192 /* which allows concurrent read & write ops */ >> +#define LOCK_TEST 256 /* check for my SH|EX locks present */ >> >> #define F_LINUX_SPECIFIC_BASE 1024 >> >> > . > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-02 19:07 ` Pavel Emelyanov @ 2014-09-02 19:43 ` J. Bruce Fields 2014-09-02 19:53 ` Jeff Layton 2014-09-09 16:18 ` J. Bruce Fields 1 sibling, 1 reply; 14+ messages in thread From: J. Bruce Fields @ 2014-09-02 19:43 UTC (permalink / raw) To: Pavel Emelyanov Cc: Jeff Layton, Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api On Tue, Sep 02, 2014 at 11:07:14PM +0400, Pavel Emelyanov wrote: > On 09/02/2014 10:44 PM, J. Bruce Fields wrote: > > On Tue, Sep 02, 2014 at 09:17:34PM +0400, Pavel Emelyanov wrote: > >> Hi, > >> > >> There's a problem with getting information about who has a flock on > >> a specific file. The thing is that the "owner" field, that is shown in > >> /proc/locks is the pid of the task who created the flock, not the one > >> who _may_ hold it. > >> > >> If the flock creator shared the file with some other task (by forking > >> or via scm_rights) and then died or closed the file, the information > >> shown in proc no longer corresponds to the reality. > >> > >> This is critical for CRIU project, that tries to dump (and restore) > >> the state of running tasks. For example, let's take two tasks A and B > >> both opened a file "/foo", one of tasks places a LOCK_SH lock on the > >> file and then "obfuscated" the owner field in /proc/locks. After this > >> we have no ways to find out who is the lock holder. > >> > >> I'd like to note, that for LOCK_EX this problem is not critical -- we > >> may go to both tasks and "ask" them to LOCK_EX the file again (we can > >> do it in CRIU, I can tell more if required). The one who succeeds is > >> the lock holder. > > > > It could be both, actually, right? > > Two succeeding with LOCK_EX? AFAIU no. Am I missing something? After a fork, two processes "own" the lock, right?: int main(int argc, char *argv[]) { int fd, ret; fd = open(argv[1], O_RDWR); ret = flock(fd, LOCK_EX); if (ret) err(1, "flock"); ret = fork(); if (ret == -1) err(1, "flock"); ret = flock(fd, LOCK_EX); if (ret) err(1, "flock"); printf("%d got exclusive lock\n", getpid()); sleep(1000); } $ touch TMP $ ./test TMP 15882 got exclusive lock 15883 got exclusive lock ^C I may misunderstand what you're doing. > >> With LOCK_SH this doesn't work. Trying to drop the > >> lock doesn't work either, as flock(LOCK_UN) reports 0 in both cases: > >> if the file is locked and if it is not. > >> > >> That said, I'd like to propose the LOCK_TEST flag to the flock call, > >> that would check whether the lock of the given type (LOCK_SH or LOCK_EX) > >> exists on the file we test. It's not the same as the existing in-kernel > >> FL_ACCESS flag, which checks whether the new lock is possible, but > >> it's a new FL_TEST flag, that checks whether the existing lock is there. > >> > >> What do you think? > > > > I guess I can't see anything really wrong with it. > > > > It ignores the (poorly documented) LOCK_MAND case, but maybe that's OK. > > I actually checked it and it seemed to work. What problems do > you see with this case? On its own it just doesn't tell you whether or not LOCK_MAND is set. But I guess you can still get that out of /proc/locks. To be honest I don't really know whether LOCK_MAND works or is used. --b. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-02 19:43 ` J. Bruce Fields @ 2014-09-02 19:53 ` Jeff Layton 2014-09-03 14:38 ` Pavel Emelyanov 0 siblings, 1 reply; 14+ messages in thread From: Jeff Layton @ 2014-09-02 19:53 UTC (permalink / raw) To: J. Bruce Fields Cc: Pavel Emelyanov, Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api On Tue, 2 Sep 2014 15:43:00 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Tue, Sep 02, 2014 at 11:07:14PM +0400, Pavel Emelyanov wrote: > > On 09/02/2014 10:44 PM, J. Bruce Fields wrote: > > > On Tue, Sep 02, 2014 at 09:17:34PM +0400, Pavel Emelyanov wrote: > > >> Hi, > > >> > > >> There's a problem with getting information about who has a flock on > > >> a specific file. The thing is that the "owner" field, that is shown in > > >> /proc/locks is the pid of the task who created the flock, not the one > > >> who _may_ hold it. > > >> > > >> If the flock creator shared the file with some other task (by forking > > >> or via scm_rights) and then died or closed the file, the information > > >> shown in proc no longer corresponds to the reality. > > >> > > >> This is critical for CRIU project, that tries to dump (and restore) > > >> the state of running tasks. For example, let's take two tasks A and B > > >> both opened a file "/foo", one of tasks places a LOCK_SH lock on the > > >> file and then "obfuscated" the owner field in /proc/locks. After this > > >> we have no ways to find out who is the lock holder. > > >> > > >> I'd like to note, that for LOCK_EX this problem is not critical -- we > > >> may go to both tasks and "ask" them to LOCK_EX the file again (we can > > >> do it in CRIU, I can tell more if required). The one who succeeds is > > >> the lock holder. > > > > > > It could be both, actually, right? > > > > Two succeeding with LOCK_EX? AFAIU no. Am I missing something? > > After a fork, two processes "own" the lock, right?: > > int main(int argc, char *argv[]) > { > int fd, ret; > > fd = open(argv[1], O_RDWR); > ret = flock(fd, LOCK_EX); > if (ret) > err(1, "flock"); > ret = fork(); > if (ret == -1) > err(1, "flock"); > ret = flock(fd, LOCK_EX); > if (ret) > err(1, "flock"); > printf("%d got exclusive lock\n", getpid()); > sleep(1000); > } > > $ touch TMP > $ ./test TMP > 15882 got exclusive lock > 15883 got exclusive lock > ^C > > I may misunderstand what you're doing. > Yeah, I don't understand either. Flock locks are owned by the file description. The task that set them is really irrelevant once they are set. In the second flock() call there, you're just "modifying" an existing lock (which turns out to be a noop here). So, I don't quite understand the problem this solves. I get that you're trying to reestablish the flock "state" after a checkpoint/restore event, but why does it matter what task actually sets the locks as long as they're set on the correct set of fds? > > >> With LOCK_SH this doesn't work. Trying to drop the > > >> lock doesn't work either, as flock(LOCK_UN) reports 0 in both cases: > > >> if the file is locked and if it is not. > > >> > > >> That said, I'd like to propose the LOCK_TEST flag to the flock call, > > >> that would check whether the lock of the given type (LOCK_SH or LOCK_EX) > > >> exists on the file we test. It's not the same as the existing in-kernel > > >> FL_ACCESS flag, which checks whether the new lock is possible, but > > >> it's a new FL_TEST flag, that checks whether the existing lock is there. > > >> > > >> What do you think? > > > > > > I guess I can't see anything really wrong with it. > > > > > > It ignores the (poorly documented) LOCK_MAND case, but maybe that's OK. > > > > I actually checked it and it seemed to work. What problems do > > you see with this case? > > On its own it just doesn't tell you whether or not LOCK_MAND is set. > But I guess you can still get that out of /proc/locks. > > To be honest I don't really know whether LOCK_MAND works or is used. > > --b. -- Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-02 19:53 ` Jeff Layton @ 2014-09-03 14:38 ` Pavel Emelyanov 2014-09-03 15:44 ` J. Bruce Fields 2014-09-03 15:55 ` Jeff Layton 0 siblings, 2 replies; 14+ messages in thread From: Pavel Emelyanov @ 2014-09-03 14:38 UTC (permalink / raw) To: Jeff Layton, J. Bruce Fields Cc: Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api On 09/02/2014 11:53 PM, Jeff Layton wrote: > On Tue, 2 Sep 2014 15:43:00 -0400 > "J. Bruce Fields" <bfields@fieldses.org> wrote: > >> On Tue, Sep 02, 2014 at 11:07:14PM +0400, Pavel Emelyanov wrote: >>> On 09/02/2014 10:44 PM, J. Bruce Fields wrote: >>>> On Tue, Sep 02, 2014 at 09:17:34PM +0400, Pavel Emelyanov wrote: >>>>> Hi, >>>>> >>>>> There's a problem with getting information about who has a flock on >>>>> a specific file. The thing is that the "owner" field, that is shown in >>>>> /proc/locks is the pid of the task who created the flock, not the one >>>>> who _may_ hold it. >>>>> >>>>> If the flock creator shared the file with some other task (by forking >>>>> or via scm_rights) and then died or closed the file, the information >>>>> shown in proc no longer corresponds to the reality. >>>>> >>>>> This is critical for CRIU project, that tries to dump (and restore) >>>>> the state of running tasks. For example, let's take two tasks A and B >>>>> both opened a file "/foo", one of tasks places a LOCK_SH lock on the >>>>> file and then "obfuscated" the owner field in /proc/locks. After this >>>>> we have no ways to find out who is the lock holder. >>>>> >>>>> I'd like to note, that for LOCK_EX this problem is not critical -- we >>>>> may go to both tasks and "ask" them to LOCK_EX the file again (we can >>>>> do it in CRIU, I can tell more if required). The one who succeeds is >>>>> the lock holder. >>>> >>>> It could be both, actually, right? >>> >>> Two succeeding with LOCK_EX? AFAIU no. Am I missing something? >> >> After a fork, two processes "own" the lock, right?: >> >> int main(int argc, char *argv[]) >> { >> int fd, ret; >> >> fd = open(argv[1], O_RDWR); >> ret = flock(fd, LOCK_EX); >> if (ret) >> err(1, "flock"); >> ret = fork(); >> if (ret == -1) >> err(1, "flock"); >> ret = flock(fd, LOCK_EX); >> if (ret) >> err(1, "flock"); >> printf("%d got exclusive lock\n", getpid()); >> sleep(1000); >> } >> >> $ touch TMP >> $ ./test TMP >> 15882 got exclusive lock >> 15883 got exclusive lock >> ^C >> >> I may misunderstand what you're doing. >> > > Yeah, I don't understand either. > > Flock locks are owned by the file description. The task that set > them is really irrelevant once they are set. > > In the second flock() call there, you're just "modifying" an existing > lock (which turns out to be a noop here). > > So, I don't quite understand the problem this solves. I get that you're > trying to reestablish the flock "state" after a checkpoint/restore > event, but why does it matter what task actually sets the locks as long > as they're set on the correct set of fds? Sorry for confusion. Let me try to explain it more clearly. First, what I meant talking about two LOCK_EX locks. Let's consider this scenario: pid = fork() fd = open("/foo"); /* both parent and child has _different_ files */ if (pid == 0) /* child only */ flock(fd, LOCK_EX); at this point we have two different files pointing to "/foo" and only one of them has LOCK_EX on it. So if try to LOCK_EX it again, only at child's file this would succeed. So we can distinguish which file is locked using this method. Now, what problem this patch is trying to solve. It's quite tricky, but still. Let's imagine this scenario: pid = fork(); fd = open("/foo"); /* yet again -- two different files */ if (pid == 0) { flock(fd, LOCK_SH); pid2 = fork(); if (pid2 != 0) exit(0); } at this point we have: task A -- the original task with file "/foo" opened task B -- the first child, that exited at the end task C -- the 2nd child, that "inherited" a file with the lock from B Note, that file at A and file at C are two different files (struct file-s). And it's only the C's one that is locked. The problem is that the /proc/locks shows the pid of B in this lock's owner field. And we have no glue to find out who the real lock owner is using the /proc/locks. If we try to do the trickery like the one we did with LOCK_EX above, this is what we would get. If putting the 2nd LOCK_SH from A and from C, both attempts would succeed, so this is not the solution. If we try to LOCK_EX from A and C, only C would succeed, so this seem to be the solution, but it's actually not. If there's another pair of A' and C' tasks holding the same "/foo" and having the LOCK_SH on C', this trick would stop working as none of the tasks would be able to put such lock on this file. Thus, we need some way to find out whether a task X has a lock on file F. This patch is one of the ways of doing this. Hope this explanation is more clear. >>>>> With LOCK_SH this doesn't work. Trying to drop the >>>>> lock doesn't work either, as flock(LOCK_UN) reports 0 in both cases: >>>>> if the file is locked and if it is not. >>>>> >>>>> That said, I'd like to propose the LOCK_TEST flag to the flock call, >>>>> that would check whether the lock of the given type (LOCK_SH or LOCK_EX) >>>>> exists on the file we test. It's not the same as the existing in-kernel >>>>> FL_ACCESS flag, which checks whether the new lock is possible, but >>>>> it's a new FL_TEST flag, that checks whether the existing lock is there. >>>>> >>>>> What do you think? >>>> >>>> I guess I can't see anything really wrong with it. >>>> >>>> It ignores the (poorly documented) LOCK_MAND case, but maybe that's OK. >>> >>> I actually checked it and it seemed to work. What problems do >>> you see with this case? >> >> On its own it just doesn't tell you whether or not LOCK_MAND is set. >> But I guess you can still get that out of /proc/locks. >> >> To be honest I don't really know whether LOCK_MAND works or is used. >> >> --b. > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-03 14:38 ` Pavel Emelyanov @ 2014-09-03 15:44 ` J. Bruce Fields 2014-09-03 15:47 ` Pavel Emelyanov 2014-09-03 15:55 ` Jeff Layton 1 sibling, 1 reply; 14+ messages in thread From: J. Bruce Fields @ 2014-09-03 15:44 UTC (permalink / raw) To: Pavel Emelyanov Cc: Jeff Layton, Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api On Wed, Sep 03, 2014 at 06:38:24PM +0400, Pavel Emelyanov wrote: > On 09/02/2014 11:53 PM, Jeff Layton wrote: > > On Tue, 2 Sep 2014 15:43:00 -0400 > > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > >> On Tue, Sep 02, 2014 at 11:07:14PM +0400, Pavel Emelyanov wrote: > >>> On 09/02/2014 10:44 PM, J. Bruce Fields wrote: > >>>> On Tue, Sep 02, 2014 at 09:17:34PM +0400, Pavel Emelyanov wrote: > >>>>> Hi, > >>>>> > >>>>> There's a problem with getting information about who has a flock on > >>>>> a specific file. The thing is that the "owner" field, that is shown in > >>>>> /proc/locks is the pid of the task who created the flock, not the one > >>>>> who _may_ hold it. > >>>>> > >>>>> If the flock creator shared the file with some other task (by forking > >>>>> or via scm_rights) and then died or closed the file, the information > >>>>> shown in proc no longer corresponds to the reality. > >>>>> > >>>>> This is critical for CRIU project, that tries to dump (and restore) > >>>>> the state of running tasks. For example, let's take two tasks A and B > >>>>> both opened a file "/foo", one of tasks places a LOCK_SH lock on the > >>>>> file and then "obfuscated" the owner field in /proc/locks. After this > >>>>> we have no ways to find out who is the lock holder. > >>>>> > >>>>> I'd like to note, that for LOCK_EX this problem is not critical -- we > >>>>> may go to both tasks and "ask" them to LOCK_EX the file again (we can > >>>>> do it in CRIU, I can tell more if required). The one who succeeds is > >>>>> the lock holder. > >>>> > >>>> It could be both, actually, right? > >>> > >>> Two succeeding with LOCK_EX? AFAIU no. Am I missing something? > >> > >> After a fork, two processes "own" the lock, right?: > >> > >> int main(int argc, char *argv[]) > >> { > >> int fd, ret; > >> > >> fd = open(argv[1], O_RDWR); > >> ret = flock(fd, LOCK_EX); > >> if (ret) > >> err(1, "flock"); > >> ret = fork(); > >> if (ret == -1) > >> err(1, "flock"); > >> ret = flock(fd, LOCK_EX); > >> if (ret) > >> err(1, "flock"); > >> printf("%d got exclusive lock\n", getpid()); > >> sleep(1000); > >> } > >> > >> $ touch TMP > >> $ ./test TMP > >> 15882 got exclusive lock > >> 15883 got exclusive lock > >> ^C > >> > >> I may misunderstand what you're doing. > >> > > > > Yeah, I don't understand either. > > > > Flock locks are owned by the file description. The task that set > > them is really irrelevant once they are set. > > > > In the second flock() call there, you're just "modifying" an existing > > lock (which turns out to be a noop here). > > > > So, I don't quite understand the problem this solves. I get that you're > > trying to reestablish the flock "state" after a checkpoint/restore > > event, but why does it matter what task actually sets the locks as long > > as they're set on the correct set of fds? > > Sorry for confusion. Let me try to explain it more clearly. > > First, what I meant talking about two LOCK_EX locks. Let's consider > this scenario: > > pid = fork() > fd = open("/foo"); /* both parent and child has _different_ files */ > if (pid == 0) > /* child only */ > flock(fd, LOCK_EX); > > at this point we have two different files pointing to "/foo" and > only one of them has LOCK_EX on it. So if try to LOCK_EX it again, > only at child's file this would succeed. So we can distinguish which > file is locked using this method. > > > > Now, what problem this patch is trying to solve. It's quite tricky, > but still. Let's imagine this scenario: > > pid = fork(); > fd = open("/foo"); /* yet again -- two different files */ > if (pid == 0) { > flock(fd, LOCK_SH); > pid2 = fork(); > if (pid2 != 0) > exit(0); > } > > at this point we have: > > task A -- the original task with file "/foo" opened > task B -- the first child, that exited at the end > task C -- the 2nd child, that "inherited" a file with the lock from B > > Note, that file at A and file at C are two different files (struct > file-s). And it's only the C's one that is locked. > > The problem is that the /proc/locks shows the pid of B in this lock's > owner field. And we have no glue to find out who the real lock owner > is using the /proc/locks. > > If we try to do the trickery like the one we did with LOCK_EX above, > this is what we would get. > > If putting the 2nd LOCK_SH from A and from C, both attempts would succeed, > so this is not the solution. > > If we try to LOCK_EX from A and C, only C would succeed, so this seem > to be the solution, but it's actually not. If there's another pair of > A' and C' tasks holding the same "/foo" and having the LOCK_SH on C', > this trick would stop working as none of the tasks would be able to > put such lock on this file. > > > Thus, we need some way to find out whether a task X has a lock on file F. > This patch is one of the ways of doing this. > > Hope this explanation is more clear. Thanks, I think I understand. Remind me how you figure out which file descriptors point to the same file description (struct file)? --b. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-03 15:44 ` J. Bruce Fields @ 2014-09-03 15:47 ` Pavel Emelyanov 0 siblings, 0 replies; 14+ messages in thread From: Pavel Emelyanov @ 2014-09-03 15:47 UTC (permalink / raw) To: J. Bruce Fields Cc: Jeff Layton, Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api On 09/03/2014 07:44 PM, J. Bruce Fields wrote: ... >> Hope this explanation is more clear. > > Thanks, I think I understand. > > Remind me how you figure out which file descriptors point to the same > file description (struct file)? We do it with the help of kcmp() syscall: https://github.com/cyrillos/linux-2.6/commit/951bc60c6a Thanks, Pavel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-03 14:38 ` Pavel Emelyanov 2014-09-03 15:44 ` J. Bruce Fields @ 2014-09-03 15:55 ` Jeff Layton 2014-09-03 16:00 ` Pavel Emelyanov 1 sibling, 1 reply; 14+ messages in thread From: Jeff Layton @ 2014-09-03 15:55 UTC (permalink / raw) To: Pavel Emelyanov Cc: J. Bruce Fields, Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api On Wed, 3 Sep 2014 18:38:24 +0400 Pavel Emelyanov <xemul@parallels.com> wrote: > On 09/02/2014 11:53 PM, Jeff Layton wrote: > > On Tue, 2 Sep 2014 15:43:00 -0400 > > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > >> On Tue, Sep 02, 2014 at 11:07:14PM +0400, Pavel Emelyanov wrote: > >>> On 09/02/2014 10:44 PM, J. Bruce Fields wrote: > >>>> On Tue, Sep 02, 2014 at 09:17:34PM +0400, Pavel Emelyanov wrote: > >>>>> Hi, > >>>>> > >>>>> There's a problem with getting information about who has a flock on > >>>>> a specific file. The thing is that the "owner" field, that is shown in > >>>>> /proc/locks is the pid of the task who created the flock, not the one > >>>>> who _may_ hold it. > >>>>> > >>>>> If the flock creator shared the file with some other task (by forking > >>>>> or via scm_rights) and then died or closed the file, the information > >>>>> shown in proc no longer corresponds to the reality. > >>>>> > >>>>> This is critical for CRIU project, that tries to dump (and restore) > >>>>> the state of running tasks. For example, let's take two tasks A and B > >>>>> both opened a file "/foo", one of tasks places a LOCK_SH lock on the > >>>>> file and then "obfuscated" the owner field in /proc/locks. After this > >>>>> we have no ways to find out who is the lock holder. > >>>>> > >>>>> I'd like to note, that for LOCK_EX this problem is not critical -- we > >>>>> may go to both tasks and "ask" them to LOCK_EX the file again (we can > >>>>> do it in CRIU, I can tell more if required). The one who succeeds is > >>>>> the lock holder. > >>>> > >>>> It could be both, actually, right? > >>> > >>> Two succeeding with LOCK_EX? AFAIU no. Am I missing something? > >> > >> After a fork, two processes "own" the lock, right?: > >> > >> int main(int argc, char *argv[]) > >> { > >> int fd, ret; > >> > >> fd = open(argv[1], O_RDWR); > >> ret = flock(fd, LOCK_EX); > >> if (ret) > >> err(1, "flock"); > >> ret = fork(); > >> if (ret == -1) > >> err(1, "flock"); > >> ret = flock(fd, LOCK_EX); > >> if (ret) > >> err(1, "flock"); > >> printf("%d got exclusive lock\n", getpid()); > >> sleep(1000); > >> } > >> > >> $ touch TMP > >> $ ./test TMP > >> 15882 got exclusive lock > >> 15883 got exclusive lock > >> ^C > >> > >> I may misunderstand what you're doing. > >> > > > > Yeah, I don't understand either. > > > > Flock locks are owned by the file description. The task that set > > them is really irrelevant once they are set. > > > > In the second flock() call there, you're just "modifying" an existing > > lock (which turns out to be a noop here). > > > > So, I don't quite understand the problem this solves. I get that you're > > trying to reestablish the flock "state" after a checkpoint/restore > > event, but why does it matter what task actually sets the locks as long > > as they're set on the correct set of fds? > > Sorry for confusion. Let me try to explain it more clearly. > > First, what I meant talking about two LOCK_EX locks. Let's consider > this scenario: > > pid = fork() > fd = open("/foo"); /* both parent and child has _different_ files */ > if (pid == 0) > /* child only */ > flock(fd, LOCK_EX); > > at this point we have two different files pointing to "/foo" and > only one of them has LOCK_EX on it. So if try to LOCK_EX it again, > only at child's file this would succeed. So we can distinguish which > file is locked using this method. > > > > Now, what problem this patch is trying to solve. It's quite tricky, > but still. Let's imagine this scenario: > > pid = fork(); > fd = open("/foo"); /* yet again -- two different files */ > if (pid == 0) { > flock(fd, LOCK_SH); > pid2 = fork(); > if (pid2 != 0) > exit(0); > } > > at this point we have: > > task A -- the original task with file "/foo" opened > task B -- the first child, that exited at the end > task C -- the 2nd child, that "inherited" a file with the lock from B > > Note, that file at A and file at C are two different files (struct > file-s). And it's only the C's one that is locked. > > The problem is that the /proc/locks shows the pid of B in this lock's > owner field. And we have no glue to find out who the real lock owner > is using the /proc/locks. > > If we try to do the trickery like the one we did with LOCK_EX above, > this is what we would get. > > If putting the 2nd LOCK_SH from A and from C, both attempts would succeed, > so this is not the solution. > > If we try to LOCK_EX from A and C, only C would succeed, so this seem > to be the solution, but it's actually not. If there's another pair of > A' and C' tasks holding the same "/foo" and having the LOCK_SH on C', > this trick would stop working as none of the tasks would be able to > put such lock on this file. > > > Thus, we need some way to find out whether a task X has a lock on file F. > This patch is one of the ways of doing this. > > Hope this explanation is more clear. > Yes, thanks for clarifying. I think we do need to be a bit careful when describing this though. flock locks are not owned by tasks, but by the file description. So you can't really tell whether task X has a lock on file F. Several tasks could have a reference to file F and none of them has any more "claim" to a lock on that file than another (at least from an API standpoint). What your patch really does is tell you whether that file description has a particular type of lock set on it. Like Bruce, I think this looks fairly reasonable. That said, I had to go through a bunch of API gyrations recently when getting the OFD lock patches merged. It would be good to accompany your kernel patch with glibc and manpage patches as well so we can make sure we have the design settled before merging anything. Sound OK? -- Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-03 15:55 ` Jeff Layton @ 2014-09-03 16:00 ` Pavel Emelyanov 2014-09-03 16:03 ` Jeff Layton 0 siblings, 1 reply; 14+ messages in thread From: Pavel Emelyanov @ 2014-09-03 16:00 UTC (permalink / raw) To: Jeff Layton Cc: J. Bruce Fields, Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api On 09/03/2014 07:55 PM, Jeff Layton wrote: > On Wed, 3 Sep 2014 18:38:24 +0400 > Pavel Emelyanov <xemul@parallels.com> wrote: > >> On 09/02/2014 11:53 PM, Jeff Layton wrote: >>> On Tue, 2 Sep 2014 15:43:00 -0400 >>> "J. Bruce Fields" <bfields@fieldses.org> wrote: >>> >>>> On Tue, Sep 02, 2014 at 11:07:14PM +0400, Pavel Emelyanov wrote: >>>>> On 09/02/2014 10:44 PM, J. Bruce Fields wrote: >>>>>> On Tue, Sep 02, 2014 at 09:17:34PM +0400, Pavel Emelyanov wrote: >>>>>>> Hi, >>>>>>> >>>>>>> There's a problem with getting information about who has a flock on >>>>>>> a specific file. The thing is that the "owner" field, that is shown in >>>>>>> /proc/locks is the pid of the task who created the flock, not the one >>>>>>> who _may_ hold it. >>>>>>> >>>>>>> If the flock creator shared the file with some other task (by forking >>>>>>> or via scm_rights) and then died or closed the file, the information >>>>>>> shown in proc no longer corresponds to the reality. >>>>>>> >>>>>>> This is critical for CRIU project, that tries to dump (and restore) >>>>>>> the state of running tasks. For example, let's take two tasks A and B >>>>>>> both opened a file "/foo", one of tasks places a LOCK_SH lock on the >>>>>>> file and then "obfuscated" the owner field in /proc/locks. After this >>>>>>> we have no ways to find out who is the lock holder. >>>>>>> >>>>>>> I'd like to note, that for LOCK_EX this problem is not critical -- we >>>>>>> may go to both tasks and "ask" them to LOCK_EX the file again (we can >>>>>>> do it in CRIU, I can tell more if required). The one who succeeds is >>>>>>> the lock holder. >>>>>> >>>>>> It could be both, actually, right? >>>>> >>>>> Two succeeding with LOCK_EX? AFAIU no. Am I missing something? >>>> >>>> After a fork, two processes "own" the lock, right?: >>>> >>>> int main(int argc, char *argv[]) >>>> { >>>> int fd, ret; >>>> >>>> fd = open(argv[1], O_RDWR); >>>> ret = flock(fd, LOCK_EX); >>>> if (ret) >>>> err(1, "flock"); >>>> ret = fork(); >>>> if (ret == -1) >>>> err(1, "flock"); >>>> ret = flock(fd, LOCK_EX); >>>> if (ret) >>>> err(1, "flock"); >>>> printf("%d got exclusive lock\n", getpid()); >>>> sleep(1000); >>>> } >>>> >>>> $ touch TMP >>>> $ ./test TMP >>>> 15882 got exclusive lock >>>> 15883 got exclusive lock >>>> ^C >>>> >>>> I may misunderstand what you're doing. >>>> >>> >>> Yeah, I don't understand either. >>> >>> Flock locks are owned by the file description. The task that set >>> them is really irrelevant once they are set. >>> >>> In the second flock() call there, you're just "modifying" an existing >>> lock (which turns out to be a noop here). >>> >>> So, I don't quite understand the problem this solves. I get that you're >>> trying to reestablish the flock "state" after a checkpoint/restore >>> event, but why does it matter what task actually sets the locks as long >>> as they're set on the correct set of fds? >> >> Sorry for confusion. Let me try to explain it more clearly. >> >> First, what I meant talking about two LOCK_EX locks. Let's consider >> this scenario: >> >> pid = fork() >> fd = open("/foo"); /* both parent and child has _different_ files */ >> if (pid == 0) >> /* child only */ >> flock(fd, LOCK_EX); >> >> at this point we have two different files pointing to "/foo" and >> only one of them has LOCK_EX on it. So if try to LOCK_EX it again, >> only at child's file this would succeed. So we can distinguish which >> file is locked using this method. >> >> >> >> Now, what problem this patch is trying to solve. It's quite tricky, >> but still. Let's imagine this scenario: >> >> pid = fork(); >> fd = open("/foo"); /* yet again -- two different files */ >> if (pid == 0) { >> flock(fd, LOCK_SH); >> pid2 = fork(); >> if (pid2 != 0) >> exit(0); >> } >> >> at this point we have: >> >> task A -- the original task with file "/foo" opened >> task B -- the first child, that exited at the end >> task C -- the 2nd child, that "inherited" a file with the lock from B >> >> Note, that file at A and file at C are two different files (struct >> file-s). And it's only the C's one that is locked. >> >> The problem is that the /proc/locks shows the pid of B in this lock's >> owner field. And we have no glue to find out who the real lock owner >> is using the /proc/locks. >> >> If we try to do the trickery like the one we did with LOCK_EX above, >> this is what we would get. >> >> If putting the 2nd LOCK_SH from A and from C, both attempts would succeed, >> so this is not the solution. >> >> If we try to LOCK_EX from A and C, only C would succeed, so this seem >> to be the solution, but it's actually not. If there's another pair of >> A' and C' tasks holding the same "/foo" and having the LOCK_SH on C', >> this trick would stop working as none of the tasks would be able to >> put such lock on this file. >> >> >> Thus, we need some way to find out whether a task X has a lock on file F. >> This patch is one of the ways of doing this. >> >> Hope this explanation is more clear. >> > > Yes, thanks for clarifying. > > I think we do need to be a bit careful when describing this though. > > flock locks are not owned by tasks, but by the file description. So you > can't really tell whether task X has a lock on file F. Several tasks > could have a reference to file F and none of them has any more "claim" > to a lock on that file than another (at least from an API standpoint). > > What your patch really does is tell you whether that file description > has a particular type of lock set on it. Exactly. > Like Bruce, I think this looks fairly reasonable. That said, I had to > go through a bunch of API gyrations recently when getting the OFD lock > patches merged. It would be good to accompany your kernel patch with > glibc and manpage patches as well so we can make sure we have the > design settled before merging anything. > > Sound OK? Sure! But I think glibc and man-pages people would first want the kernel part to get finished, as it's the part that mostly drives the API. Since the linux-api@ is in Cc for this patch, what else would you suggest me to do to keep the process moving? Thanks, Pavel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-03 16:00 ` Pavel Emelyanov @ 2014-09-03 16:03 ` Jeff Layton 2014-09-03 16:57 ` Andy Lutomirski 0 siblings, 1 reply; 14+ messages in thread From: Jeff Layton @ 2014-09-03 16:03 UTC (permalink / raw) To: Pavel Emelyanov Cc: J. Bruce Fields, Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api, Michael Kerrisk (man-pages) On Wed, 3 Sep 2014 20:00:02 +0400 Pavel Emelyanov <xemul@parallels.com> wrote: > On 09/03/2014 07:55 PM, Jeff Layton wrote: > > On Wed, 3 Sep 2014 18:38:24 +0400 > > Pavel Emelyanov <xemul@parallels.com> wrote: > > > >> On 09/02/2014 11:53 PM, Jeff Layton wrote: > >>> On Tue, 2 Sep 2014 15:43:00 -0400 > >>> "J. Bruce Fields" <bfields@fieldses.org> wrote: > >>> > >>>> On Tue, Sep 02, 2014 at 11:07:14PM +0400, Pavel Emelyanov wrote: > >>>>> On 09/02/2014 10:44 PM, J. Bruce Fields wrote: > >>>>>> On Tue, Sep 02, 2014 at 09:17:34PM +0400, Pavel Emelyanov wrote: > >>>>>>> Hi, > >>>>>>> > >>>>>>> There's a problem with getting information about who has a flock on > >>>>>>> a specific file. The thing is that the "owner" field, that is shown in > >>>>>>> /proc/locks is the pid of the task who created the flock, not the one > >>>>>>> who _may_ hold it. > >>>>>>> > >>>>>>> If the flock creator shared the file with some other task (by forking > >>>>>>> or via scm_rights) and then died or closed the file, the information > >>>>>>> shown in proc no longer corresponds to the reality. > >>>>>>> > >>>>>>> This is critical for CRIU project, that tries to dump (and restore) > >>>>>>> the state of running tasks. For example, let's take two tasks A and B > >>>>>>> both opened a file "/foo", one of tasks places a LOCK_SH lock on the > >>>>>>> file and then "obfuscated" the owner field in /proc/locks. After this > >>>>>>> we have no ways to find out who is the lock holder. > >>>>>>> > >>>>>>> I'd like to note, that for LOCK_EX this problem is not critical -- we > >>>>>>> may go to both tasks and "ask" them to LOCK_EX the file again (we can > >>>>>>> do it in CRIU, I can tell more if required). The one who succeeds is > >>>>>>> the lock holder. > >>>>>> > >>>>>> It could be both, actually, right? > >>>>> > >>>>> Two succeeding with LOCK_EX? AFAIU no. Am I missing something? > >>>> > >>>> After a fork, two processes "own" the lock, right?: > >>>> > >>>> int main(int argc, char *argv[]) > >>>> { > >>>> int fd, ret; > >>>> > >>>> fd = open(argv[1], O_RDWR); > >>>> ret = flock(fd, LOCK_EX); > >>>> if (ret) > >>>> err(1, "flock"); > >>>> ret = fork(); > >>>> if (ret == -1) > >>>> err(1, "flock"); > >>>> ret = flock(fd, LOCK_EX); > >>>> if (ret) > >>>> err(1, "flock"); > >>>> printf("%d got exclusive lock\n", getpid()); > >>>> sleep(1000); > >>>> } > >>>> > >>>> $ touch TMP > >>>> $ ./test TMP > >>>> 15882 got exclusive lock > >>>> 15883 got exclusive lock > >>>> ^C > >>>> > >>>> I may misunderstand what you're doing. > >>>> > >>> > >>> Yeah, I don't understand either. > >>> > >>> Flock locks are owned by the file description. The task that set > >>> them is really irrelevant once they are set. > >>> > >>> In the second flock() call there, you're just "modifying" an existing > >>> lock (which turns out to be a noop here). > >>> > >>> So, I don't quite understand the problem this solves. I get that you're > >>> trying to reestablish the flock "state" after a checkpoint/restore > >>> event, but why does it matter what task actually sets the locks as long > >>> as they're set on the correct set of fds? > >> > >> Sorry for confusion. Let me try to explain it more clearly. > >> > >> First, what I meant talking about two LOCK_EX locks. Let's consider > >> this scenario: > >> > >> pid = fork() > >> fd = open("/foo"); /* both parent and child has _different_ files */ > >> if (pid == 0) > >> /* child only */ > >> flock(fd, LOCK_EX); > >> > >> at this point we have two different files pointing to "/foo" and > >> only one of them has LOCK_EX on it. So if try to LOCK_EX it again, > >> only at child's file this would succeed. So we can distinguish which > >> file is locked using this method. > >> > >> > >> > >> Now, what problem this patch is trying to solve. It's quite tricky, > >> but still. Let's imagine this scenario: > >> > >> pid = fork(); > >> fd = open("/foo"); /* yet again -- two different files */ > >> if (pid == 0) { > >> flock(fd, LOCK_SH); > >> pid2 = fork(); > >> if (pid2 != 0) > >> exit(0); > >> } > >> > >> at this point we have: > >> > >> task A -- the original task with file "/foo" opened > >> task B -- the first child, that exited at the end > >> task C -- the 2nd child, that "inherited" a file with the lock from B > >> > >> Note, that file at A and file at C are two different files (struct > >> file-s). And it's only the C's one that is locked. > >> > >> The problem is that the /proc/locks shows the pid of B in this lock's > >> owner field. And we have no glue to find out who the real lock owner > >> is using the /proc/locks. > >> > >> If we try to do the trickery like the one we did with LOCK_EX above, > >> this is what we would get. > >> > >> If putting the 2nd LOCK_SH from A and from C, both attempts would succeed, > >> so this is not the solution. > >> > >> If we try to LOCK_EX from A and C, only C would succeed, so this seem > >> to be the solution, but it's actually not. If there's another pair of > >> A' and C' tasks holding the same "/foo" and having the LOCK_SH on C', > >> this trick would stop working as none of the tasks would be able to > >> put such lock on this file. > >> > >> > >> Thus, we need some way to find out whether a task X has a lock on file F. > >> This patch is one of the ways of doing this. > >> > >> Hope this explanation is more clear. > >> > > > > Yes, thanks for clarifying. > > > > I think we do need to be a bit careful when describing this though. > > > > flock locks are not owned by tasks, but by the file description. So you > > can't really tell whether task X has a lock on file F. Several tasks > > could have a reference to file F and none of them has any more "claim" > > to a lock on that file than another (at least from an API standpoint). > > > > What your patch really does is tell you whether that file description > > has a particular type of lock set on it. > > Exactly. > > > Like Bruce, I think this looks fairly reasonable. That said, I had to > > go through a bunch of API gyrations recently when getting the OFD lock > > patches merged. It would be good to accompany your kernel patch with > > glibc and manpage patches as well so we can make sure we have the > > design settled before merging anything. > > > > Sound OK? > > Sure! But I think glibc and man-pages people would first want the > kernel part to get finished, as it's the part that mostly drives the > API. Since the linux-api@ is in Cc for this patch, what else would > you suggest me to do to keep the process moving? > > Thanks, > Pavel > (cc'ing Michael Kerrisk, the manpages maintainer) Michael had good suggestions for me when I was doing the OFD lock work. I'd also consider cc'ing the glibc development list. Cheers, -- Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-03 16:03 ` Jeff Layton @ 2014-09-03 16:57 ` Andy Lutomirski 0 siblings, 0 replies; 14+ messages in thread From: Andy Lutomirski @ 2014-09-03 16:57 UTC (permalink / raw) To: Jeff Layton Cc: Michael Kerrisk, linux-kernel, Alexander Viro, Pavel Emelyanov, J. Bruce Fields, linux-fsdevel, Linux API On Sep 3, 2014 9:04 AM, "Jeff Layton" <jlayton@poochiereds.net> wrote: > > On Wed, 3 Sep 2014 20:00:02 +0400 > Pavel Emelyanov <xemul@parallels.com> wrote: > > > On 09/03/2014 07:55 PM, Jeff Layton wrote: > > > On Wed, 3 Sep 2014 18:38:24 +0400 > > > Pavel Emelyanov <xemul@parallels.com> wrote: > > > > > >> On 09/02/2014 11:53 PM, Jeff Layton wrote: > > >>> On Tue, 2 Sep 2014 15:43:00 -0400 > > >>> "J. Bruce Fields" <bfields@fieldses.org> wrote: > > >>> > > >>>> On Tue, Sep 02, 2014 at 11:07:14PM +0400, Pavel Emelyanov wrote: > > >>>>> On 09/02/2014 10:44 PM, J. Bruce Fields wrote: > > >>>>>> On Tue, Sep 02, 2014 at 09:17:34PM +0400, Pavel Emelyanov wrote: > > >>>>>>> Hi, > > >>>>>>> > > >>>>>>> There's a problem with getting information about who has a flock on > > >>>>>>> a specific file. The thing is that the "owner" field, that is shown in > > >>>>>>> /proc/locks is the pid of the task who created the flock, not the one > > >>>>>>> who _may_ hold it. > > >>>>>>> > > >>>>>>> If the flock creator shared the file with some other task (by forking > > >>>>>>> or via scm_rights) and then died or closed the file, the information > > >>>>>>> shown in proc no longer corresponds to the reality. > > >>>>>>> > > >>>>>>> This is critical for CRIU project, that tries to dump (and restore) > > >>>>>>> the state of running tasks. For example, let's take two tasks A and B > > >>>>>>> both opened a file "/foo", one of tasks places a LOCK_SH lock on the > > >>>>>>> file and then "obfuscated" the owner field in /proc/locks. After this > > >>>>>>> we have no ways to find out who is the lock holder. > > >>>>>>> > > >>>>>>> I'd like to note, that for LOCK_EX this problem is not critical -- we > > >>>>>>> may go to both tasks and "ask" them to LOCK_EX the file again (we can > > >>>>>>> do it in CRIU, I can tell more if required). The one who succeeds is > > >>>>>>> the lock holder. > > >>>>>> > > >>>>>> It could be both, actually, right? > > >>>>> > > >>>>> Two succeeding with LOCK_EX? AFAIU no. Am I missing something? > > >>>> > > >>>> After a fork, two processes "own" the lock, right?: > > >>>> > > >>>> int main(int argc, char *argv[]) > > >>>> { > > >>>> int fd, ret; > > >>>> > > >>>> fd = open(argv[1], O_RDWR); > > >>>> ret = flock(fd, LOCK_EX); > > >>>> if (ret) > > >>>> err(1, "flock"); > > >>>> ret = fork(); > > >>>> if (ret == -1) > > >>>> err(1, "flock"); > > >>>> ret = flock(fd, LOCK_EX); > > >>>> if (ret) > > >>>> err(1, "flock"); > > >>>> printf("%d got exclusive lock\n", getpid()); > > >>>> sleep(1000); > > >>>> } > > >>>> > > >>>> $ touch TMP > > >>>> $ ./test TMP > > >>>> 15882 got exclusive lock > > >>>> 15883 got exclusive lock > > >>>> ^C > > >>>> > > >>>> I may misunderstand what you're doing. > > >>>> > > >>> > > >>> Yeah, I don't understand either. > > >>> > > >>> Flock locks are owned by the file description. The task that set > > >>> them is really irrelevant once they are set. > > >>> > > >>> In the second flock() call there, you're just "modifying" an existing > > >>> lock (which turns out to be a noop here). > > >>> > > >>> So, I don't quite understand the problem this solves. I get that you're > > >>> trying to reestablish the flock "state" after a checkpoint/restore > > >>> event, but why does it matter what task actually sets the locks as long > > >>> as they're set on the correct set of fds? > > >> > > >> Sorry for confusion. Let me try to explain it more clearly. > > >> > > >> First, what I meant talking about two LOCK_EX locks. Let's consider > > >> this scenario: > > >> > > >> pid = fork() > > >> fd = open("/foo"); /* both parent and child has _different_ files */ > > >> if (pid == 0) > > >> /* child only */ > > >> flock(fd, LOCK_EX); > > >> > > >> at this point we have two different files pointing to "/foo" and > > >> only one of them has LOCK_EX on it. So if try to LOCK_EX it again, > > >> only at child's file this would succeed. So we can distinguish which > > >> file is locked using this method. > > >> > > >> > > >> > > >> Now, what problem this patch is trying to solve. It's quite tricky, > > >> but still. Let's imagine this scenario: > > >> > > >> pid = fork(); > > >> fd = open("/foo"); /* yet again -- two different files */ > > >> if (pid == 0) { > > >> flock(fd, LOCK_SH); > > >> pid2 = fork(); > > >> if (pid2 != 0) > > >> exit(0); > > >> } > > >> > > >> at this point we have: > > >> > > >> task A -- the original task with file "/foo" opened > > >> task B -- the first child, that exited at the end > > >> task C -- the 2nd child, that "inherited" a file with the lock from B > > >> > > >> Note, that file at A and file at C are two different files (struct > > >> file-s). And it's only the C's one that is locked. > > >> > > >> The problem is that the /proc/locks shows the pid of B in this lock's > > >> owner field. And we have no glue to find out who the real lock owner > > >> is using the /proc/locks. > > >> > > >> If we try to do the trickery like the one we did with LOCK_EX above, > > >> this is what we would get. > > >> > > >> If putting the 2nd LOCK_SH from A and from C, both attempts would succeed, > > >> so this is not the solution. > > >> > > >> If we try to LOCK_EX from A and C, only C would succeed, so this seem > > >> to be the solution, but it's actually not. If there's another pair of > > >> A' and C' tasks holding the same "/foo" and having the LOCK_SH on C', > > >> this trick would stop working as none of the tasks would be able to > > >> put such lock on this file. > > >> > > >> > > >> Thus, we need some way to find out whether a task X has a lock on file F. > > >> This patch is one of the ways of doing this. > > >> > > >> Hope this explanation is more clear. > > >> > > > > > > Yes, thanks for clarifying. > > > > > > I think we do need to be a bit careful when describing this though. > > > > > > flock locks are not owned by tasks, but by the file description. So you > > > can't really tell whether task X has a lock on file F. Several tasks > > > could have a reference to file F and none of them has any more "claim" > > > to a lock on that file than another (at least from an API standpoint). > > > > > > What your patch really does is tell you whether that file description > > > has a particular type of lock set on it. > > > > Exactly. > > > > > Like Bruce, I think this looks fairly reasonable. That said, I had to > > > go through a bunch of API gyrations recently when getting the OFD lock > > > patches merged. It would be good to accompany your kernel patch with > > > glibc and manpage patches as well so we can make sure we have the > > > design settled before merging anything. > > > > > > Sound OK? > > > > Sure! But I think glibc and man-pages people would first want the > > kernel part to get finished, as it's the part that mostly drives the > > API. Since the linux-api@ is in Cc for this patch, what else would > > you suggest me to do to keep the process moving? > > > > Thanks, > > Pavel > > > > (cc'ing Michael Kerrisk, the manpages maintainer) > > Michael had good suggestions for me when I was doing the OFD lock work. > I'd also consider cc'ing the glibc development list. I would suggest writing up a short description of the exact semantics of your proposal. That way reviewers can decide whether the semantics make sense and then check whether the code matches the description. This could take the form of text that would go in the appropriate manpage. --Andy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-02 19:07 ` Pavel Emelyanov 2014-09-02 19:43 ` J. Bruce Fields @ 2014-09-09 16:18 ` J. Bruce Fields 2014-09-10 13:32 ` Jeff Layton 1 sibling, 1 reply; 14+ messages in thread From: J. Bruce Fields @ 2014-09-09 16:18 UTC (permalink / raw) To: Pavel Emelyanov Cc: Jeff Layton, Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api On Tue, Sep 02, 2014 at 11:07:14PM +0400, Pavel Emelyanov wrote: > > Would it make sense to return the lock type held instead, so you could > > do one flock(fd, LOCK_TEST) instead of flock(fd, LOCK_TEST|LOCK_SH) and > > flock(fd, LOCK_TEST|LOCK_EX) ? > > Well, in our case we parse /proc/locks anyway to see what > files at least to test for being locked. But what you propose > looks even better. I'll look what can be done here. Actually I think I prefer your version. It seems cleaner to define LOCK_TEST as returning the same result as you'd get if you actually tried the lock, just without applying the lock. It avoids having a different return-value convention for this one command. It might avoid some ambiguity in cases where the flock might be denied for reasons other than a conflicting flock (e.g. on NFS where flocks and fcntl locks conflict). It's closer to what GETLK does in the fcntl case. --b. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] locks: Ability to test for flock presence on fd 2014-09-09 16:18 ` J. Bruce Fields @ 2014-09-10 13:32 ` Jeff Layton 0 siblings, 0 replies; 14+ messages in thread From: Jeff Layton @ 2014-09-10 13:32 UTC (permalink / raw) To: Pavel Emelyanov Cc: J. Bruce Fields, Alexander Viro, linux-fsdevel, Linux Kernel Mailing List, linux-api On Tue, 9 Sep 2014 12:18:21 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Tue, Sep 02, 2014 at 11:07:14PM +0400, Pavel Emelyanov wrote: > > > Would it make sense to return the lock type held instead, so you could > > > do one flock(fd, LOCK_TEST) instead of flock(fd, LOCK_TEST|LOCK_SH) and > > > flock(fd, LOCK_TEST|LOCK_EX) ? > > > > Well, in our case we parse /proc/locks anyway to see what > > files at least to test for being locked. But what you propose > > looks even better. I'll look what can be done here. > > Actually I think I prefer your version. It seems cleaner to define > LOCK_TEST as returning the same result as you'd get if you actually > tried the lock, just without applying the lock. It avoids having a > different return-value convention for this one command. It might avoid > some ambiguity in cases where the flock might be denied for reasons > other than a conflicting flock (e.g. on NFS where flocks and fcntl locks > conflict). It's closer to what GETLK does in the fcntl case. > Yeah, I think I agree here too. Best to keep the interface as simple as possible, and the principle of least surprise would dictate that the return value match how other flock() calls work. I would still like to see a proposed manpage update for it. For bonus points, writing a section on flock() for the glibc manual might help get that piece merged as well. It was my experience that getting the small header file #defines into glibc for OFD locks was much more difficult than the kernel piece. YMMV of course, but getting the glibc folks to buy into the idea ahead of time would be good if possible. -- Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-09-10 13:32 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-02 17:17 [PATCH] locks: Ability to test for flock presence on fd Pavel Emelyanov 2014-09-02 18:44 ` J. Bruce Fields 2014-09-02 19:07 ` Pavel Emelyanov 2014-09-02 19:43 ` J. Bruce Fields 2014-09-02 19:53 ` Jeff Layton 2014-09-03 14:38 ` Pavel Emelyanov 2014-09-03 15:44 ` J. Bruce Fields 2014-09-03 15:47 ` Pavel Emelyanov 2014-09-03 15:55 ` Jeff Layton 2014-09-03 16:00 ` Pavel Emelyanov 2014-09-03 16:03 ` Jeff Layton 2014-09-03 16:57 ` Andy Lutomirski 2014-09-09 16:18 ` J. Bruce Fields 2014-09-10 13:32 ` Jeff Layton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).