Re: [PATCH 2/2] fs/lock: show locks taken by processes from another pidns
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: 0212 > > 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: 0212 > > 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 > > --- > > 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
Re: [PATCH 2/2] fs/lock: show locks taken by processes from another pidns
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: 0212 > > 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: 0212 > > 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 > > --- > > 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
Re: [PATCH 2/2] fs/lock: show locks taken by processes from another pidns
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: 0212 > 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: 0212 > 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 > --- > 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? -- Jeff Layton
Re: [PATCH 2/2] fs/lock: show locks taken by processes from another pidns
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: 0212 > 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: 0212 > 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 > --- > 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? -- Jeff Layton
Re: [PATCH 2/2] fs/lock: show locks taken by processes from another pidns
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: 0212 > 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: 0212 > 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 Thanks, Andrei > Fixes: 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific l_pid for > remote locks") > Signed-off-by: Konstantin Khorenko > --- > 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 >
Re: [PATCH 2/2] fs/lock: show locks taken by processes from another pidns
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: 0212 > 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: 0212 > 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 Thanks, Andrei > Fixes: 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific l_pid for > remote locks") > Signed-off-by: Konstantin Khorenko > --- > 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 >
[PATCH 2/2] fs/lock: show locks taken by processes from another pidns
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: 0212 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: 0212 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 --- 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
[PATCH 2/2] fs/lock: show locks taken by processes from another pidns
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: 0212 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: 0212 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 --- 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