[PATCH] fs/locks: print full locks information

[PATCH] fs/locks: print full locks information

[Luo Longjun]

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(-)

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);
 	}
 }
 
-- 
2.17.1

Re: [PATCH] fs/locks: print full locks information

[Jeff Layton]

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);
>  	}
>  }
>  
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] fs/locks: print full locks information

[Al Viro]

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...

Re: [PATCH] fs/locks: print full locks information

[Jeff Layton]

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.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] fs/locks: print full locks information

[J. Bruce Fields]

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.

[PATCH v3] fs/locks: print full locks information

[Luo Longjun]

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.

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

diff --git a/fs/locks.c b/fs/locks.c
index 99ca97e81b7a..ecaecd1f1b58 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2828,7 +2828,7 @@ 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;
 	unsigned int fl_pid;
@@ -2844,7 +2844,11 @@ 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);
+
+	if (repeat)
+		seq_printf(f, "%*s", repeat - 1 + (int)strlen(pfx), pfx);
+
 	if (IS_POSIX(fl)) {
 		if (fl->fl_flags & FL_ACCESS)
 			seq_puts(f, "ACCESS");
@@ -2906,21 +2910,64 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 	}
 }
 
+static struct file_lock *get_next_blocked_member(struct file_lock *node)
+{
+	struct file_lock *tmp;
+
+	/* NULL node or root node */
+	if (node == NULL || node->fl_blocker == NULL)
+		return NULL;
+
+	/* Next member in the linked list could be itself */
+	tmp = list_next_entry(node, fl_blocked_member);
+	if (list_entry_is_head(tmp, &node->fl_blocker->fl_blocked_requests, fl_blocked_member)
+		|| tmp == node) {
+		return NULL;
+	}
+
+	return tmp;
+}
+
 static int locks_show(struct seq_file *f, void *v)
 {
 	struct locks_iterator *iter = f->private;
-	struct file_lock *fl, *bfl;
+	struct file_lock *cur, *tmp;
 	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
+	int level = 0;
 
-	fl = hlist_entry(v, struct file_lock, fl_link);
+	cur = hlist_entry(v, struct file_lock, fl_link);
 
-	if (locks_translate_pid(fl, proc_pidns) == 0)
+	if (locks_translate_pid(cur, proc_pidns) == 0)
 		return 0;
 
-	lock_get_status(f, fl, iter->li_pos, "");
+	/* View this crossed linked list as a binary tree, the first member of fl_blocked_requests
+	 * is the left child of current node, the next silibing in fl_blocked_member is the
+	 * right child, we can alse get the parent of current node from fl_blocker, so this
+	 * question becomes traversal of a binary tree
+	 */
+	while (cur != NULL) {
+		if (level)
+			lock_get_status(f, cur, iter->li_pos, "-> ", level);
+		else
+			lock_get_status(f, cur, iter->li_pos, "", level);
 
-	list_for_each_entry(bfl, &fl->fl_blocked_requests, fl_blocked_member)
-		lock_get_status(f, bfl, iter->li_pos, " ->");
+		if (!list_empty(&cur->fl_blocked_requests)) {
+			/* Turn left */
+			cur = list_first_entry_or_null(&cur->fl_blocked_requests,
+				struct file_lock, fl_blocked_member);
+			level++;
+		} else {
+			/* Turn right */
+			tmp = get_next_blocked_member(cur);
+			/* Fall back to parent node */
+			while (tmp == NULL && cur->fl_blocker != NULL) {
+				cur = cur->fl_blocker;
+				level--;
+				tmp = get_next_blocked_member(cur);
+			}
+			cur = tmp;
+		}
+	}
 
 	return 0;
 }
@@ -2941,7 +2988,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);
 	}
 }
 
-- 
2.17.1

Re: [PATCH v3] fs/locks: print full locks information

[Jeff Layton]

On Thu, 2021-02-25 at 22:58 -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.
> 
> Signed-off-by: Luo Longjun <luolongjun@huawei.com>
> ---
>  fs/locks.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 56 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 99ca97e81b7a..ecaecd1f1b58 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2828,7 +2828,7 @@ 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;
>  	unsigned int fl_pid;
> @@ -2844,7 +2844,11 @@ 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);
> +
> +	if (repeat)
> +		seq_printf(f, "%*s", repeat - 1 + (int)strlen(pfx), pfx);

Shouldn't that be "%.*s" ?

Also, isn't this likely to end up walking past the end of "pfx" (or even
ending up at an address before the buffer)? You have this below:

    lock_get_status(f, fl, *id, "", 0);

...so the "length" value you're passing into the format there is going
to be -1. It also seems like if you get a large "level" value in
locks_show, then you'll end up with a length that is much longer than
the actual string.

> +
>  	if (IS_POSIX(fl)) {
>  		if (fl->fl_flags & FL_ACCESS)
>  			seq_puts(f, "ACCESS");
> @@ -2906,21 +2910,64 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>  	}
>  }
>  
> 
> 
> 
> +static struct file_lock *get_next_blocked_member(struct file_lock *node)
> +{
> +	struct file_lock *tmp;
> +
> +	/* NULL node or root node */
> +	if (node == NULL || node->fl_blocker == NULL)
> +		return NULL;
> +
> +	/* Next member in the linked list could be itself */
> +	tmp = list_next_entry(node, fl_blocked_member);
> +	if (list_entry_is_head(tmp, &node->fl_blocker->fl_blocked_requests, fl_blocked_member)
> +		|| tmp == node) {
> +		return NULL;
> +	}
> +
> +	return tmp;
> +}
> +
>  static int locks_show(struct seq_file *f, void *v)
>  {
>  	struct locks_iterator *iter = f->private;
> -	struct file_lock *fl, *bfl;
> +	struct file_lock *cur, *tmp;
>  	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
> +	int level = 0;
>  
> 
> 
> 
> -	fl = hlist_entry(v, struct file_lock, fl_link);
> +	cur = hlist_entry(v, struct file_lock, fl_link);
>  
> 
> 
> 
> -	if (locks_translate_pid(fl, proc_pidns) == 0)
> +	if (locks_translate_pid(cur, proc_pidns) == 0)
>  		return 0;
>  
> 
> 
> 
> -	lock_get_status(f, fl, iter->li_pos, "");
> +	/* View this crossed linked list as a binary tree, the first member of fl_blocked_requests
> +	 * is the left child of current node, the next silibing in fl_blocked_member is the
> +	 * right child, we can alse get the parent of current node from fl_blocker, so this
> +	 * question becomes traversal of a binary tree
> +	 */
> +	while (cur != NULL) {
> +		if (level)
> +			lock_get_status(f, cur, iter->li_pos, "-> ", level);
> +		else
> +			lock_get_status(f, cur, iter->li_pos, "", level);
>  
> 
> 
> 
> -	list_for_each_entry(bfl, &fl->fl_blocked_requests, fl_blocked_member)
> -		lock_get_status(f, bfl, iter->li_pos, " ->");
> +		if (!list_empty(&cur->fl_blocked_requests)) {
> +			/* Turn left */
> +			cur = list_first_entry_or_null(&cur->fl_blocked_requests,
> +				struct file_lock, fl_blocked_member);
> +			level++;
> +		} else {
> +			/* Turn right */
> +			tmp = get_next_blocked_member(cur);
> +			/* Fall back to parent node */
> +			while (tmp == NULL && cur->fl_blocker != NULL) {
> +				cur = cur->fl_blocker;
> +				level--;
> +				tmp = get_next_blocked_member(cur);
> +			}
> +			cur = tmp;
> +		}
> +	}
>  
> 
> 
> 
>  	return 0;
>  }
> @@ -2941,7 +2988,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);
>  	}
>  }
>  
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3] fs/locks: print full locks information

[Luo Longjun]

在 2021/3/9 21:37, Jeff Layton 写道:
> On Thu, 2021-02-25 at 22:58 -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.
>>
>> Signed-off-by: Luo Longjun <luolongjun@huawei.com>
>> ---
>>   fs/locks.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 56 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/locks.c b/fs/locks.c
>> index 99ca97e81b7a..ecaecd1f1b58 100644
>> --- a/fs/locks.c
>> +++ b/fs/locks.c
>> @@ -2828,7 +2828,7 @@ 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;
>>   	unsigned int fl_pid;
>> @@ -2844,7 +2844,11 @@ 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);
>> +
>> +	if (repeat)
>> +		seq_printf(f, "%*s", repeat - 1 + (int)strlen(pfx), pfx);
> Shouldn't that be "%.*s" ?
>
> Also, isn't this likely to end up walking past the end of "pfx" (or even
> ending up at an address before the buffer)? You have this below:
>
>      lock_get_status(f, fl, *id, "", 0);
>
> ...so the "length" value you're passing into the format there is going
> to be -1. It also seems like if you get a large "level" value in
> locks_show, then you'll end up with a length that is much longer than
> the actual string.

In my understanding, the difference of "%*s" and "%.*s" is that, "%*s" 
specifies the minimal filed width while "%.*s" specifies the precision 
of the string.

Here, I use "%*s", because I want to print locks information in the 
follwing format:

2: FLOCK  ADVISORY  WRITE 110 00:02:493 0 EOF
2: -> FLOCK  ADVISORY  WRITE 111 00:02:493 0 EOF
2:  -> FLOCK  ADVISORY  WRITE 112 00:02:493 0 EOF
2:   -> FLOCK  ADVISORY  WRITE 113 00:02:493 0 EOF
2:    -> FLOCK  ADVISORY  WRITE 114 00:02:493 0 EOF

And also, there is another way to show there information, in the format 
like:

60: FLOCK  ADVISORY  WRITE 23350 08:02:4456514 0 EOF
60: -> FLOCK  ADVISORY  WRITE 23356 08:02:4456514 0 EOF
60: -> FLOCK  ADVISORY  WRITE 24217 08:02:4456514 0 EOF
60: -> FLOCK  ADVISORY  WRITE 24239 08:02:4456514 0 EOF

I think both formats are acceptable, but the first format shows 
competition relationships between these locks.

In the following code:

> lock_get_status(f, fl, *id, "", 0);

repeat is 0, and in the function:

+ if (repeat)
+		seq_printf(f, "%*s", repeat - 1 + (int)strlen(pfx), pfx);

The if branch will not take effect, so it could not be -1.

>> +
>>   	if (IS_POSIX(fl)) {
>>   		if (fl->fl_flags & FL_ACCESS)
>>   			seq_puts(f, "ACCESS");
>> @@ -2906,21 +2910,64 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>>   	}
>>   }
>>   
>>
>>
>>
>> +static struct file_lock *get_next_blocked_member(struct file_lock *node)
>> +{
>> +	struct file_lock *tmp;
>> +
>> +	/* NULL node or root node */
>> +	if (node == NULL || node->fl_blocker == NULL)
>> +		return NULL;
>> +
>> +	/* Next member in the linked list could be itself */
>> +	tmp = list_next_entry(node, fl_blocked_member);
>> +	if (list_entry_is_head(tmp, &node->fl_blocker->fl_blocked_requests, fl_blocked_member)
>> +		|| tmp == node) {
>> +		return NULL;
>> +	}
>> +
>> +	return tmp;
>> +}
>> +
>>   static int locks_show(struct seq_file *f, void *v)
>>   {
>>   	struct locks_iterator *iter = f->private;
>> -	struct file_lock *fl, *bfl;
>> +	struct file_lock *cur, *tmp;
>>   	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
>> +	int level = 0;
>>   
>>
>>
>>
>> -	fl = hlist_entry(v, struct file_lock, fl_link);
>> +	cur = hlist_entry(v, struct file_lock, fl_link);
>>   
>>
>>
>>
>> -	if (locks_translate_pid(fl, proc_pidns) == 0)
>> +	if (locks_translate_pid(cur, proc_pidns) == 0)
>>   		return 0;
>>   
>>
>>
>>
>> -	lock_get_status(f, fl, iter->li_pos, "");
>> +	/* View this crossed linked list as a binary tree, the first member of fl_blocked_requests
>> +	 * is the left child of current node, the next silibing in fl_blocked_member is the
>> +	 * right child, we can alse get the parent of current node from fl_blocker, so this
>> +	 * question becomes traversal of a binary tree
>> +	 */
>> +	while (cur != NULL) {
>> +		if (level)
>> +			lock_get_status(f, cur, iter->li_pos, "-> ", level);
>> +		else
>> +			lock_get_status(f, cur, iter->li_pos, "", level);
>>   
>>
>>
>>
>> -	list_for_each_entry(bfl, &fl->fl_blocked_requests, fl_blocked_member)
>> -		lock_get_status(f, bfl, iter->li_pos, " ->");
>> +		if (!list_empty(&cur->fl_blocked_requests)) {
>> +			/* Turn left */
>> +			cur = list_first_entry_or_null(&cur->fl_blocked_requests,
>> +				struct file_lock, fl_blocked_member);
>> +			level++;
>> +		} else {
>> +			/* Turn right */
>> +			tmp = get_next_blocked_member(cur);
>> +			/* Fall back to parent node */
>> +			while (tmp == NULL && cur->fl_blocker != NULL) {
>> +				cur = cur->fl_blocker;
>> +				level--;
>> +				tmp = get_next_blocked_member(cur);
>> +			}
>> +			cur = tmp;
>> +		}
>> +	}
>>   
>>
>>
>>
>>   	return 0;
>>   }
>> @@ -2941,7 +2988,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);
>>   	}
>>   }
>>   
>>
>>
>>

Re: [PATCH v3] fs/locks: print full locks information

[Jeff Layton]

On Thu, 2021-03-11 at 11:45 +0800, Luo Longjun wrote:
> 在 2021/3/9 21:37, Jeff Layton 写道:
> > On Thu, 2021-02-25 at 22:58 -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.
> > > 
> > > Signed-off-by: Luo Longjun <luolongjun@huawei.com>
> > > ---
> > >   fs/locks.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++--------
> > >   1 file changed, 56 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/fs/locks.c b/fs/locks.c
> > > index 99ca97e81b7a..ecaecd1f1b58 100644
> > > --- a/fs/locks.c
> > > +++ b/fs/locks.c
> > > @@ -2828,7 +2828,7 @@ 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;
> > >   	unsigned int fl_pid;
> > > @@ -2844,7 +2844,11 @@ 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);
> > > +
> > > +	if (repeat)
> > > +		seq_printf(f, "%*s", repeat - 1 + (int)strlen(pfx), pfx);
> > Shouldn't that be "%.*s" ?
> > 
> > Also, isn't this likely to end up walking past the end of "pfx" (or even
> > ending up at an address before the buffer)? You have this below:
> > 
> >      lock_get_status(f, fl, *id, "", 0);
> > 
> > ...so the "length" value you're passing into the format there is going
> > to be -1. It also seems like if you get a large "level" value in
> > locks_show, then you'll end up with a length that is much longer than
> > the actual string.
> 
> In my understanding, the difference of "%*s" and "%.*s" is that, "%*s" 
> specifies the minimal filed width while "%.*s" specifies the precision 
> of the string.
> 

Oh, right. I always forget about the first usage.

> Here, I use "%*s", because I want to print locks information in the 
> follwing format:
> 
> 2: FLOCK  ADVISORY  WRITE 110 00:02:493 0 EOF
> 2: -> FLOCK  ADVISORY  WRITE 111 00:02:493 0 EOF
> 2:  -> FLOCK  ADVISORY  WRITE 112 00:02:493 0 EOF
> 2:   -> FLOCK  ADVISORY  WRITE 113 00:02:493 0 EOF
> 2:    -> FLOCK  ADVISORY  WRITE 114 00:02:493 0 EOF
> 
> And also, there is another way to show there information, in the format 
> like:
> 
> 60: FLOCK  ADVISORY  WRITE 23350 08:02:4456514 0 EOF
> 60: -> FLOCK  ADVISORY  WRITE 23356 08:02:4456514 0 EOF
> 60: -> FLOCK  ADVISORY  WRITE 24217 08:02:4456514 0 EOF
> 60: -> FLOCK  ADVISORY  WRITE 24239 08:02:4456514 0 EOF
> 
> I think both formats are acceptable, but the first format shows 
> competition relationships between these locks.
> 

We might as well go with the one this patch implements. I like seeing
the chain of waiters as well, and it doesn't seem to break lslocks
(which is, to my knowledge, the only real programmatic consumer of this
file).

> In the following code:
> 
> > lock_get_status(f, fl, *id, "", 0);
> 
> repeat is 0, and in the function:
> 
> + if (repeat)
> +		seq_printf(f, "%*s", repeat - 1 + (int)strlen(pfx), pfx);
> 
> The if branch will not take effect, so it could not be -1.
> 


Good point.

Ok, I'll go ahead and put this one in linux-next for now. Assuming there
are no problems, it should make v5.13.

Thanks!
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v2 02/24] fs/locks: print full locks information

[Luo Longjun]

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 non-recursion DFS.

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

diff --git a/fs/locks.c b/fs/locks.c
index 99ca97e81b7a..fdf240626777 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2827,8 +2827,14 @@ struct locks_iterator {
 	loff_t	li_pos;
 };
 
+struct locks_traverse_list {
+	struct list_head head;
+	struct file_lock *lock;
+	int level;
+};
+
 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;
 	unsigned int fl_pid;
@@ -2844,7 +2850,11 @@ 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);
+
+	if (repeat)
+		seq_printf(f, "%*s", repeat - 1 + strlen(pfx), pfx);
+
 	if (IS_POSIX(fl)) {
 		if (fl->fl_flags & FL_ACCESS)
 			seq_puts(f, "ACCESS");
@@ -2912,17 +2922,66 @@ static int locks_show(struct seq_file *f, void *v)
 	struct file_lock *fl, *bfl;
 	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
 
+	struct list_head root;
+	struct list_head *tail = &root;
+	struct list_head *pos, *tmp;
+	struct locks_traverse_list *node, *node_child;
+
+	int ret = 0;
+
 	fl = hlist_entry(v, struct file_lock, fl_link);
 
 	if (locks_translate_pid(fl, proc_pidns) == 0)
-		return 0;
+		return ret;
+
+	INIT_LIST_HEAD(&root);
 
-	lock_get_status(f, fl, iter->li_pos, "");
+	node = kmalloc(sizeof(struct locks_traverse_list), GFP_KERNEL);
+	if (!node) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
-	list_for_each_entry(bfl, &fl->fl_blocked_requests, fl_blocked_member)
-		lock_get_status(f, bfl, iter->li_pos, " ->");
+	node->level = 0;
+	node->lock = fl;
+	list_add(&node->head, tail);
+	tail = &node->head;
 
-	return 0;
+	while (tail != &root) {
+		node = list_entry(tail, struct locks_traverse_list, head);
+		if (!node->level)
+			lock_get_status(f, node->lock, iter->li_pos, "", node->level);
+		else
+			lock_get_status(f, node->lock, iter->li_pos, "-> ", node->level);
+
+		tmp = tail->prev;
+		list_del(tail);
+		tail = tmp;
+
+		list_for_each_entry_reverse(bfl, &node->lock->fl_blocked_requests,
+						fl_blocked_member) {
+			node_child = kmalloc(sizeof(struct locks_traverse_list), GFP_KERNEL);
+			if (!node_child) {
+				ret = -ENOMEM;
+				goto out;
+			}
+
+			node_child->level = node->level + 1;
+			node_child->lock = bfl;
+			list_add(&node_child->head, tail);
+			tail = &node_child->head;
+		}
+		kfree(node);
+	}
+
+out:
+	list_for_each_safe(pos, tmp, &root) {
+		node = list_entry(pos, struct locks_traverse_list, head);
+		list_del(pos);
+		if (!node)
+			kfree(node);
+	}
+	return ret;
 }
 
 static void __show_fd_locks(struct seq_file *f,
@@ -2941,7 +3000,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);
 	}
 }
 
-- 
2.17.1

Re: [PATCH v2 02/24] fs/locks: print full locks information

[J. Bruce Fields]

On Wed, Feb 24, 2021 at 03:35:44AM -0500, Luo Longjun wrote:
> @@ -2912,17 +2922,66 @@ static int locks_show(struct seq_file *f, void *v)
>  	struct file_lock *fl, *bfl;
>  	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
>  
> +	struct list_head root;
> +	struct list_head *tail = &root;
> +	struct list_head *pos, *tmp;
> +	struct locks_traverse_list *node, *node_child;
> +
> +	int ret = 0;
> +
>  	fl = hlist_entry(v, struct file_lock, fl_link);
>  
>  	if (locks_translate_pid(fl, proc_pidns) == 0)
> -		return 0;
> +		return ret;
> +
> +	INIT_LIST_HEAD(&root);
>  
> -	lock_get_status(f, fl, iter->li_pos, "");
> +	node = kmalloc(sizeof(struct locks_traverse_list), GFP_KERNEL);

Is it safe to allocate here?  I thought this was under the
blocked_lock_lock spinlock.

And I still don't think you need a stack.  Have you tried the suggestion
in my previous mail?

--b.