diff mbox series

fs/locks: print full locks information

Message ID 20210220063250.742164-1-luolongjun@huawei.com (mailing list archive)
State New
Headers show
Series fs/locks: print full locks information | expand

Commit Message

Luo Longjun Feb. 20, 2021, 6:32 a.m. UTC
Commit fd7732e033e3 ("fs/locks: create a tree of dependent requests.")
has put blocked locks into a tree.

So, with a for loop, we can't check all locks information.

To solve this problem, we should traverse the tree by DFS.

Signed-off-by: Luo Longjun <luolongjun@huawei.com>
---
 fs/locks.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

Comments

Jeff Layton Feb. 21, 2021, 4:34 p.m. UTC | #1
On Sat, 2021-02-20 at 01:32 -0500, Luo Longjun wrote:
> Commit fd7732e033e3 ("fs/locks: create a tree of dependent requests.")
> has put blocked locks into a tree.
> 
> So, with a for loop, we can't check all locks information.
> 
> To solve this problem, we should traverse the tree by DFS.
> 
> Signed-off-by: Luo Longjun <luolongjun@huawei.com>
> ---
>  fs/locks.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 

(cc'ing Neil B.)

It is true that you don't see the full set of blocked locks here like
you used to. This patch doesn't exactly restore the old behavior either
though. You end up with a tree of blocked locks like this in the output
when things are lined up behind a single lock:

1: POSIX  ADVISORY  WRITE 1553 00:21:27 0 EOF
1: -> POSIX  ADVISORY  WRITE 1629 00:21:27 0 EOF
1:  -> POSIX  ADVISORY  WRITE 1577 00:21:27 0 EOF
1:   -> POSIX  ADVISORY  WRITE 1610 00:21:27 0 EOF
1:    -> POSIX  ADVISORY  WRITE 1595 00:21:27 0 EOF
1:     -> POSIX  ADVISORY  WRITE 1568 00:21:27 0 EOF
1:      -> POSIX  ADVISORY  WRITE 1639 00:21:27 0 EOF

That's not necessarily wrong, since that does represent how things are
organized now, but it is different in that before you'd see everything
lined up behind the first lock rather than a chain of them like this.

/proc/locks turns out to be one of the more troublesome parts of this
code, as there really is no guiding design behind it, and it's hard to
know whether changes there will break userland.

The only tool that I know of that _really_ depends on /proc/locks is
lslocks, and I think it should work fine in the face of this patch. So,
I'm inclined to take it, unless anyone has objections.

I'll plan to pull it into -next for now, and this should make v5.13
(unless you think it needs to go in sooner).

Thanks,
Jeff

> diff --git a/fs/locks.c b/fs/locks.c
> index 99ca97e81b7a..1f7b6683ed54 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2828,9 +2828,10 @@ struct locks_iterator {
>  };
>  
> 
> 
> 
>  static void lock_get_status(struct seq_file *f, struct file_lock *fl,
> -			    loff_t id, char *pfx)
> +			    loff_t id, char *pfx, int repeat)
>  {
>  	struct inode *inode = NULL;
> +	int i;
>  	unsigned int fl_pid;
>  	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
>  
> 
> 
> 
> @@ -2844,7 +2845,13 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>  	if (fl->fl_file != NULL)
>  		inode = locks_inode(fl->fl_file);
>  
> 
> 
> 
> -	seq_printf(f, "%lld:%s ", id, pfx);
> +	seq_printf(f, "%lld: ", id);
> +	for (i = 1; i < repeat; i++)
> +		seq_puts(f, " ");
> +
> +	if (repeat)
> +		seq_printf(f, "%s", pfx);
> +
>  	if (IS_POSIX(fl)) {
>  		if (fl->fl_flags & FL_ACCESS)
>  			seq_puts(f, "ACCESS");
> @@ -2906,6 +2913,19 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>  	}
>  }
>  
> 
> 
> 
> +static int __locks_show(struct seq_file *f, struct file_lock *fl, int level)
> +{
> +	struct locks_iterator *iter = f->private;
> +	struct file_lock *bfl;
> +
> +	lock_get_status(f, fl, iter->li_pos, "-> ", level);
> +
> +	list_for_each_entry(bfl, &fl->fl_blocked_requests, fl_blocked_member)
> +		__locks_show(f, bfl, level + 1);
> +
> +	return 0;
> +}
> +
>  static int locks_show(struct seq_file *f, void *v)
>  {
>  	struct locks_iterator *iter = f->private;
> @@ -2917,10 +2937,10 @@ static int locks_show(struct seq_file *f, void *v)
>  	if (locks_translate_pid(fl, proc_pidns) == 0)
>  		return 0;
>  
> 
> 
> 
> -	lock_get_status(f, fl, iter->li_pos, "");
> +	lock_get_status(f, fl, iter->li_pos, "", 0);
>  
> 
> 
> 
>  	list_for_each_entry(bfl, &fl->fl_blocked_requests, fl_blocked_member)
> -		lock_get_status(f, bfl, iter->li_pos, " ->");
> +		__locks_show(f, bfl, 1);
>  
> 
> 
> 
>  	return 0;
>  }
> @@ -2941,7 +2961,7 @@ static void __show_fd_locks(struct seq_file *f,
>  
> 
> 
> 
>  		(*id)++;
>  		seq_puts(f, "lock:\t");
> -		lock_get_status(f, fl, *id, "");
> +		lock_get_status(f, fl, *id, "", 0);
>  	}
>  }
>  
> 
> 
>
Al Viro Feb. 21, 2021, 4:52 p.m. UTC | #2
On Sat, Feb 20, 2021 at 01:32:50AM -0500, Luo Longjun wrote:
>  
> @@ -2844,7 +2845,13 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>  	if (fl->fl_file != NULL)
>  		inode = locks_inode(fl->fl_file);
>  
> -	seq_printf(f, "%lld:%s ", id, pfx);
> +	seq_printf(f, "%lld: ", id);
> +	for (i = 1; i < repeat; i++)
> +		seq_puts(f, " ");
> +
> +	if (repeat)
> +		seq_printf(f, "%s", pfx);

RTFCStandard(printf, %*s), please

> +static int __locks_show(struct seq_file *f, struct file_lock *fl, int level)
> +{
> +	struct locks_iterator *iter = f->private;
> +	struct file_lock *bfl;
> +
> +	lock_get_status(f, fl, iter->li_pos, "-> ", level);
> +
> +	list_for_each_entry(bfl, &fl->fl_blocked_requests, fl_blocked_member)
> +		__locks_show(f, bfl, level + 1);

Er...  What's the maximal depth, again?  Kernel stack is very much finite...
Jeff Layton Feb. 21, 2021, 6:43 p.m. UTC | #3
On Sun, 2021-02-21 at 16:52 +0000, Al Viro wrote:
> On Sat, Feb 20, 2021 at 01:32:50AM -0500, Luo Longjun wrote:
> >  
> > 
> > @@ -2844,7 +2845,13 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
> >  	if (fl->fl_file != NULL)
> >  		inode = locks_inode(fl->fl_file);
> >  
> > 
> > -	seq_printf(f, "%lld:%s ", id, pfx);
> > +	seq_printf(f, "%lld: ", id);
> > +	for (i = 1; i < repeat; i++)
> > +		seq_puts(f, " ");
> > +
> > +	if (repeat)
> > +		seq_printf(f, "%s", pfx);
> 
> RTFCStandard(printf, %*s), please
> 
> > +static int __locks_show(struct seq_file *f, struct file_lock *fl, int level)
> > +{
> > +	struct locks_iterator *iter = f->private;
> > +	struct file_lock *bfl;
> > +
> > +	lock_get_status(f, fl, iter->li_pos, "-> ", level);
> > +
> > +	list_for_each_entry(bfl, &fl->fl_blocked_requests, fl_blocked_member)
> > +		__locks_show(f, bfl, level + 1);
> 
> Er...  What's the maximal depth, again?  Kernel stack is very much finite...

Ooof, good point. I don't think there is a maximal depth on the tree
itself. If you do want to do something like this, then you'd need to
impose a hard limit on the recursion somehow.
J . Bruce Fields Feb. 21, 2021, 8:10 p.m. UTC | #4
On Sun, Feb 21, 2021 at 01:43:03PM -0500, Jeff Layton wrote:
> On Sun, 2021-02-21 at 16:52 +0000, Al Viro wrote:
> > On Sat, Feb 20, 2021 at 01:32:50AM -0500, Luo Longjun wrote:
> > > +	list_for_each_entry(bfl, &fl->fl_blocked_requests, fl_blocked_member)
> > > +		__locks_show(f, bfl, level + 1);
> > 
> > Er...  What's the maximal depth, again?  Kernel stack is very much finite...
> 
> Ooof, good point. I don't think there is a maximal depth on the tree
> itself. If you do want to do something like this, then you'd need to
> impose a hard limit on the recursion somehow.

I think all you need to do is something like: follow the first entry of
fl_blocked_requests, printing as you go, until you get down to lock with
empty fl_blocked_requests (a leaf of the tree).  When you get to a leaf,
print, then follow fl_blocker back up and look for your parent's next
sibling on its fl_blocked_requests list.  If there are no more siblings,
continue up to your grandparent, etc.

It's the traverse-a-maze-by-always-turning-left algorithm applied to a
tree.  I think we do it elsewhere in the VFS.

You also need an integer that keeps track of your current indent depth.
But you don't need a stack.

?

--b.
diff mbox series

Patch

diff --git a/fs/locks.c b/fs/locks.c
index 99ca97e81b7a..1f7b6683ed54 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2828,9 +2828,10 @@  struct locks_iterator {
 };
 
 static void lock_get_status(struct seq_file *f, struct file_lock *fl,
-			    loff_t id, char *pfx)
+			    loff_t id, char *pfx, int repeat)
 {
 	struct inode *inode = NULL;
+	int i;
 	unsigned int fl_pid;
 	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
 
@@ -2844,7 +2845,13 @@  static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 	if (fl->fl_file != NULL)
 		inode = locks_inode(fl->fl_file);
 
-	seq_printf(f, "%lld:%s ", id, pfx);
+	seq_printf(f, "%lld: ", id);
+	for (i = 1; i < repeat; i++)
+		seq_puts(f, " ");
+
+	if (repeat)
+		seq_printf(f, "%s", pfx);
+
 	if (IS_POSIX(fl)) {
 		if (fl->fl_flags & FL_ACCESS)
 			seq_puts(f, "ACCESS");
@@ -2906,6 +2913,19 @@  static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 	}
 }
 
+static int __locks_show(struct seq_file *f, struct file_lock *fl, int level)
+{
+	struct locks_iterator *iter = f->private;
+	struct file_lock *bfl;
+
+	lock_get_status(f, fl, iter->li_pos, "-> ", level);
+
+	list_for_each_entry(bfl, &fl->fl_blocked_requests, fl_blocked_member)
+		__locks_show(f, bfl, level + 1);
+
+	return 0;
+}
+
 static int locks_show(struct seq_file *f, void *v)
 {
 	struct locks_iterator *iter = f->private;
@@ -2917,10 +2937,10 @@  static int locks_show(struct seq_file *f, void *v)
 	if (locks_translate_pid(fl, proc_pidns) == 0)
 		return 0;
 
-	lock_get_status(f, fl, iter->li_pos, "");
+	lock_get_status(f, fl, iter->li_pos, "", 0);
 
 	list_for_each_entry(bfl, &fl->fl_blocked_requests, fl_blocked_member)
-		lock_get_status(f, bfl, iter->li_pos, " ->");
+		__locks_show(f, bfl, 1);
 
 	return 0;
 }
@@ -2941,7 +2961,7 @@  static void __show_fd_locks(struct seq_file *f,
 
 		(*id)++;
 		seq_puts(f, "lock:\t");
-		lock_get_status(f, fl, *id, "");
+		lock_get_status(f, fl, *id, "", 0);
 	}
 }