Re: [PATCH] fs/locks: print full locks information
On Sun, Feb 21, 2021 at 01:43:03PM -0500, Jeff Layton wrote: > On Sun, 2021-02-21 at 16:52 +, Al Viro wrote: > > On Sat, Feb 20, 2021 at 01:32:50AM -0500, Luo Longjun wrote: > > > + list_for_each_entry(bfl, >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.
Re: [PATCH] fs/locks: print full locks information
On Sun, 2021-02-21 at 16:52 +, 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_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
Re: [PATCH] fs/locks: print full locks information
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_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
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
> ---
> 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_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_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
[PATCH] fs/locks: print full locks information
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
---
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_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_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
