| Message ID | 20180608142712.32460-3-khorenko@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 08, 2018 at 05:27:12PM +0300, Konstantin Khorenko wrote: > Currently if we face a lock taken by a process invisible in the current > pidns we skip the lock completely, but this > > 1) makes the output not that nice > (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3 > pos: 4 > flags: 02100002 > mnt_id: 257 > lock: (root@vz7)/: > > 2) makes it more difficult to debug issues with leaked flocks > if you get error on lock, but don't see any locks in /proc/$id/fdinfo/$file 3) breaks the CRIU project. criu reads fdinfo to dump file locks. > > Let's show information about such locks again as previously, but > show zero in the owner pid field. > > After the patch: > =============== > (root@vz7)/:cat /proc/${PID_A2}/fdinfo/3 > pos: 4 > flags: 02100002 > mnt_id: 295 > lock: 1: FLOCK ADVISORY WRITE 0 b6:f8a61:529946 0 EOF > I think initially this was broken in this commit: Fixes: d67fd44f697d ("locks: Filter /proc/locks output on proc pid ns") Acked-by: Andrei Vagin <avagin@openvz.org> Thanks, Andrei > Fixes: 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks") > Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com> > --- > fs/locks.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/fs/locks.c b/fs/locks.c > index bfee5b7f2862..e533623e2e99 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -2633,12 +2633,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > > fl_pid = locks_translate_pid(fl, proc_pidns); > /* > - * If there isn't a fl_pid don't display who is waiting on > - * the lock if we are called from locks_show, or if we are > - * called from __show_fd_info - skip lock entirely > + * If lock owner is dead (and pid is freed) or not visible in current > + * pidns, zero is shown as a pid value. Check lock info from > + * init_pid_ns to get saved lock pid value. > */ > - if (fl_pid == 0) > - return; > > if (fl->fl_file != NULL) > inode = locks_inode(fl->fl_file); > -- > 2.15.1 >
On Fri, 2018-06-08 at 17:27 +0300, Konstantin Khorenko wrote: > Currently if we face a lock taken by a process invisible in the current > pidns we skip the lock completely, but this > > 1) makes the output not that nice > (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3 > pos: 4 > flags: 02100002 > mnt_id: 257 > lock: (root@vz7)/: > > 2) makes it more difficult to debug issues with leaked flocks > if you get error on lock, but don't see any locks in /proc/$id/fdinfo/$file > > Let's show information about such locks again as previously, but > show zero in the owner pid field. > > After the patch: > =============== > (root@vz7)/:cat /proc/${PID_A2}/fdinfo/3 > pos: 4 > flags: 02100002 > mnt_id: 295 > lock: 1: FLOCK ADVISORY WRITE 0 b6:f8a61:529946 0 EOF > > Fixes: 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks") > Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com> > --- > fs/locks.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/fs/locks.c b/fs/locks.c > index bfee5b7f2862..e533623e2e99 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -2633,12 +2633,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > > fl_pid = locks_translate_pid(fl, proc_pidns); > /* > - * If there isn't a fl_pid don't display who is waiting on > - * the lock if we are called from locks_show, or if we are > - * called from __show_fd_info - skip lock entirely > + * If lock owner is dead (and pid is freed) or not visible in current > + * pidns, zero is shown as a pid value. Check lock info from > + * init_pid_ns to get saved lock pid value. > */ > - if (fl_pid == 0) > - return; > > if (fl->fl_file != NULL) > inode = locks_inode(fl->fl_file); (cc'ing Nickolay) As Andrey points out, this behavior was originally added in commit d67fd44f697d to address performance issues when there are a lot of locks held by tasks in other namespaces. Will allowing this code to show these again cause a problem there?
On Thu, Jun 14, 2018 at 07:00:07AM -0400, Jeff Layton wrote: > On Fri, 2018-06-08 at 17:27 +0300, Konstantin Khorenko wrote: > > Currently if we face a lock taken by a process invisible in the current > > pidns we skip the lock completely, but this > > > > 1) makes the output not that nice > > (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3 > > pos: 4 > > flags: 02100002 > > mnt_id: 257 > > lock: (root@vz7)/: > > > > 2) makes it more difficult to debug issues with leaked flocks > > if you get error on lock, but don't see any locks in /proc/$id/fdinfo/$file > > > > Let's show information about such locks again as previously, but > > show zero in the owner pid field. > > > > After the patch: > > =============== > > (root@vz7)/:cat /proc/${PID_A2}/fdinfo/3 > > pos: 4 > > flags: 02100002 > > mnt_id: 295 > > lock: 1: FLOCK ADVISORY WRITE 0 b6:f8a61:529946 0 EOF > > > > Fixes: 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks") > > Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com> > > --- > > fs/locks.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/fs/locks.c b/fs/locks.c > > index bfee5b7f2862..e533623e2e99 100644 > > --- a/fs/locks.c > > +++ b/fs/locks.c > > @@ -2633,12 +2633,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > > > > fl_pid = locks_translate_pid(fl, proc_pidns); > > /* > > - * If there isn't a fl_pid don't display who is waiting on > > - * the lock if we are called from locks_show, or if we are > > - * called from __show_fd_info - skip lock entirely > > + * If lock owner is dead (and pid is freed) or not visible in current > > + * pidns, zero is shown as a pid value. Check lock info from > > + * init_pid_ns to get saved lock pid value. > > */ > > - if (fl_pid == 0) > > - return; > > > > if (fl->fl_file != NULL) > > inode = locks_inode(fl->fl_file); > > (cc'ing Nickolay) > > As Andrey points out, this behavior was originally added in commit > d67fd44f697d to address performance issues when there are a lot of locks > held by tasks in other namespaces. > > Will allowing this code to show these again cause a problem there? No, it will not. The content of /proc/locks is still filtered. As for fdinfo, it shows locks for one file descriptor, there will not be a lot of locks. And fdinfo was designed to show all locks for a file descriptor, it doesn't matter in what pidns they were taken. Thanks, Andrei > -- > Jeff Layton <jlayton@kernel.org>
===============
(root@vz7)/:cat /proc/${PID_A2}/fdinfo/3
pos: 4
flags: 02100002
mnt_id: 295
lock: 1: FLOCK ADVISORY WRITE 0 b6:f8a61:529946 0 EOF
Fixes: 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks")
Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
---
fs/locks.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index bfee5b7f2862..e533623e2e99 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2633,12 +2633,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
fl_pid = locks_translate_pid(fl, proc_pidns);
/*
- * If there isn't a fl_pid don't display who is waiting on
- * the lock if we are called from locks_show, or if we are
- * called from __show_fd_info - skip lock entirely
+ * If lock owner is dead (and pid is freed) or not visible in current
+ * pidns, zero is shown as a pid value. Check lock info from
+ * init_pid_ns to get saved lock pid value.
*/
- if (fl_pid == 0)
- return;
if (fl->fl_file != NULL)
inode = locks_inode(fl->fl_file);
Currently if we face a lock taken by a process invisible in the current pidns we skip the lock completely, but this 1) makes the output not that nice (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3 pos: 4 flags: 02100002 mnt_id: 257 lock: (root@vz7)/: 2) makes it more difficult to debug issues with leaked flocks if you get error on lock, but don't see any locks in /proc/$id/fdinfo/$file Let's show information about such locks again as previously, but show zero in the owner pid field. After the patch: